After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 652248 - Please consider merging changes from Inkscape project
Please consider merging changes from Inkscape project
Status: RESOLVED FIXED
Product: gdl
Classification: Other
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Anjuta maintainers
Anjuta maintainers
Depends on: 666984 666995
Blocks:
 
 
Reported: 2011-06-09 23:53 UTC by Alex Valavanis
Modified: 2012-05-17 20:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Changes from gdl-0.7.7 to the local copy in Inkscape (52.36 KB, patch)
2011-06-09 23:53 UTC, Alex Valavanis
reviewed Details | Review
Changes from gdl 871CA to inkscape 10794 (36.14 KB, patch)
2011-12-22 16:37 UTC, Alex Valavanis
none Details | Review

Description Alex Valavanis 2011-06-09 23:53:16 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!
Comment 1 Johannes Schmid 2011-06-10 08:53:25 UTC
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.
Comment 2 Johannes Schmid 2011-06-10 08:55:06 UTC
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.
Comment 3 Alex Valavanis 2011-06-13 08:27:24 UTC
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!
Comment 4 Johannes Schmid 2011-06-13 08:54:31 UTC
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
Comment 5 Gustav Broberg 2011-06-16 20:29:58 UTC
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.
Comment 6 Johannes Schmid 2011-06-20 15:05:34 UTC
> 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.
Comment 7 Ignacio Casal Quinteiro (nacho) 2011-07-22 09:35:38 UTC
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.
Comment 8 Ignacio Casal Quinteiro (nacho) 2011-07-22 09:37:18 UTC
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?
Comment 9 Alex Valavanis 2011-12-22 16:37:23 UTC
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.
Comment 10 Johannes Schmid 2011-12-22 22:19:13 UTC
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!
Comment 11 Alex Valavanis 2011-12-29 15:09:17 UTC
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
Comment 12 Alex Valavanis 2011-12-29 15:10:21 UTC
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!
Comment 13 Ignacio Casal Quinteiro (nacho) 2012-02-02 08:11:47 UTC
any news on this?
Comment 14 Alex Valavanis 2012-02-02 11:23:21 UTC
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?
Comment 15 Sébastien Granjoux 2012-05-17 20:16:35 UTC
I have committed your patch for the last bug #666995. Is there something else to merge or could we close this bug too?
Comment 16 Alex Valavanis 2012-05-17 20:29:56 UTC
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.
Comment 17 Sébastien Granjoux 2012-05-17 20:43:50 UTC
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.