|
|
| Applications |
| Mailing Lists |
| Miscellaneous |
| Operating Systems |
| Programming |
[long] An attempt to improve the code of the GTK+ frontend
Date: Sat Jul 07 2007 - 19:28:44 EDT
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
`. `'`
`-
-- To UNSUBSCRIBE, email to debian-boot-REQUEST@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
- application/pgp-signature attachment: Digital signature
This archive was generated by hypermail 2.1.8 : Mon Jul 09 2007 - 08:50:40 EDT
|
Contact Us Legal Notices Order Services Online Pantek Home Privacy Policy IT news Site Map Pantek Library |