Bug#564441: new version of the proposed patch
Hello Frans,
Le Wed, 3 Feb 2010 22:54:01 +0100,
Frans Pop <elendil@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.
Reply to: