On Sunday 08 July 2007 01:28, Jérémy Bobbio wrote:
> I never knew that it would take me so much time, but I now consider
> that the work I had started on improving the code of the GTK+ frontend
> of cdebconf is ready for a broader review.
Thanks for all the work Jérémy!
I'll not comment on the really technical side of the changes, mostly
because my grasp of C is not sufficient to have an opinion on them.
However, in general the refactoring seems like a good idea to me.
> --- 8< ---
>
> We could use the following process to integrate it:
> * discuss, comment and test;
Ack.
> * make the needed changes to fe_gtk's branch;
Ack.
> * once we agree that it can be integrated in the main branch,
> write small patches (or incremental commits) to slowly transform the
> current code into the result of the process.
Hmm. If the changes are so structural, I'm not sure that the effort of
creating patches to have a gradual changeover is worth it. I'd suggest
just getting the code in your branch into the state we want it and then
just merging it into trunk in one commit.
> I am open to normalize the code before integrating it. But I would
> be inclined to get a broader opinion about at least:
>
> } else {
[...]
> I find the former much more readable!
I agree that this would be an improvement. Colin?
> 2.3 Code has been split in different files
> The longest file have a total of 638 lines (including documentation
> and header) vs. 1790 lines for the current code.
The stats at the bottom show an increase in lines of code of 1000 lines,
or 70%. That seems excessive, but I suspect the true increase is more
subtle.
Could you provide sloccounts:
- excluding comments
- excluding completely new functionality (such as the entropy plugin
That would give a more realistic basis for comparison.
> 2.5 Separation of code specific to the debian-installer
> The screenshot module is compiled conditionally as I see no reason
> to enable this feature on standard desktops.
Agreed.
> 3.5 fe_gtk_set_title() reactivated
>
> The implementation of the set_title method of cdebconf was changed
> to the default one in r43372 to avoid spurious title changes during
> pkgsel.
Note that the TITLE command is deprecated in debconf anyway, although
there is at least one location in D-I (lowmem) where it is currently used
and, AFAICT, needed.
> 4.1 Banner adapts to various screen size
>
> The banner now adapts to any screen width. The logo is aligned on
> the left of the screen, and its right side is filled with the color
> defined in LOGO_BACKGROUND_COLOR.
I'm not sure that this is a good solution. With the current bannersetting
a fixed background color works, but this would not be true for more
complex banners as proposed in #414785 or [1], or with banners for
derived distributions using a completely different banner (such a Dzongha
Linux [2]).
Basically, hardcoding a color seems very wrong to me.
> 4.2 Main window adapts to various screen size
>
> WINDOW_HEIGHT and WINDOW_WIDTH constants were removed, as the screen
> height and width are are now dynamically retrieved through
> gdk_screen_get_height() and gdk_screen_get_width(). The frontend now
> works a bit better on higher-resolution [11].
IMO supporting higher resolutions will require a redesign of the
interface, for the following reasons:
- the text area becomes too wide, which means that lines of text become
too long which reduces readability
- the buttons get very isolated at the extreme bottom of the screen
In other words: yes, supporting higher resolutions is a great goal, but
not without making sure that the total presentation is still useable. IMO
we'd need to limit the effective usable area of the screen, for example
by defining a rectangular container within which all other elements are
positioned.
We'd probably also need a somewhat more interesting background outside
that container than our current grey.
[Next to sections switched]
> 4.4 Go Back as secondary button
>
> The Go Back button was set secondary. In GTK+ terminology, this
> means that it will be displayed on the opposite side of other buttons
> added to the action box.
>
> This make the frontend looks a bit more like the newt frontend. I am
> not sure if it's better on a usability standpoint, though.
I don't think it is, mainly because the buttons are now much too far
apart.
> 4.3 Screenshot button has been removed
>
> To be able to make more experiments with the interface, the
> screenshot button has been removed. F10 can be pressed to optain the
> same effect. The code to create a button is still in screenshot.c
> though, but commented out.
I don't think F10 is a good button to use for that as it is completely
non-intuitive. It would be nice to have the PrtScreen button as an
alternative way to activate a screenshot.
> Frans suggested on IRC that it could perfectly be an option in a
> menu. I would find it great, except that I don't really have an idea on
> where we could put it. :)
Yes, I do think that having a menu in D-I would be a perfect option.
It could contain not only the screenshot button, but for example also
the "extra" menu items (CD integrity check, Save debug logs, Change
debconf priority, etc.).
I am against removing the screenshot button while we do not have an
alternative method that is *visible* to the user.
However, making the screenshot button "secondary" and moving it to the
other side of the screen does seem like a good option to me.
> 4.5 Yes/No buttons for boolean questions
>
> When a boolean question is the only question in a GO, Yes and No
> buttons are used instead of radio buttons to get the answer.
>
> A screenshot is available [12] if you want to take a look on the
> result.
Looking at the screenshots, I don't think this is an improvement over
using the radio buttons. Again, the main reason is distance; in this case
distance from the question.
In the newt frontend, the buttons are immediately below the question.
Here, you have to make a visual journey of more that 20 cm (on my laptop)
to link the question with the buttons...
Again, not a bad option, but would require a significant redesign of how
dialogs are displayed.
> 4.6 glib logs to syslog
>
> Instead of being lost, logs written by GTK+ are now going to the
> global syslog. I hope this will allow us to track problems more
> easily.
Seems OK to me, especially since only real "errors" get redirected there,
so it does not completely clutter the syslog.
Would it be possible to redirect the other messages on VT1 to a file too,
possibly/preferably a different logfile?
> 5.1 An entropy plugin for the GTK+ frontend
>
> In order to test that the public API for the plugin was sound, I
> implemented a GTK+ version of the entropy plugin [13] found in
> cdebconf-entropy.
That's really excellent!
> It worked well during my tests but is not ready to be packaged for
> two reasons: I was unsure about reusing the templates of
> cdebconf-entropy-text and I also stumbled on a linking issue.
Hmm. Does that mean that you also ask the user to enter characters?
Wouldn't it be better to gain entropy through random mouse movement (or
at least offer that as an alternative)? In that case I doubt there would
be much overlap in templates.
> Where this leads to complication is in terms of packaging...
> Currently cdebconf-entropy Build-Depends on libdebconfclient0-dev to
> gain access to the necessary headers. But if we want to explicitely
> link to the plugin, it would need to Build-Depends on
> cdebconf-gtk-udeb which does not sound nice to me...
Build depending on a udeb is not possible.
The obvious solution seems to me to include the plugins in the cdebconf
source package, probably under a new dir ./modules/plugins.
Alternatively, maybe the stuff that you need to depend on could be
factored out into a separate small library per frontend?
> 5.2 Progress bar and questions?
>
> Beforing figuring out that during pkgsel, GO commands were issued
> during a PROGRESS, I got something which we could develop further:
> the popularity-contest question was asked below the progress
> bar [14].
>
> This could actually be great to keep the progress bar running during
> pkgsel while the various questions are being asked. This would not
> be so hard to do, IMHO, with the current design.
Worth further investigation IMO, especially as we have plenty of space on
the screen to play with.
Basically I guess it's not that different from displaying multiple
questions on the same screen, which we already do.
Cheers,
FJP
[1]http://wiki.debian.org/DebianDesktopArtwork/DebianInstallerEtch
[2]http://svn.debian.org/wsvn/d-i/people/pema/packages/rootskel-gtk/src/usr/share/graphics/logo_debian.png?op=file&rev=0&sc=0
Attachment:
pgpHmLs870JrI.pgp
Description: PGP signature