Jérémy Bobbio wrote:
On Thu, Jun 21, 2007 at 10:39:24AM +0200, Attilio Fiandrotti wrote:Jérémy Bobbio wrote:I'd like to know What's the problem in having the gtk frontend in one single file: same is for newt and text frontends, are you planning a similar code refactoring for them too?The whole frontend is currently in one huge single C file. I'm proofreading the whole thing for refactoring, memory leaks, potentiel segfauts, etc.It might worth it as well. :) I was currently interested into improving the GTK+ frontend codebase because it seemed like the one the most likely to get improvements and new features. It's not a *real* problem to have everything in just one single 2500 lines long C file. But it's not what people usually call maintainable. :) Here's the statistics for the three frontend that were mentioned: 1347 newt/newt.c 923 text/text.c 2354 gtk/gtk.c (after my add of more lines to improve readability, though) GTK+ API is quite verbose and if we keep adding new debconf question type to improve the installer usuability, it might just become really too long to be humanly apprehendable. :) Another improvement in spliting the current file to more separated modules is to get more defined interfaces and area of concerns. In the process of refactoring into more modules, you have to define clear interfaces and the whole process really helps to improve code readability (and add documentation). I'd be happy to have a more lively discussion about all that on IRC if you (or anyone else) are interested.
I've been looking at recent commits to cdebconf's gtk frontend, and my comments are
* r47575- There are plans for having cdebconf replacing debconf someday, this means the gtk frontend should build against gtk/x11 too [1]. Including more directfb private includes files goes the oppposite way, so this change is wrong.
- Text reformatting you performed to gtk.c makes it longer, althought cleaner, so *less* readable.
* 47577 - Makefile got overcomplicated without any real need for that- "if (NULL != questionbox_scroll) {" is unneded: if there are multiple questions to display, that scrollbox cannot be NULL
- Same observation on text reformatting as above * 47578- #define'ing patch to icons is ok, but someday the may be read as debconf questions, so this change may need to be reverted in the future
* 47580- comments were added to almost any gtk_* function: those functions are already well documented in gtk's API manuals, so there is absolutely no need to add many and many times *the same* comment all over the file.
So, we have gtk.c@47287 (before Lunar's changes) : 1779 lines (newt.c is ~1300) gtk.c@47580 (after Lunar's changes) : 2354 lines difference is +575 lines (~+33% of lines)So, i don't think this means improving code readability at all, especially because the comments you provided, as i said before, are on the usage of gtk functions only, so completly useless.
One thing that *really* should be done is moving the gtkhandler_select_single_tree() code to an appropriate plugin, and modifying the debconf clients accordingly: this will reduce gtk.c of ~130 lines.
This will reduce gtk.c to ~1600 lines, only slightly bigger than newt.c, so i don't think there is any real need to split it in multiple files.
I must evantually say i'm disappointed of the way the gtk frontend code was nonchalantly modified, without any patch being posted and discussed on d-boot previously, moreover proving to ignore many design decisions behind the whole g-i.
Because of all the above reasons, i'd like to revert the gtk frontend to r47287 ASAP.
regards Attilio Fiandrotti