Hi!
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.
I have written a report describing most of what I have done. An HTML
version can be found at:
http://people.debian.org/~lunar/fe_gtk_report.html
Follows a text version of the same report to ease discussions:
--- 8< ---
Report on a codebase cleanup effort for the GTK+ frontend of cdebconf
This report presents fe_gtk, an effort to cleanup and improve
readability of the code of the GTK+ frontend of cdebconf.
1 Introduction
This document presents the result of more than two weeks of nearly
full time work. I am quite happy with my achievements and I really
think they are worthwile for the debian-installer project.
I have put a lot of energy and heart into all this. I would be happy
to explain in every details all choices that I made during this work,
but this report is already too long, so please don't hesitate to ask.
:)
This work can be seen as a first attempt to improve the general code
quality of cdebconf. Other parts of the code and other frontends
might benefit from a similar process in the future.
1.1 Workflow
I am fully aware that this work was not really done in the best way
for free software, but after Attilio's reaction to my first
commits [1], it was just easier to work alone and present the final
result.
We could use the following process to integrate it:
* discuss, comment and test;
* make the needed changes to fe_gtk's branch;
* 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.
Even if I have been able to go through multiple installations and
cdebconf's test suite several time. I am also pretty sure that
Attilio or Frans will be able to spot regressions in my code. I have
not done any side by side comparisons of both codebase to spot every
details.
[1] http://lists.debian.org/debian-boot/2007/06/msg00444.html
1.2 Testing
The code is available in
svn://svn.debian.org/svn/d-i/people/lunar/fe_gtk. Also have a look at
the generated documentation [2].
You can test non d-i frontend by using the following commands:
$ svn co svn://svn.debian.org/svn/d-i/trunk/packages/cdebconf .
$ cd cdebconf
$ svn co svn://svn.debian.org/svn/d-i/people/lunar/fe_gtk \
src/modules/frontend/fe_gtk
$ ./configure --with-frontend=fe_gtk \
--with-conffile=./cdebconf.conf
$ cd src/test
$ # Edit cdebconf.conf according with the example found on
$ # doc/testing.txt. Configure "default_fe" to use "fe_gtk".
$ for t in *.templates; do ../debconf-loadtemplate debian $t; done
$ for t in *.config; do ../debconf $t; done
To create a d-i image using the new frontend, you will need to make
changes in the following packages:
* rootskel
Replace "gtk" by "fe_gtk" in:
+ src/lib/debian-installer.d/S60frontend
+ src/lib/debian-installer.d/S70menu-linux
* rootskel-gtk
Replace "gtk" by "fe_gtk" in:
+ src/lib/debian-installer.d/S61mouse-support-x86
+ src/lib/debian-installer.d/S61mouse-support-powerpc
+ src/usr/bin/gtk-set-font
+ src/usr/bin/gtk-set-theme
The logo also needs to be slightly modified to remove the border
around the red area.
* cdebconf
+ Adding the fe_gtk directory to src/modules/frontend.
+ Replace "gtk" by "fe_gtk" in DEB_FRONTENDS and
UDEB_FRONTENDS at the beginning of debian/rules.
+ Change gtk* to fe_gtk* in debian/cdebconf-gtk-udeb.install.
You can also use the mini.iso I have built for i386 [3] and amd64 [4].
[2] http://people.debian.org/~lunar/fe_gtk/doc/html/
[3] http://people.debian.org/~lunar/fe_gtk_mini_i386.iso
[4] http://people.debian.org/~lunar/fe_gtk_mini_amd64.iso
1.3 Some words about the coding style
It is not compliant with the style described in doc/coding.txt. But
that is the style that I am used to, and it was just all easier to
work with it in the first place.
The inspiration for my coding style [5] is still available online, if
you want to take a look.
I am open to normalize the code before integrating it. But I would be
inclined to get a broader opinion about at least:
} else {
versus:
}
else
{
I find the former much more readable!
Function were all renamed to start with a verb and variables are no
longer abbreviations. Lines have no more than 79 characters.
[5] http://etudiants.insia.org/~jbobbio/c-codingstyle.html
2 Architectural changes
A few architectural changes in the GTK+ frontend were made during the
refactoring. These changes were made to create a more clean, robust
and readable code.
2.1 Frontend has been renamed to fe_gtk
This is a solution for namespace polution: plugins are expected to
implement a function with the following name:
<frontend>_handler_<type>. Naming the plugin gtk means that a plugin
will look like, for example, gtk_handler_entropy which polutes the
gtk namespace that should be left for the GTK+ library.
You might argue that I'm nitpicking here... It was also more
convenient during the development to work in a separate directory.
2.2 fe_gtk namespace
Exported functions are all prefixed with fe_gtk. The only symbols
that do not respect this convention is the needed
debconf_frontend_module. :)
Having a clear namespace helps to quickly spot where to look at
various functions while digging in the code or reading stack traces.
2.3 Code has been split in different files
In order to better separate areas of concerns and to make it easier
to add new features in the future, the code has been spread out to
multiple files. Each C file, except fe_gtk.c, has a corresponding
header that defines an internal interface to the module.
Please refer to the generated file list [6] and the identifier map
below to have a better view on how the split was done.
The longest file have a total of 638 lines (including documentation
and header) vs. 1790 lines for the current code.
[6] http://people.debian.org/~lunar/fe_gtk/doc/html/files.html
2.4 Public API for plugins is in fe_gtk.h
A clear API was created for frontend plugins. This API is defined and
documented in fe_gtk.h [7]. This API was tested to be fine by writing
a new plugin (see below).
Please not that there is no mention of struct frontend_data in the
public API: a plugin should not be able to fiddle with the frontend
data structures.
A linker-script is also available to reduce the exported symbols to
those defined in this public API. The Makefile contains a commented
line enabling it, which unfortunately breaks when building the whole
package. I did not investigate why exactly.
[7] http://people.debian.org/~lunar/fe_gtk/doc/html/fe__gtk_8h.html
2.5 Separation of code specific to the debian-installer
Most of the development done here was actually done by building the
frontend against the standard GTK+ X11 library. In order to do so,
all code specific to the debian-installer is now isolated in one
specific module.
This module is only linked in when --enable-d-i has been given to the
./configure script. This option also adds a field to the private
handling in struct frontend_data and calls to fe_gtk_di_setup()
during frontend initialization, fe_gtk_di_shutdown() during frontend
shutdown and fe_gtk_di_run_dialog() on start of GO and PROGRESS.
(Note that I don't like the name of this last function, if someone
could make up a better name, that would be great.)
The screenshot module is compiled conditionally as I see no reason to
enable this feature on standard desktops.
2.6 Plugin handling
The current code tries to load a plugin to handle the current
questions eight times for each question (see gtk_go()).
This was replaced by a simple function, find_external_handler() which
cache plugins in an hash table after they have been loaded. Plugins
are unloaded when the corresponding entry is deleted from the cache.
2.7 select and multiselect handling
All select and multiselect handlers are now build as views of a
common GtkTreeModel. This really fits the GTK+ API better, as
GtkTreeView and GtkComboBox get their data from a GtkTreeModel.
The only exception is the handler of multiselect when there is
multiple questions asked in a GO and which uses checkboxes, but even
in this case, a GtkTreeModel is a convenient way to store the
relevant data.
The views are implemented in select_handlers.c [8].
The model is created and manipulated through functions defined in
choice_model.h [9], fe_gtk_choice_model_create_full() being the full
interface to create a new model from a given question. The model
stores the index, the value in canonical and translated form, and a
flag for the selection status. This enables to relevant setters to
fully abstract the view, which lead to prettier code.
The code is quite generic and clean in this regard: the previous code
duplication related to tree-like questions
(countrychooser/country-name and partman/choose_partition) is now
gone. Tree-like questions are now implemented through a predicate
function (see parent_predicate, is_disk(), is_country()) and
registered in special_questions.
This has a drawback though: GtkTreeView adds a small margin in all
case if the model used is a GtkTreeStore. This can quite easily be
worked around by adding a little bit more code, but it was not
worthing it in my opinion: I would be in favor of simplifying that
code, returning to a more simple GtkListStore and moving away
specific d-i bits to new plugin (a world map for countrychooser,
anyone? ;) ). See the present code as an experiment on how far it
was possible to avoid code duplication.
[8] http://people.debian.org/~lunar/fe_gtk/doc/html/select__handlers_8c.html
[9] http://people.debian.org/~lunar/fe_gtk/doc/html/choice__model_8h.html
2.8 Dynamic content for target box and action box
The target box is the container where are laid out question widgets
(for GO) and progress widgets (for PROGRESS). In current code, a
container for questions widgets and a container for progress where
both created during the frontend initialization. They are hidden or
shown when needed.
The action box is container for buttons found in the bottom of the
frontend. In the same spirit, buttons where created during plugin
initialization and hidden or disabled at appropriate time.
I have tried a different direction, and this is a pretty discutable
change: the target box and action box is filled and emptied when
appropriate.
The initial idea was to totally separate GO and PROGRESS handling.
Ultimately, this failed on my first run of pkgsel as GO is called
(package installation) while the PROGRESS about remaining packages is
running. But the resulting bug might prompt us to a new direction
which is detailed below.
Another point in making such change was to get rid of tricky
functions actually refreshing various labels on language change. A
lot of references and tricky code seems to be avoidable by using this
quite simple design.
This change allow plugins to have more control about buttons
displayed in the action box though. This might help us to improve the
usuability, but this is also worth more discussions.
2.9 Handling of keyboard shortcuts
The previous key_press_event() has been replaced by a more scalable
mechanism. A function, fe_gtk_add_global_key_handler() registers a
new global (associated to the main window) key handler. This handler
is tied to a specific widgets, and will be desactivated if the widget
is destroyed.
This change is also quite tied to the previous one. A plugin can
define new keyboard shortcuts associated with its buttons or its
entry widget. These shortcuts will then be removed automatically at
the end of the GO run.
3 Smaller changes
3.1 call_setters() no longer free setters
A separate function, free_setters() has been introduced to do the
job. This allow us to recover memory if the setters are not called
(e.g. "Go Back" has been pressed).
3.2 Compilation flags
The frontend is built with the following flags:
-funsigned-char -fstrict-aliasing -Wall -W -Werror -Wundef \
-Wcast-align -Wwrite-strings -Wsign-compare -Wno-unused-parameter \
-Winit-self -Wpointer-arith -Wredundant-decls \
-Wno-format-zero-length -Wmissing-prototypes \
-Wmissing-format-attribute
The meaning of each of these flags is explained in the Makefile.
The compiler can spot a wider range of issues with this flags, and it
has been pretty helpful already.
I would strongly advocate to progressively enable these flags for all
cdebconf code.
3.3 Changes in struct frontend_data
The data structure frontend_data is the holder for internal data of
the GTK+ frontend. It is defined in the current code in
cdebconf_gtk.h whereas it now lies in its own specific internal
header, fe_data.h [10].
The following changes were made:
* button_next, button_prev, button_screenshot, button_cancel were
all removed as there is no need to keep a reference to them.
* action_box was added to keep a reference to the button container.
* progress_bar, progress_bar_label, progress_bar_box were moved to
the opaque progress_data.
* button_val was renamed to answer.
* answer_mutex and asnwer_cond were added to replace the previously
global button_mutex and button_cond.
* event_listener was added to be able to join the thread processing
GTK+ events on shutdown.
* plugins was added to keep a cache of already loaded plugins.
* The opaque di_data was added to hold data specific to the
debian-installer.
[10] http://people.debian.org/~lunar/fe_gtk/doc/html/fe__data_8h.html
3.4 fe_gtk_get_text() and memory handling
The current code contains a get_text() function (renamed
fe_gtk_get_text()) which is basically a copy and paste of cdebconf's
question_get_text(). This function has been kept as an opportunity to
add an important fix regarding memory management: question_get_text()
does not call strdup() for the fallback string. The returned string
can then either come from both dynamic or static allocation at the
same time, which disallow correct memory management.
question_get_text() should be fixed and this function removed from
the GTK+ frontend.
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.
I implemented a somewhat different workaround: fe_gtk_set_title()
implements the method and update the frontend field like the default
one. It also triggers an update of the label when the frontend is not
built for d-i. When built with --enable-d-i, the label is update
update is done during fe_gtk_di_run_dialog() at the beginning of a
PROGRESS or a GO.
3.6 Changes in the common naming of variables
Arguments of type struct frontend * were renamed to a more meaningful
"fe" instead of "obj".
This is a discutable change as well, but "obj" sounded really
internal to me.
3.7 STRING_MAX_LENGTH was removed
I have seen no reasons to limit the size of entered answers for
string questions. So the STRING_MAX_LENGTH constraints were just
removed.
4 New features
Doing only refactoring became quite boring at some point and I
couldn't resist to add some small new features described here.
Most of them are experiments, both on user interface usuability and
on the soundness of the code that I was (re)writing.
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.
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].
[11] http://people.debian.org/~lunar/di_fe_gtk_1024x768.png
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.
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. :)
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 usuability standpoint, though.
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.
[12] http://people.debian.org/~lunar/di_fe_gtk.png
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.
5 Bonuses
To close this report, two bonuses that where found along the path...
:)
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.
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.
The current plugin for the newt frontend uses dlopen() to gain access
to three functions defined in newt.so. While this sounds reasonable
for only 3 functions, it sounds less reasonable for 12 or more.
We can fully avoid dlopen() and dlsym() calls if we explicitely link
the plugin to the frontend shared object. This is basically just
adding fe_gtk.so at the end of GCC's command line.
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...
It will depend on how we are going to solve this issue, but in all
cases, the same system should be adopted by all frontends.
[13] http://people.debian.org/~lunar/fe_gtk-plugin-entropy.c
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.
[14] http://people.debian.org/~lunar/di_fe_gtk_err.png
6 Map of current identifiers
Please refer to:
http://people.debian.org/~lunar/fe_gtk_report.html#map-of-current-identifiers
7 Statistics on code length
7.1 Current code
Number of functions: 48
Average LOC per function: 26.06
Median LOC per function: 10
Maximum LOC in a function: 137
sloccount: 1,441
7.2 Proposed code
Number of functions: 141
Average LOC per function: 11.78
Median LOC per function: 9
Maximum LOC in a function: 90
sloccount: 2,441
--- >8 ---
Cheers,
--
Jérémy Bobbio .''`.
lunar@debian.org : :Ⓐ : # apt-get install anarchism
`. `'`
`-
Attachment:
signature.asc
Description: Digital signature