GNOME Bugzilla – Bug 652248
Please consider merging changes from Inkscape project
Last modified: 2012-05-17 20:43:50 UTC
Created attachment 189591 [details] [review] Changes from gdl-0.7.7 to the local copy in Inkscape I'm a new member of the Inkscape developers team, and I'm looking at removing our embedded fork of gdl and offering our changes to the upstream project. It seems that Inkscape forked gdl at version 0.7.7. I have attached a diff showing all of the changes between gdl-0.7.7 and the version that we currently have in Inkscape. A summary of the changes is provided here: http://article.gmane.org/gmane.comp.graphics.inkscape.devel/29285 So... 1) Have any of these changes been implemented upstream? 2) Is there anything useful to upstream gdl here? 3) Are we doing anything the "wrong" way with these changes? I'm not very familiar with the Inkscape code yet, or with gdl, but I hope I can help liaise between the two projects. There is a parallel report open in Launchpad for Inkscape: https://launchpad.net/bugs/792115 Thanks very much in advance for any help you can offer!
Review of attachment 189591 [details] [review]: Detailed review. Most of this seems ok but I guess you will have to apply all those changes manually as a lot of code changed in the Gtk+3 transition. Doesn't seem to be a huge amount of work though. ::: gdl//gdl-dock-bar.c @@ +304,3 @@ + g_object_get (item, "stock-id", &stock_id, "pixbuf-icon", &pixbuf_icon, + "long-name", &name, NULL); Allowing the use of a pixbuf instead of a stock-id looks useful. ::: gdl//gdl-dock-bar.h @@ +23,3 @@ #define __GDL_DOCK_BAR_H__ +#include <gtk/gtk.h> Pretty sure the single include thing has been done upstream everywhere ::: gdl//gdl-dock.c @@ +329,3 @@ GDK_WINDOW_TYPE_HINT_NORMAL); + gtk_window_set_skip_taskbar_hint (GTK_WINDOW (dock->_priv->window), Probably useful. @@ +1079,3 @@ /* Check if dock_object touches center in terms of width */ if (GTK_WIDGET (dock)->allocation.width/2 > object_size.width) { + return GDL_DOCK_TOP; I think that reflects this part of the mail: This fix was more of a hack and I wouldn't propose pushing it upstream (it most certainly breaks some fundamental design ideas of GDL ;). Anyhow, it's committed in revision 16337 in trunk (src/libgdl). I would rather "fix" this in Inkscape by using GDL in a more normal way. In practice this would mean that the dock should always use the available display height but no more (i.e. no scroll bar). This is how it works in Anjuta and also in SWT (used by Eclipse) and I believe Adobe Photoshop/Illustrator/etc. CS>2. ::: gdl//gdl-dock-item.c @@ +82,3 @@ static GtkType gdl_dock_item_child_type (GtkContainer *container); +static void gdl_dock_item_set_focus_child (GtkContainer *container, This focus-child functionality would be useful upstream. ::: gdl//gdl-dock-item-grip.c @@ +110,3 @@ + if (pixbuf) { + grip->_priv->icon_pixbuf = pixbuf; + grip->_priv->icon_pixbuf_valid = TRUE; As I said, pixbuf looks useful. @@ +146,3 @@ + + gdk_draw_rectangle (GDK_DRAWABLE (widget->window), bg_style, TRUE, + 1, 0, widget->allocation.width - 1, widget->allocation.height); This seems part of the focus code. ::: gdl//gdl-dock-item.h @@ +149,3 @@ + const gchar *long_name, + const GdkPixbuf *pixbuf_icon, + GdlDockItemBehavior behavior); Pixbuf... ::: gdl//gdl-dock-master.c @@ +172,3 @@ + GDL_DOCK_EXPANSION_DIRECTION_NONE, + G_PARAM_READWRITE)); + Not entirely sure what this does, but it seems optional, so if Inkspace needs it we can merge it. @@ +651,3 @@ +#ifdef WIN32 + GdkLineStyle lineStyle = GDK_LINE_ON_OFF_DASH; + if (is_os_vista()) This is replaced by much better code upstream. ::: gdl//gdl-dock-notebook.c @@ +390,2 @@ g_object_get (requestor_item, "long-name", &long_name, + "stock-id", &stock_id, "pixbuf-icon", &pixbuf_icon, NULL); Pixbuf again... ::: gdl//gdl-dock-object.c @@ +142,3 @@ + g_param_spec_pointer ("pixbuf-icon", _("Pixbuf Icon"), + _("Pixbuf icon for the dock object"), + G_PARAM_READWRITE)); And more pixbuf... ::: gdl//gdl-dock-object.h @@ +73,3 @@ + GDL_DOCK_EXPANSION_DIRECTION_LEFT, + GDL_DOCK_EXPANSION_DIRECTION_RIGHT +} GdlDockExpansionDirection; As I said - some documentation for this would be great. ::: gdl//gdl-dock-paned.c @@ +145,3 @@ + +static void +gdl_dock_paned_resize_paned_ancestors (GdlDockPaned *paned, More information (or test cases) on this needed but seems to be a sane idea. ::: gdl//gdl-dock-placeholder.c @@ +190,3 @@ g_object_class, PROP_FLOAT_X, g_param_spec_int ("floatx", _("X-Coordinate"), + _("X coordinate for dock when floating"), Pretty cosmetic change ;) ::: gdl//libgdlmarshal.list @@ +1,2 @@ VOID:VOID +VOID:ENUM Guess this is need for this docking placement.
Hi! > 1) Have any of these changes been implemented upstream? IIRC a couple of bug fixed have been upstreamed by us in the past, not sure how many. > 2) Is there anything useful to upstream gdl here? Seems like the CANT_DOCK thing and some of the placement code, see my detailed review. > 3) Are we doing anything the "wrong" way with these changes? Overall, I don't think so. > Thanks very much in advance for any help you can offer! I think moving to upstream gdl will help you a lot with porting stuff to gtk+-3 as a lot of the drawing code changed and everything was fixed in gdl-3. There are also a couple of places were drawing is much better now.
Hi Johannes, Thanks for your very helpful review. I have created a branch of Inkscape that builds against external gdl 2.30.1. I have discussed this with the Inkscape developers mailing list, and the main points are: * There seems to be quite strong support for switching to external gdl sooner rather than later. * We can live without icons in docked dialogs for now. In fact, Inkscape registers itself as a provider of stock icons, so we may just move to gdl_dock_item_new_with_stock(...). A discussion is underway. * The loss of child focus-switching/key bindings definitely feels like a problem when we use external gdl. Lots of our dialogs contain multiple input fields, so it is quite painful if we can't tab-select them. We'd find it very useful if you could implement this! I'm not sure if it is a show-stopper yet. So, I guess the question now is... what next? Are you able to add the focus switching functions from our patch? If so, which gdl release version/date do you plan to target? Thanks again for all your help!
Hi! > * We can live without icons in docked dialogs for now. In fact, Inkscape > registers itself as a provider of stock icons, so we may just move to > gdl_dock_item_new_with_stock(...). A discussion is underway. Actually it would be cool to merge that functionality as the usage of pixmaps is sometimes more convenient than stock icons. > * The loss of child focus-switching/key bindings definitely feels like a > problem when we use external gdl. Lots of our dialogs contain multiple input > fields, so it is quite painful if we can't tab-select them. We'd find it very > useful if you could implement this! I'm not sure if it is a show-stopper yet. Well, it is actually implemented in your patch so I would just ask you to merge these changes manually into git master and provide a patch here. I am sorry, but I don't have the time to do this. As you see from my review, this shouldn't really be difficult at all. In the long run, I guess you might be interested in the gdlmm[1] project. I don't know exactly what the state is but it shouldn't be difficult to bring this up-to-date and I would give you gtkmm compatible C++ bindings for gdl. > So, I guess the question now is... what next? Are you able to add the focus > switching functions from our patch? If so, which gdl release version/date do > you plan to target? The point is that the current gdl version is 3.0 and development is going into the unstable branch[2] (3.1.x) that will be released as gdl 3.2 with GNOME 3.0 which is in September. All these version depend on gtk+ 3.0. It might be able to create another gdl 2.30.x version if you *also* provide a patch against the gnome-2-30[3] branch in case that inkscape doesn't want to depend on gtk+ 3.0 for now. Anyway, gtk+-3.0 fixes some drawing issues with gdl, too. [1] http://git.gnome.org/browse/gdlmm [2] http://git.gnome.org/browse/gdl/tree/ [3] http://git.gnome.org/browse/gdl/log/?h=gnome-2-30
It's good to see that you are working on pushing Inkscape's GDL changes upstream! (I blame myself for this not happening earlier.) I'm the one who did the original fork of GDL for Inkscape. Even though I have been away from Inkscape for a good while, I probably can answer a few of the questions raised. (In reply to comment #1) > Review of attachment 189591 [details] [review]: > > Detailed review. [...] > @@ +1079,3 @@ > /* Check if dock_object touches center in terms of width */ > if (GTK_WIDGET (dock)->allocation.width/2 > object_size.width) { > + return GDL_DOCK_TOP; > > I think that reflects this part of the mail: > This fix was more of a hack and I wouldn't propose pushing it Actually, looking at the VC logs, this was part of a patch that: "Prevented automatic center docking from happening". Looking at this now, I'm not sure why it wasn't made configurable... [...] > ::: gdl//gdl-dock-master.c > @@ +172,3 @@ > + GDL_DOCK_EXPANSION_DIRECTION_NONE, > + G_PARAM_READWRITE)); > + > > Not entirely sure what this does, but it seems optional, so if Inkspace needs > it we can merge it. [...] > ::: gdl//gdl-dock-object.h > @@ +73,3 @@ > + GDL_DOCK_EXPANSION_DIRECTION_LEFT, > + GDL_DOCK_EXPANSION_DIRECTION_RIGHT > +} GdlDockExpansionDirection; > > As I said - some documentation for this would be great. > The two changes above are part of the change described like this in my original mail: * I added an option to let dock items expand their container dock objects. This was needed in order to allow the user to always expand a dialog in the dock by pulling it's bottom handle downwards. In turn, the reason for wanting this was based on the chosen design where the dock can hold an unlimited amount of content (i.e. it should always grow on the height). This fix was more of a hack and I wouldn't propose pushing it upstream (it most certainly breaks some fundamental design ideas of GDL ;). Anyhow, it's committed in revision 16337 in trunk (src/libgdl). So my view is (still) that this should be dealt with by changing Inkscape rather than GDL. I'm not proud of the current solution in Inkscape and it never worked perfectly anyway.
> So my view is (still) that this should be dealt with by changing > Inkscape rather than GDL. I'm not proud of the current solution > in Inkscape and it never worked perfectly anyway. OK, so we probably shouldn't merge it and you can change it in inkscape if it wasn't working anyway.
Some note about the custom drawing of the handler. If we want to do it correctly this should be done in this way: create a GDL_HANDLER_CLASS, and draw the background with this style context. So inkscape would just have to edit its css so draw it with the background it wants.
Can we also try to split the patch in small changes that can be already applied to master and if needed gnome-2-30 branches?
Created attachment 204099 [details] [review] Changes from gdl 871CA to inkscape 10794 I've merged a load of the upstream GDL changes into the Inkscape trunk and I've also stripped out a lot of the unimportant changes we made in our fork (minor compiler warnings etc). I'll see if I can split the minimal patch into separate chunks too.
Thanks! Doing this work is really great! If you can split the patches a bit and preferably create them using git format-patch with a suitable commit message it would be easier to review and to integrate into gdl trunk. It would be super cool if everybody could use the upstream version of gdl!
OK, all our changes are now split into separate chunks, and I have submitted them as individual patches against master (using git format-patch). Summary of our changes... * Placement/expansion changes: Not forwarded, dropped from Inkscape (see discussion above) * Skip taskbar hint: bug #666995 * CANT_DOCK implementation: bug #666987 * Focus handling/key binding: bug #666984 * Pixbuf icons: bug #666969
Please let us know if there is anything else we can do to help with this. It would be great to have these in the next upstream version of gdl so we can finally kill our fork!
any news on this?
Hi Nacho, I've been pretty busy of late. I can have a go at fixing the property handling stuff in bug #666995 soon, I hope. Is there anything else needed for bug #666984 or can the merge go ahead?
I have committed your patch for the last bug #666995. Is there something else to merge or could we close this bug too?
That's great. No, I think this bug can be closed. There is a follow-up report in bug #671701, requesting that docked dialogs indicate when they have focus.
Ok Thanks. I'm closing this bugs then. I have seen the bug #671701, but I haven't an example where I can change the focus between different dock widgets.