GNOME Bugzilla – Bug 666995
Please review taskbar hint patch (inkscape)
Last modified: 2012-05-17 20:22:36 UTC
Created attachment 204321 [details] [review] Skip taskbar hint (Inkscape) Here's the final patch from Inkscape. This one skips the taskbar hint for gdl dock.
Review of attachment 204321 [details] [review]: That would mean that all not-docked gdl windows are not shown in the taskbar, right? Not sure if this is always a good thing depending on how you use gdl. Or do you think that is always correct? The patch is correct, just wondering if that should be configurable.
I guess it suits Inkscape better to skip displaying in the taskbar, but yes it would be better to be configurable... what's the best way to implement this?
The best way is probably a property on gdl-dock like "skip-taskbar" which is set to TRUE by default. Including a set_ and get_ public method would also be good together with docs for the property and the methods. People who like to show the windows in the taskbar can set this property to FALSE. (Or you name it the other way round and they set it to TRUE, I don't really care).
why not making this kind of windows modal? no taskbar and they are visible to the user until they are closed or docked.
I think the "skip-taskbar" property might be the best option from an Inkscape-developer perspective. If I understand correctly, modal windows prevent user interaction with any other part of the application until they are closed. This wouldn't work for us in Inkscape, because we use floating gdl-docks to hold things like palettes and style selectors that are used in conjunction with graphical elements in the main application window. It might be worth adding as another configurable property at some point though.
(In reply to comment #4) > why not making this kind of windows modal? no taskbar and they are visible to > the user until they are closed or docked. There was a bug that the windows shouldn't be "always on top" and thus we disabled it. It is probably another useful option but it is different from "modal" and from "skip-taskbar" in various ways.
Created attachment 204423 [details] [review] Add configurable skip-taskbar property I've added the skip-taskbar property with GTK-Doc comment. I set it as CONSTRUCT_ONLY for now... does it make sense to allow this to be changed after the window is constructed? We could easily add get/set methods later if they are likely to be useful.
Review of attachment 204423 [details] [review]: CONSTRUCT_ONLY is not correct here. It would mean that you would have to pass the value at construction time of the GdlDock object (which you cannot as you would have to do that in the g_object_new(..) call in gdl_dock_new()). This has nothing to do with the individual window which is possibly created by the dock later on.
Created attachment 209060 [details] [review] Add configurable skip-taskbar property Here's a new version of the patch that uses G_PARAM_CONSTRUCT instead of G_PARAM_CONSTRUCT_ONLY. Please let me know if any further work is needed. Apologies for my ineptitude here: I haven't done anything with Glib properties before!
Review of attachment 209060 [details] [review]: See comments. ::: gdl/gdl-dock.c @@ +129,3 @@ gint height; + gboolean skip_taskbar; for libs it is better to have it at the end of the struct and like: gboolean skip_taskbar : 1; @@ +236,3 @@ + g_object_class_install_property ( + g_object_class, PROP_SKIP_TASKBAR, + * taskbar. _("Skip taskbar") in a newline @@ +238,3 @@ + g_param_spec_boolean ("skip-taskbar", _("Skip taskbar"), + _("Whether or not to prevent a floating dock window " + * indentation is wrong here @@ +356,3 @@ GDK_WINDOW_TYPE_HINT_NORMAL); + + if (dock->priv->skip_taskbar) { is this coding style correct? shouldn't be the brackend in a newline?
Created attachment 209062 [details] [review] Add configurable skip-taskbar property I've addressed all the formatting issues in the latest patch, except for the last one - it seems that in gdl-dock.c, all for and if statements have their opening bracket on the same line. If you'd prefer me to do it on a new line, I can certainly do that.
Review of attachment 209062 [details] [review]: I wonder if the property should param construct only.
I did that in the original patch, but changed it in response to comment 8.
dunno G_PARAM_CONSTRUCT means that something can be modified at construction time or later on. In our case it only works at construction time as we set the property in the constructor. Either we have to change that and change the property also from a specific method or make it contruct only...
Review of attachment 209062 [details] [review]: The property shouldn't be CONSTRUCT_ONLY (as that doesn't make much sense), it can be _CONSTRUCT but I don't think it makes much difference in that case. But the set() property methods needs to call set_skip_taskbar_hint() because otherwise the property won't have any effect. And if you do it in the set() method there is no need do it in the constructor(). Bonus points for a gdl_dock_set_skip_taskbar(gboolean skip) which is called by set() and does all the work. Thanks! ::: gdl/gdl-dock.c @@ +420,3 @@ break; + case PROP_SKIP_TASKBAR: + dock->priv->skip_taskbar = g_value_get_boolean (value); You need to set gtk_window_set_skip_tasbar_hint() in the property_set() method and not (only) in the constructor.
Created attachment 209144 [details] [review] Add configurable skip-taskbar property Hi Johannes, I have tried to implement the changes you suggested in this updated patch. Please let me know if this is OK. If not, can you provide any examples of places where similar properties are implemented correctly in GDL or GTK+?
Review of attachment 209144 [details] [review]: inline comment. ::: gdl/gdl-dock.c @@ +420,3 @@ + dock->priv->skip_taskbar = g_value_get_boolean (value); + + if (dock->priv->window && dock->priv->window) { this should be factored out into a gdl_dock_set_skip_taskbar
Created attachment 209147 [details] [review] Add configurable skip-taskbar property I've split the functionality into a separate function as suggested.
Review of attachment 209147 [details] [review]: Some more inline comments. ::: gdl/gdl-dock.c @@ +487,2 @@ static void +gdl_dock_set_skip_taskbar (GdlDock *dock, gboolean skip) I think this should be made public. But let's wait for jhs comment about it. @@ +488,3 @@ +gdl_dock_set_skip_taskbar (GdlDock *dock, gboolean skip) +{ + dock->priv->skip_taskbar = skip; this must be: dock->priv->skip_taskbar = (skip != FALSE); @@ +1297,3 @@ "floatx", x, "floaty", y, + "skip-taskbar", TRUE, is this correct?
Review of attachment 209147 [details] [review]: It's nearly fine - thanks a lot! ::: gdl/gdl-dock.c @@ +232,3 @@ + * + * Whether or not to prevent a floating dock window from appearing in the + * taskbar. We should add here: "Note: This does only affect floating windows that are created after this flag is yet, existing windows aren't affected. Usually this property is used when you create the dock. @@ +487,2 @@ static void +gdl_dock_set_skip_taskbar (GdlDock *dock, gboolean skip) Yep, it was supposed to be public. @@ +488,3 @@ +gdl_dock_set_skip_taskbar (GdlDock *dock, gboolean skip) +{ + dock->priv->skip_taskbar = skip; ? I think = skip is perfectly ok, or do you think that will break the 1 bit wide variable? @@ +1297,3 @@ "floatx", x, "floaty", y, + "skip-taskbar", TRUE, Should be dock->priv->skip_taskbar instead of TRUE I guess.
we must always validate the boolean values that come from public api. that's why we need (skip != FALSE)
nacho is right...
Created attachment 209239 [details] [review] Add configurable skip-taskbar property I've implemented most of the changes, but I couldn't get the "skip-taskbar" flag to set correctly in gdl_dock_add_floating_item(). It just seems to give the following error when I include it in the g_object_new call, except when I set it to a literal TRUE/FALSE: (lt-test-dock:13269): GLib-GObject-WARNING **: value "TRUE" of type `gboolean' is invalid or out of range for property `skip-taskbar' of type `gboolean' The error stops occurring if I get rid of the single-bit flag so I have changed: gboolean skip_taskbar : 1; to: gboolean skip_taskbar; in the latest patch, and everything seems fine... however, I guess there is a better way to fix this?!
Created attachment 209240 [details] [review] Add configurable skip-taskbar property Oops - I forgot to update the patch!
Hi guys, Any thoughts on the latest patch version? AV
Review of attachment 209240 [details] [review]: Some minor comments. Apart from that it looks good to me. ::: gdl/gdl-dock.c @@ +234,3 @@ + * this property is used when you create the dock. + * + * Whether or not to prevent a floating dock window from appearing in the should be 3.6 @@ +493,3 @@ + * appearing in the system taskbar. + * + * @skip: %TRUE if floating docks should be prevented from appearing in the taskbar 3.6 @@ +501,3 @@ + + if (dock->priv->window && dock->priv->window) { + * you should use here dock->priv->skip_taskbar as it is the normalized one
Thanks for your patch Alex and for the review of Johannes and Ignacio. I have just committed your patch, including the latest changes requested by Ignacio.
That's great. Many thanks