[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#564441: new version of the proposed patch

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 
(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 
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 

* 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.


Attachment: iso-scan_review.tgz
Description: application/tgz

Reply to: