GNOME Bugzilla – Bug 394182
Massive code cleanup
Last modified: 2007-08-13 04:48:50 UTC
I took some time over the holidays to clean up GtkHtml's code a bit. The patch turned out to be ~5000 lines, so my apologies in advance to whomever winds up reviewing this. Here's what I changed [1]: - Fix (almost) all compiler warnings. Obsoletes bug #363036. Excludes warnings caused by bug #363033. - Stop using deprecated GTK+ widgets and symbols. NOTE: There's still a few spots where deprecated GTK+ symbols are being used, so we can't compile with -DGTK_DISABLE_DEPRECATED just yet. In particular, construction of the editor's toolbar involves calls to things like gtk_toolbar_append_widget(). Also, htmlselect.c uses the deprecated GtkCombo widget, but I don't yet understand that code well enough mess with it. Use GtkComboBox instead of GtkOptionMenu [2]. Use g_object_force_floating() instead of GTK_OBJECT_SET_FLAGS(). Use g_object_is_floating() instead of GTK_OBJECT_FLOATING(). Use g_object_ref_sink() instead of gtk_object_sink(). Use g_idle_add() instead of gtk_idle_add(). Use g_timeout_add() instead of gtk_timeout_add(). Use g_source_remove() instead of gtk_idle_remove(). Use g_source_remove() instead of gtk_timeout_remove(). Use gtk_widget_set_size_request() instead of gtk_widget_set_usize(). Use gtk_window_set_resizable() instead of gtk_window_set_policy(). - Stop using _some_ deprecated libgnomeui widgets and symbols. Use GtkColorButton instead of GnomeColorPicker. Use gtk_window_set_icon_name() instead of gnome_window_icon_set_from_file(). - Stop including <gnome.h> <gnome.h> includes <libgnomeui/libgnomeui.h> which in turn uses the deprecated GtkCombo widget. In many cases, <gtk/gtk.h> is all that's really needed. When libgnomeui symbols are needed, include only the necessary header files (e.g. <libgnomeui/gnome-app-helper.h>). - Prefer stock or named icons. GtkHtml still distributes many icon files that are now provided by gnome-icon-theme. The following is a list of GtkHtml icon files and their equivalent stock or named icons which are now used in the code. I removed these icon files from art/Makefile.am so they will no longer be distributed, and I propose removing them from Subversion as well. GtkHtml Icon File Replaced With ------------------------------ ------------------------------ art/16_copy.png GTK_STOCK_COPY art/24_copy.png GTK_STOCK_COPY art/16_cut.png GTK_STOCK_CUT art/24_cut.png GTK_STOCK_CUT art/16_paste.png GTK_STOCK_PASTE art/24_paste.png GTK_STOCK_PASTE art/font-tt-16.png "stock_text-monospaced" art/font-tt-24.png "stock_text-monospaced" art/insert-image-16.png "stock_insert_graphic" art/insert-image-24.png "stock_insert_graphic" art/insert-link-16.png "stock_insert-url" art/insert-link-24.png "stock_insert-url" art/insert-object-16.png "stock_insert_graphic" art/insert-object-24.png "stock_insert_graphic" art/insert-rule-16.png "stock_insert-rule" art/insert-rule-24.png "stock_insert-rule" art/insert-table-16.png "stock_insert-table" art/insert-table-24.png "stock_insert-table" art/properties-16.png GTK_STOCK_PROPERTIES art/redo-16.png GTK_STOCK_REDO art/redo-24.png GTK_STOCK_REDO art/search-16.png GTK_STOCK_FIND art/search-24.png GTK_STOCK_FIND art/search-and-replace-16.png GTK_STOCK_FIND_AND_REPLACE art/search-and-replace-24.png GTK_STOCK_FIND_AND_REPLACE art/smiley-1.png "stock_smiley-6" art/smiley-2.png "stock_smiley-5" art/smiley-3.png "stock_smiley-1" art/smiley-4.png "stock_smiley-3" art/smiley-5.png "stock_smiley-2" art/smiley-6.png "stock_smiley-4" art/smiley-8.png "stock_smiley-8" art/smiley-9.png "stock_smiley-9" art/smiley-10.png "stock_smiley-10" art/smiley-11.png "stock_smiley-11" art/smiley-12.png "stock_smiley-26" art/table-column-16.png "stock_select-column" art/table-row-16.png "stock_select-row" art/table-table-16.png "stock_select-table" art/undo-16.png GTK_STOCK_UNDO art/undo-24.png GTK_STOCK_UNDO [1] The capplet code appears to be dead. It's not distributed in GtkHtml releases, and the latest capplet/ChangeLog entry is dated 2004-07-22. Therefore I didn't bother with it. This bug only applies to the libgtkhtml and html-editor source code. [2] The paragraph style combo box was particularly fun to migrate. Since some of its items are desensitized when in plaintext mode, a simple text combo box won't do. Instead, we have to set up a two-column list store to populate the combo box widgets and manage the sensitivity of their items. I wound up splitting the logic for this widget into new source files: components/html-editor/paragraph-style.[ch]
Created attachment 79730 [details] [review] Proposed patch
Created attachment 79813 [details] [review] Updated patch for gtkhtml-3.13.5
I'll review this asap.
I think we should take care of the patches that add features or fix bugs before applying a huge patch that potentially breaks all other patches...
I agree. I'll try to keep the patch up-to-date until the time is right to consider applying it. I'm already shipping it in Fedora "Rawhide" to give it some field testing.
Matthew, It took me really long time to just go through it once. Sorry about that. Felt that quite a lot of code are pretty straight forward. I dont think I can be 100% sure of the patch. I think we should push this to HEAD to get it more tested. It is really difficult to review such a HUGE patch to 100% confidence :( Hope you can understand the point. Please push this to next HEAD build.
Sure, I understand completely. FWIW, this has been in Fedora "Rawhide" for a few weeks now and I've not heard any complaints. I'll try to partition future efforts like this into less overwhelming chunks. One question before I commit this. Should I also remove the old GtkHtml icon files listed in comment #0 from Subversion? Those icons are now provided by gnome-icon-theme, and I don't see much point in keeping them around.
Committed to HEAD. I removed the obsolete icon files. If this is a problem I can add them back easy enough, but I see no need for them to remain under source control.
Kjartan Maraas noticed a small error in my patch: Index: toolbar.c =================================================================== --- toolbar.c (revision 8364) +++ toolbar.c (working copy) @@ -61,7 +61,7 @@ active_font_size_changed (GtkComboBox *c static void font_size_changed (GtkHTML *html, - GtkHTMLParagraphStyle style, + GtkHTMLFontStyle style, GtkHTMLControlData *cd) { if (style == GTK_HTML_FONT_STYLE_DEFAULT) I'm going to go ahead and commit the change as part of this bug.
During my investigations on bug #426496 I was pointed by 'rodo' to see at this bug too, because this patch causes that bug. And I think there are few places where the replace from sink to ref_sink isn't right, it could leak. See patch.
Created attachment 93271 [details] [review] proposed gtkhtml patch it's the addon for the previous patch, I hope I did it right.
Matthew, can you review/approve this?
Looks okay to me. Thanks for catching those errors, Milan.
Committed revision 8520.