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. 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. The most invasive change is the restructuring of the state engine, but even that changes your new functionality only in very minor ways. But I hope you'll agree that with my changes it's more structured and readable. Although my version of the script has more lines, it is actually roughly the same size as your version. I've not yet reviewed the template texts yet as that's better delayed until we have the user interface finalized. I've attached the patches as an incremental series because that way you can better see why I made individual changes. I've added a number of FIXMEs in the code. Some relate to your changes, but some are more general questions that should be considered now we're doing a major overhaul anyway. Before we discuss any changes to the user interface I want to give you a chance to read through my changes. Please let me know if you have questions or don't agree with something I've done. On Thursday 28 January 2010, Fred wrote: > There are some areas where I'm not sure the proposed patch is fully ok : > - the templates are definitely not best written texts See above. > - about suggestions from F. Pop, I didn't know how to handle 'preseed', > as I don't know how it works Actually, I think that will mostly just work. Users should even be able to preseed a specific device (or list of devices) to scan, and even using both normal and persistent device names. My patch 0013 addresses that a bit. Most important is to ensure that users will not end up in an endless loop, for example if no ISO can be found. > - then looking at the second pass for ISOs in sub-directories, I wonder > if I had to give also the top-directories' detected ISO ; I left them > in the final list, but perhaps it isn't the best choice. There was a bug there. After the second pass images in a top level directory would be listed twice. My patch 0010 solved that, but with that other images would disappear after the second pass. So I came up with 0011 which is a pretty good implementation I think. See the commit comments for details. There were a few important issues with your patch. * It made some dialogs untranslatable You'd hardcoded a number of changes to dialogs in a way that could not be translated. I've fixed the device selection dialog (the "Choices-C" mechanism I've used there is really excellent), but not yet the ISO selection dialog. Essentially you cannot make *any* assumptions about how a translation will look. Because of that it's not possible to change parts of sentences on the fly. We can however quite easily use different paragraphs in different situations. Localechooser has a lot of examples of that. * Backing up to the main menu failed Instead you'd end up endlessly doing the first state. My patch 0012 fixes that. (The first state could even be moved out of the state engine, but I've left it in as that keeps the option open to implement a "scan for new devices" option. I don't think we'll need that though.) * The state engine was rather complex You had a number of state variables that needed complex testing and sometimes referred back to the result of earlier states. The restructuring I've done is very much based on the state engine in localechooser. A general rule is to have only one db_input in a state (at the end) and that the next state should start by testing the result of that. And it's perfectly OK to have states that don't ask questions. Have a look and feel free to ask questions. * Suite not set in final dialog (see patch 0008) This could possibly be solved by creating a dir under /var/lib/ and saving the suite (or codename) in a file that has as its name the full path of the iso with s:/:_:g (or some other character). I've already committed the first patch of the series in the D-I repo. That's why I've included your own patch: it's rebased on top of that (without any other changes). You can just apply the patches one-by-one on top of current SVN using 'patch -p1', but, especially if you already know git, you could also consider doing the following (done from memory, so may not completely work): * Save my patches somewhere; delete the first one. $ mkdir iso-scan $ git svn init svn+ssh://svn.debian.org/svn/d-i/trunk/packages/iso-scan $ git svn fetch -r HEAD $ git checkout -b from-frans $ git am <path-to-my-patches>00*.patch $ git checkout -b my-new work And then you can easily make any incremental changes. Cheers, FJP
Attachment:
iso-scan_review.tgz
Description: application/tgz