Hello Frans,
Le Wed, 3 Feb 2010 22:54:01 +0100,
Frans Pop <elen...@planet.nl> a écrit :
> Hello Fred,
>
> It took a bit of time, but I think you'll see why.
>
> I do really like the new functionality. I think we'll need to discuss
> the details of the user interface a bit more, but the concept
> definitely works. My compliments for your work.
Thanks for the compliments, much appreciated :-)
> Attached a patch series, mostly on top of your last patch. It looks
> like a lot, but you'll see that I've not actually changed your
> functionality that much. A lot of the changes are cleanups and coding
> style improvements. Also, some of the changes are improvements of
> existing code/functionality and don't really change your code. I did
> fix a few issues; more below.
> …
I reviewed your patches, quite nothing to note, it's a nicer and
cleaner code (and I know that terse code doesn't mean maintainable
one ;-).
I followed your suggestion to use git (git-svn) to study it, and one
more time, I noted I have to learn more this great tool and all stuff
around it.
Here is my small remarks/questions :
0004-Fix-some-l10n-issues-more-remaining.patch
* nice trick with ${var:+} : devlist=${devlist:+$devlist, }$dev
* what does mean ':sl3:' in debconf templates ? I didn't use the '_'
before 'description' because then doing it, then loading template inside
iso-scan.postinst (as a hack you shown me), I got empty strings.
Actually, I incorrectly put a ^ instead of _ just before building my
patch.
0006-Refactor-analyze_cd-to-return-the-description.patch
* the 'printf' call simplifies the ISO description, well done.
0007-Define-variables-local-in-functions.patch
* ok for 'local' use (I didn't find one so assume it wasn't used
here). About variable names, I only guessed that uppercase names were
for global variables, whereas lowercase were for local ones.
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 ?
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.
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 ?
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.
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'll try to look about other FIXme tags in the next days, and propose
patches if I can do nice code. 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.
with regards,
Fred.
--
To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
