Re: [long] An attempt to improve the code of the GTK+ frontend
Frans Pop wrote:
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.
Indeed, the code is technically well written / formatted
--- 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.
Probably, because of the huge number of changes, a single commit is a
more viable option
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?
I agree: code in GTK frontend is not very clean and needed some reworking
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.
I see that in current jeremy's proposal the screenshot button is
disabaled by default: i think we must somehow allow the user to take
screenshots using the mouse only.
In the past, that screenshot facility revealed to be very useful in
reporting issues with complex fonts, isuues that may arise when new
languages are added to the installer or we switch to new fontsets.
So, i suggest to put a small "Camera" icon in the bottom-left corner of
the screen (pherhaps displayed only in the case the installer is started
in expert mode ?)
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.
Because this looks like it's more related to the generic debconf
architecture, i cannot properly speak about it.
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.
fjp's observation are correct, probably a better solution has to be seeked.
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.
Probably a good idea would be taking advantage of hi-resolution
framebuffers (like PPC's) by using bigger fonts, so that the screen is
not left empty because of small size fonts.
Of course this would require some experimenting and usability testing.
[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.
I agree: the back and continue buttons should be close one to each other
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.
Currently PrtSc does take a screenshot in ppm format, but the problem in
taking screenshots using a key is that not all architectures have same
keys (e.g.: on PPCs there is no PrtSc key) and, some embedded deviced
may simply have no keyboard at all, but just a pointing device.
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.
I agree on both points ith fjp (menu and screenshot button)
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.
IIRC, the radio option was preferred over both pulldown menu and buttons .
More important, in current design GtkButtons are specifically reserved
for back/continue/screenshot butotns, so i suggest to never use
GtkButtons somewhere else.
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.
ok i agree
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!
Yes, that's something the GTK frontend lacks in order to be idempotent
with newt, very good!
<snip/>
On the reworking of SELECT handlers, i would temporarily move into a
separate file special handlers for countrychooser and partman, and
latermove that part of code into an appropriate plugin.
As i said before privately, i agree with re-designing the GTK frontend
code the way you described, provided you can support the new codebase
during Lenny release cycle.
regards
Attilio
Reply to: