Bug#564441: new version of the proposed patch

[Frans Pop <elendil@planet.nl>]

On Tuesday 09 February 2010, Fred wrote:
> Here is my small remarks/questions :
>
> 0004-Fix-some-l10n-issues-more-remaining.patch
> * what does mean ':sl3:' in debconf templates ?

It's a D-I specific comment that allows us to split PO files into 5 
different levels so translators can concentrate on the most important 
parts first. Don't worry about it :-)

> 0007-Define-variables-local-in-functions.patch
> * ok for 'local' use (I didn't find one so assume it wasn't used here).

It's more important when a script gets more complex.

>   About variable names, I only guessed that uppercase names were 
>   for global variables, whereas lowercase were for local ones.

Correct. That's the convention we use, but it isn't 100% consistent.

> 0010-Reset-ISOS_FOUND-to-avoid-displaying-duplicates-afte.patch
> * you're right for the bug. I wonder why we differentiate '.  and
>   first-level dirs ?

From memory: mostly because we want the progress display for both.
But it could be cleaned up. Something like:
   dirs="$(find . -maxdepth 1 -type d)"
   for dir in $dirs; do

That would also give the number of dirs for progress bar when scanning only 
one device: count=$(echo "$dirs" | wc -l)

> 0011-Alternative-implementation-of-directory-scan.patch
> * fixme about -e sufficient to detect broken symlinks : yes.
> * about using '-f' find flag to filter symlinks : I would not use
>   it in first pass, to allow symlinks in the top dir pointing to a
>   sub-level one, but use it in second pass to avoid duplicates.

That's a nice suggestion.

> 0012-Restructure-state-engine.patch
> * state 20 : why not choosing debconf level (between medium and high)
>   depending on number of ISO found, as in state 30 ?
> * state 30 : we ask user to choose which ISO to use even if there is
>   only one, isn't it ?

That's the part that still needs discussion. I haven't yet fully thought it 
through. Plan to send a proposal soon.

> I thought also that the first argument for scan_device_for_isos
> function should be a text arg like 'top'/'full' instead of 0/1,
> we'd have better readability and don't loose speed.

Doesn't make that much difference IMO.

I like the following for on/off switches (clean test/smallest code):
some_function() {
	condition=$1
	other_var=$2
	if [ "$condition" ]; then
	...
}

And then call it with:
	some_function 1 "$var"
	some_function "" "$var"

But this is also an option:
some_function() {
	condition=$1
	other_var=$2
	if $condition; then
	...
}

And then call with:
	some_function true "$var"
	some_function false "$var"

> One 'FIXme' you didn't add but spoke before was about ISO presentation
> when selecting one : perhaps we could better present devices (with
> disk name, partition labels if any) ?

I have already improved device selection. See [1]. My current version even 
shows the label of the device and the device/partition size.

For selection of the ISO I don't really think it's possible as the total 
description would get too long. And for that the description of the ISO 
itself should IMO be sufficient anyway.

I've also added an option to rescan devices in the device selection dialog.

> I also have to test your version more carefully in my Qemu setup, as
> first runs didn't run as expected, but anyway, I'm using your version
> now. 

Testing would be much appreciated. I did test with ISOs for different 
arches in different locations on 2 disks, but bugs are quite possible.

[1] http://lists.debian.org/debian-boot/2010/02/msg00102.html