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 700914 - Add GtkHeaderBar support
Add GtkHeaderBar support
Status: RESOLVED FIXED
Product: glade
Classification: Applications
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Glade 3 Maintainers
Glade 3 Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-05-23 18:40 UTC by Bastien Nocera
Modified: 2014-11-07 10:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add GtkHeaderBar support (3.56 KB, patch)
2013-08-06 12:33 UTC, John Stowers
none Details | Review
Initial GtkHeaderBar support (8.86 KB, patch)
2014-04-26 05:23 UTC, Matthias Clasen
none Details | Review
Support CSD windows (5.44 KB, patch)
2014-04-26 06:16 UTC, Matthias Clasen
none Details | Review

Description Bastien Nocera 2013-05-23 18:40:17 UTC
So I can create composite widgets from it.
Comment 1 John Stowers 2013-08-06 12:33:47 UTC
Created attachment 250961 [details] [review]
Add GtkHeaderBar support
Comment 2 John Stowers 2013-08-06 12:36:27 UTC
Works for simple use cases

Missing

* an icon
* multiple start/end children support
* custom-title widget support

Will fix coding style when I get back from GUADEC
Comment 3 Richard Hughes 2013-08-15 11:17:44 UTC
Hmm, this indeed shows the GtkHeaderBar for me in glade, but it also doesn't show the GtkWindow contents, i.e. the main bit in the UI. It *just* shows the header and nothing else.
Comment 4 Richard Hughes 2013-08-15 11:23:28 UTC
(glade:16629): Gtk-WARNING **: Attempting to add a widget with type GtkBox to a GtkWindow, but as a GtkBin subclass a GtkWindow can only contain one widget at a time; it already contains a widget of type GtkHeaderBar
Comment 5 Reece H. Dunn 2014-03-11 01:24:13 UTC
That warning is because glade does not recognize <child type="titlebar">...</child>. If you place the GtkHeaderBar within that, you can add the main window UI in the <child>...</child> element and it will work in the application and glade-previewer. Glade will remove the |type="titlebar"| from a child node.
Comment 6 Matthias Clasen 2014-04-26 05:23:37 UTC
Created attachment 275189 [details] [review]
Initial GtkHeaderBar support

Add support for GtkHeaderBar.
Based on a patch by John Stowers.
Comment 7 Matthias Clasen 2014-04-26 05:25:19 UTC
Slight elaboration of Johns patch. It adds icons, and allows to add/remove slots.

Still missing:

- support for replacing title+subtitle with a custom widget

- start/end size pseudo properties

- support for the 'titlebar' slot on windows
Comment 8 Matthias Clasen 2014-04-26 06:16:17 UTC
Created attachment 275191 [details] [review]
Support CSD windows

This patch adds preliminary support for the titlebar slot
in GtkWindow. The rendering of windows with csd titlebar
is somewhat broken in glade, and the virtual use-csd property
does not get set up properly when loading a ui file.
Comment 9 Bastien Nocera 2014-06-12 10:26:22 UTC
I tested Matthias' 2 patches on top of glade master.

Opening gnome-control-center/panels/sharing/sharing.ui says "Property has versioning problems: Use Header Bar". Toggling the "CSD" switch twice will hide all the contents of the window. Seems that the patch needs more work for the "use_header_bar" GtkDialogs.
Comment 10 Lionel Landwerlin 2014-08-09 23:27:10 UTC
The window content disappearing can be "fixed" by putting a
 gtk_widget_map (GTK_WIDGET (object));

after the 

gtk_window_set_titlebar (GTK_WINDOW (object), NULL);

Though GTK+ will still complain you set a titlebar on a mapped window.
Comment 11 Matthias Clasen 2014-10-13 11:48:17 UTC
I've pushed a headerbar branch, which works well enough for me. Supports custom titles, and multiple children at either end.
Comment 12 Tristan Van Berkom 2014-10-14 09:47:46 UTC
(In reply to comment #11)
> I've pushed a headerbar branch, which works well enough for me. Supports custom
> titles, and multiple children at either end.

So I tried the branch and have some preliminary comments before really digging into the patches themselves.


First problem I found, The CSD property can only be set to TRUE once, after 
which point setting the property has no effect and there are errors in the 
console about calling gtk_window_set_titlebar() on a realized window.

I'm not sure why this works the first time, actually, it's possible that we 
need that virtual property to behave like construct-only properties but as I'm 
not sure how it could work on an already displayed window the first time, I'm 
really unsure.

Setting the CSD property to TRUE and doing Undo/Redo has the same effect (top 
placeholder is lost and can never be restored).


These patches (probably the first one) break GtkWindow, it no longer offers a 
placeholder for the actual window content.


The header bar properties have some properties which seem irrelevant
for a project using Glade, i.e. the 'Has Subtitle' and 'Decoration Layout Set'

We should consider either:

  a.) Following the pattern in Glade's custom editors where there is a checkbox
      for the 'Has Subtitle' and when that is disabled, the text entry for the
      subtitle text on the right is disabled.

      This is only a good idea if it's really wanted to have a headerbar with no
      subtitle but still 'reserving space for the subtitle'.

  b.) Probably more appropriately, we just hide these properties altogether from
      the UI and sync them up in the plugin in various places:

        - At load time, set has-subtitle according to whether subtitle text is
          actually provided

        - At save time, set do the same

        - For the runtime appearance, adjust the runtime widget's has-subtitle
          whenever the subtitle text changes


Selecting a headerbar child and modifying it's GtkPackType does not effect
the placement of a child widget at runtime, however if you save it and load it
then the child appears on the other side.

The "Add slot at start" / "Add slot at end" actions are only set as child 
actions, but should probably also be added as widget actions of the headerbar 
type proper (i.e. right clicking on a child on the left or right shows the 
action, right clicking on the headerbar itself does not let you add/remove 
slots).

Furthermore, the add/remove slot actions effect project data without going 
through the glade command API (so adding/removing slots is not undoable and can 
cause errors in large undo/redo chains).

Further, the add/remove slot actions do not result in changing of the virtual 
properties "Number of items at the start" and "Number of items at the end"

Quick pointer: Placeholders are not project data but generated by the adaptors
based on thier own design, most bin widgets always ensure a placeholder when
no project widget is there, things like GtkBox/GtkGrid ensure that placeholders
exist by dynamically generating them whenever relevant properties change and
when widgets are added/removed.

So basically program flows as:
  o User interacts with Glade
  o Glade issues a command
  o The command effects a Glade property or add/remove widget
  o The adaptor ensures that placeholders exist

Some other details, the CSD property should declare a <_tooltip> as it's meant 
to be visible in the UI and there is no other tooltip source (virtual 
properties that show up in the UI should all declare an expanitory tooltip).

Glossing over the patch I've found glade_command_push_group() called without a
real command description, instead only a widget name is used. Command 
descriptions should be brief but explanitory, i.e. "Creating $foo" or
"Setting dimentions of $foo", etc.
Comment 13 Tristan Van Berkom 2014-10-14 10:10:42 UTC
A few more notes on this:

Setting the CSD property to TRUE / FALSE does not delete the children
which might already exist, or restore them at undo time

There are two ways to do this proplerly:

  o The preferred way to do this is with a custom editor, like most widgets
    already have.

    In a custom editor, one could add a GtkCheckButton directly for the CSD
    property and issue GladeCommands when that button state changes.

  o There is a signal 'pre-commit' and 'post-commit' on the GladeEditorProperty
    object which actually displays the property in the editor.

    These signals can be caught to trap the action into a command group, 
    allowing one to issue GladeCommands before and after, one could use this 
    approach to delete the titlebar content undoably

For the record, in my last comments I mentioned the awkwardness of the
headerbar properties 'has-subtitle' and 'decoration-layout-set', I do not
consider these issues to particularly block the initial landing of this branch.

It will probably be a high priority before rolling the next stable Glade though,
because if we restrict users to not expose the 'has-subtitle' property, it's
probably best to do it from day one, and not remove it in a later release
(users can set that property manually if that's needed for dynamically set
subtitles, but it will break their apps if allow them to set it in Glade and
then suddenly remove it later on).

Thanks for your serious attention to this Matthias, headerbar presents a lot
of work, I'll do my best to keep up with reviews in the limited time I have.
Comment 14 Matthias Clasen 2014-10-14 13:43:50 UTC
toggling the setting works fine here. yes, you get a warning about the realized window, but we do the right thing there.
Comment 15 Tristan Van Berkom 2014-10-14 14:26:33 UTC
(In reply to comment #14)
> toggling the setting works fine here. yes, you get a warning about the realized
> window, but we do the right thing there.

This is a capture taken right now:
    https://people.gnome.org/~tvb/test.ogv

It's running against a GTK+3 3.13 from the looks of my latest GTK+ configure.ac, with an x11 backend.
Comment 16 Tristan Van Berkom 2014-10-14 14:31:20 UTC
Rebuilt GTK+3 master just to re-confirm, same problem persists.
Comment 17 Matthias Clasen 2014-10-14 17:39:18 UTC
I'll look again. Toggling csd on and off like that is not really something that ever happens in real life, so it is poorly tested. Maybe an alternative to the switch would be a separate 'csd window' object - can glade do 'fake types' like that ?
Comment 18 Matthias Clasen 2014-10-17 18:37:58 UTC
https://git.gnome.org/browse/gtk+/commit/?id=8821d488c52ca0d571fc76c8160f181c6aa43f41

fixes the content visibility problem when repeatedly toggling the csd switch

I've also moved the use-csd property into the GladeWindowEditor. I think the one blocker thats left before I consider this good enough is a way to remove an existing headerbar widget when csd is toggled back off. It disappears from the preview, but it still hangs around in the widget tree. I just don't know enough glade internals to figure out how to make it go away.
Comment 19 Tristan Van Berkom 2014-10-18 06:31:07 UTC
(In reply to comment #18)
> https://git.gnome.org/browse/gtk+/commit/?id=8821d488c52ca0d571fc76c8160f181c6aa43f41
> 
> fixes the content visibility problem when repeatedly toggling the csd switch
> 
> I've also moved the use-csd property into the GladeWindowEditor. I think the
> one blocker thats left before I consider this good enough is a way to remove an
> existing headerbar widget when csd is toggled back off. It disappears from the
> preview, but it still hangs around in the widget tree. I just don't know enough
> glade internals to figure out how to make it go away.

Hi Matthias,

I've pushed a commit onto your branch (and reverted the commit which issues 
glade commands from the GladeWidgetAdaptor->set_property() method) here:

https://git.gnome.org/browse/glade/commit/?h=headerbar&id=32fc32041c333790b7b87ba0e88831865836d820

It can be confusing since there are various levels at which one can participate 
in how Glade handles widgets, hopefully the above commit clarifies this a bit
(Note specifically that the GladeWindowEditor includes a check button, and
when the check button changes state, the editor pushes a command group which
changes the CSD setting and possibly deletes a project widget), this makes
the whole thing properly undoable and in one command group.

Basically:

  o The GladeWidgetAdaptor->set_property()/get_property() methods are called
    whenever a widget should be updated in Glade's workspace.

    Objects in the workspace are updated whenever GladeWidget properties change,
    which might be due to some user input, and might also be due to an
    Undo/Redo command.

    So it's important to not issue new commands at this point ;-)

  o Editors issues commands, those are both widget-level editors and
    property level editors.

    When the user interacts with an editor, it should always be GladeCommand
    API that is used to effect the data model.

    GladeWidgetAdaptor offers methods for the creation of editor classes
    (both property editors and widget editors).

Anyway, the fix you pushed to GTK+ indeed fixes the re-appearance of the
titlebar, and the commit I pushed to your branch makes the CSD property
properly undoable and consequentially deleted project widgets reappear
at undo time.

This took me a chunk of my day so I don't really have time for another full
review, please take a look at the headerbar branch and let me know when you
think the branch is ready for another review.
Comment 20 Tristan Van Berkom 2014-10-18 06:50:09 UTC
Actually while I'm here though... I think that at least the client side 
decorations feature is at least just about ready to land.

If you can split out the 'use-csd' property related patches and merge only
those to master, then we can concentrate on the headerbar widget in this bug.

There are still 2 concerns I still have regarding the 'use-csd' patches:

  o I think that the use-csd setting has to be exclusive to some other
    properties, however I'm not sure which ones they are really.

    For instance, the 'window title' property is still meaningful with
    csd enabled ? I would suspect it is still useful even if it doesnt
    show up in the titlebar (i.e. it's still a word that will show up
    in some window managers task lists correct ?)

    Are the 'decorated' and 'hide titlebar during maximization' properties
    meaningful if the application provides a custom titlebar ?

  o The static function you added searches the window hierachy for a child
    widget with the style class 'headerbar', this feels error prone, like
    it may break if a user ever adds a widget somewhere with the 'titlebar'
    class, or that it may break with an upgrade to a future GTK+ version.

    Can we please have something more stable to retrieve the titlebar ?

    Perhaps an API in GTK+ proper for this ?

In any case, I still think we can safely land the CSD portions of this
branch and address the above issues separately.
Comment 21 Matthias Clasen 2014-10-18 15:37:18 UTC
(In reply to comment #19)

> This took me a chunk of my day so I don't really have time for another full
> review, please take a look at the headerbar branch and let me know when you
> think the branch is ready for another review.

Thanks, Tristan. I'll check it out.
Comment 22 Matthias Clasen 2014-10-18 15:40:53 UTC
(In reply to comment #20)

> There are still 2 concerns I still have regarding the 'use-csd' patches:
> 
>   o I think that the use-csd setting has to be exclusive to some other
>     properties, however I'm not sure which ones they are really.
> 
>     For instance, the 'window title' property is still meaningful with
>     csd enabled ? I would suspect it is still useful even if it doesnt
>     show up in the titlebar (i.e. it's still a word that will show up
>     in some window managers task lists correct ?)
> 
>     Are the 'decorated' and 'hide titlebar during maximization' properties
>     meaningful if the application provides a custom titlebar ?

You are right, hide-titlebar-when-maximized and decorated ignored for csd windows, we should probably reflect that in the UI. The title is passed on to the headerbar (if you use a GtkHeaderBar for the titlebar widget). But GtkHeaderBar exposes
the title itself, so we could just as well make make the title property go away for csd windows.

>   o The static function you added searches the window hierachy for a child
>     widget with the style class 'headerbar', this feels error prone, like
>     it may break if a user ever adds a widget somewhere with the 'titlebar'
>     class, or that it may break with an upgrade to a future GTK+ version.
> 
>     Can we please have something more stable to retrieve the titlebar ?
>
>     Perhaps an API in GTK+ proper for this ?

I've been thinking the same.
Comment 23 Matthias Clasen 2014-10-18 15:52:58 UTC
The title propagation doesn't quite work in glade though - glade doesn't pick up on the fact that the headerbar title changes 'spontaneously'.
Comment 24 Matthias Clasen 2014-10-18 21:13:00 UTC
I've pushed two new branches:

csd - containing the cleaned up csd support, including your editor fixes. The only changes on top of what was in the original headerbar branch is that I make the title, decorated and hide-when-maximized controls insensitive when csd is on, and I use gtk_window_get_titlebar from GTK+ when available.

headerbar - a new headerbar branch, containing just the commit that adds support for GtkHeaderBar as a widget.
Comment 25 Tristan Van Berkom 2014-10-22 05:22:10 UTC
(In reply to comment #24)
> I've pushed two new branches:
> 
> csd - containing the cleaned up csd support, including your editor fixes. The
> only changes on top of what was in the original headerbar branch is that I make
> the title, decorated and hide-when-maximized controls insensitive when csd is
> on, and I use gtk_window_get_titlebar from GTK+ when available.


Hi, thanks for splitting it out.

Sorry to bring up another 'gotcha', but the way you made those properties
appear insensitive is broken, and we have facilities to do that properly.

Broken issues with insensitivity:

  o When loading a project with a window that has CSD turned on, the properties
    are not insensitive

  o When there is more than one window, one with CSD on and another not, 
    selecting one and then the other shows that insensitive state is not
    synced to project state

  o When turning on CSD and doing Undo/Redo, sensitivity does not follow

No worry, you didn't know, just highlighting the reasons of why we have
a facility for this, which is glade_widget_property_set_sensitive(), it
needs to be called from the GladeWidgetAdaptor->set_property() method
whenever the "CSD" property changes state.

Here is an example of how we control sensitivity based on GtkImage's virtual
"edit-mode" property (which shows up as a radio button in the UI):

   https://git.gnome.org/browse/glade/tree/plugins/gtk+/glade-gtk-image.c#n87

As an added bonus, the API also sets the tooltip which will be shown when
the property is insensitive (so we can have that say something like
"This property has no effect while CSD is enabled").

Note, what I also normally do when one property causes another one to be
insensitive, is I also set it to the default value in the issuing GladeCommand
group.

Another GtkImage example:

https://git.gnome.org/browse/glade/tree/plugins/gtk+/glade-image-editor.c#n138

This should also be done for the 'decorated', 'title' and 'hide-when-maximized'
properties from the same command group which possibly deletes the titlebar
child, in glade-window-editor.c


So, to sum it up:

  o Please move the gtk_widget_set_sensitive() calls out of
    glade-window-editor.c and add calls to glade_widget_property_set_sensitive()
    to glade-gtk-window.c in the GladeWidgetAdaptor.set_property() method

  o Please set those property to their defaults undoably from inside the
    GladeCommand group, *only* when the CSD property is becomming TRUE.

And then we can go ahead and merge the branch.
Comment 26 Matthias Clasen 2014-10-23 01:05:51 UTC
> So, to sum it up:

I hope I've done these three things now, pushed a new csd branch with an additional commit.
Comment 27 Tristan Van Berkom 2014-10-23 08:27:10 UTC
(In reply to comment #26)
> > So, to sum it up:
> 
> I hope I've done these three things now, pushed a new csd branch with an
> additional commit.

Great, everything in the csd branch is working great.

I merged it to master now so we can move on.

Also, I made 2 changes on top of your branch:

  o #define CSD_DISABLED_MESSAGE _("This property does not apply...")

    I'm not certain if it's really needed, but it's a practice we've
    been doing for a long time, i.e. avoiding redundant translatable
    strings (we just ensure the same string doesnt show up multiple
    times in the po files).

  o Just made master depend on GTK+ 3.15.0 and removed the GTK_CHECK_VERSION
    ifdefs around gtk_window_get_titlebar().
Comment 28 Tristan Van Berkom 2014-10-23 08:28:35 UTC
Comment on attachment 250961 [details] [review]
Add GtkHeaderBar support

Marking patch as obsolete, headerbar work is currently under review in the headerbar branch
Comment 29 Tristan Van Berkom 2014-10-23 08:29:19 UTC
Comment on attachment 275189 [details] [review]
Initial GtkHeaderBar support

Marking patch obsolete, headerbar work is taking place on the headerbar branch.
Comment 30 Tristan Van Berkom 2014-10-23 08:30:15 UTC
Comment on attachment 275191 [details] [review]
Support CSD windows

Marking patch as obsolete, csd window support was further improved under the 'csd' branch and now merged in master.
Comment 31 Tristan Van Berkom 2014-10-31 07:58:43 UTC
Matthias,

  I took a look at headerbar today and see it's still in the same
state it was in (nothing headerbar related has been addressed since comment 12).

Anyway, here is a list of issues I have with headerbar:

  o What is the user expected to put in "decoration-layout" ?

    Looking at gtk_header_bar_set_decoration_layout(), this looks
    like a rather cumbersome API to expose in Glade, so until we
    come up with a good UI for this, let's just assume the user
    is expected to set a string.

  o What about the "decoration-layout-set" property of headerbar ?

    Is this really a property which should be programatically set ?

    I think it's an implementation detail and should be omitted from
    Glade.

  o Similar story of 'has-subtitle'

    I don't think Glade should allow the user to set the 'has-subtitle'
    property.

    It it important that has-subtitle should be set for a user's
    subtitle to appear ? It's not that clear from a quick read of
    gtkheaderbar.c

    If it's important to set has-subtitle, then we should hide the
    has-subtitle property in Glade and set it to TRUE automatically
    at GladeWidgetAdaptor->write_widget() time.

    If it's not important that the user sets this property, it should
    be disabled in Glade.

  o More orphaned widget issues with the "Custom Title" property.

    This will unfortunately require a custom editor to do properly (hopefully
    that's not too difficult as there are plenty of examples to start from).

    Set "Custom Title" to TRUE, then add a widget in the placeholder at
    the center of the headerbar, then set "Custom Title" to FALSE:

      --> Notice the added child still shows up in the widget tree even
          though the custom title widget was removed, it's still in the
          project.

    Now Hit Undo:

      --> Notice the child which was added does not re-appear properly
          at undo time.

  o Also related to the custom-title property, when the user sets
    the custom title to TRUE, this should undoably clear the text which
    may have previously been set in the "title" property.

  o While custom-title is TRUE, we should have the "title" property
    insensitive with an informative message telling the user that
    the title property does not apply when a custom title is set.
Comment 32 Matthias Clasen 2014-10-31 14:33:28 UTC
thanks for the list. I'll look for some time this weekend to work through it.
Comment 33 Matthias Clasen 2014-11-02 22:54:10 UTC
I pushed a new headerbar branch that should address your concerns by adding a headerbar editor.
Comment 34 Matthias Clasen 2014-11-02 22:56:12 UTC
Note that you need 287319564450211a88e313d99eb3381016ae8074 in gtk+ to avoid a segfault when resurrecting a custom title child via undo.

One thing I couldn't quite figure out is how to make the entire decoration-layout row insensitive when show-decorations is unset. Not really a big deal though.
Comment 35 Tristan Van Berkom 2014-11-03 05:46:35 UTC
Good progress, now that I play with it in depth I'm finding some rougher areas.

 o Placeholder disappears on Undo/Redo

   Steps:
     a.) Create a headerbar (no need for a window or anything, can be in a 
         window or not, doesnt make a difference)
     b.) Set the 'Custom Title' property to TRUE
     c.) Add a button to the left hand placeholder in the headerbar
     d.) Drag'n'Drop the button from the left position to the center placeholder
     e.) Undo
     f.) Redo

     Now notice that the left placeholder has disappeared completely

     In the console is printed the message:

     Gtk-WARNING **: Can't set a parent on widget which has a parent

   This points to a probable error when the adaptor is called with
   replace_child() or add/remove_child() with an existing placeholder
   from the undo/redo code (GladeCommand does not use the GtkContainer API
   directly).

   Note that the message:

     Gtk-WARNING **: gtkcontainer.c:1289: child property `position' of container class `GtkHeaderBar' is not writable

   Is issued any time something is positioned in the headerbar, which may
   indicate that the adaptor code does not have the right expectations.

 o Undo/Redo Crashes

   This issue occurs with the same steps above... now that you have the
   missing placeholder, continue from step f:

     g.) Undo until the project is empty (until before headerbar creation)

     h.) Redo until you get back to the end.

   Now we get a crash, and on the console we see these messages:

     GladeUI-WARNING **: Discrepancy found in TreeModel data proxy. Can not get children iter for widget headerbar1
     GladeUI-WARNING **: Internal data model error, object 0x38cc530 GtkButton not found in tree model
     Gtk:ERROR:gtkcontainer.c:3656:gtk_container_propagate_draw: assertion failed: (gtk_widget_get_parent (child) == GTK_WIDGET (container))


 o Child action for 'Add Slot at start' and 'Add Slot at end' create
   a command group but do not seem to effect project data (and the
   command group does not even have a proper title)

   The way we normally do this for box is we manage the 'size' virtual
   property and the position properties of children (which *are* project data),
   then, as a consequence of project data changing, when the "size" property
   changes, the adaptor ensures there are enough placeholders.

 o When adding a slot at start/end, the "Number of items at start" and
   "Number of items at end" properties are not properly updated

 o "Number of items at start" property can be decreased to 0 even if a
   project widget is added there, oddly, decreasing the number of items
   to 0 does not even cause the added widgets to disappear or orphan
   any project data

 o Copy/Paste does not preserve the number of placeholders on the left
   and right sides of the headerbar

     a.) Create headerbar
     b.) Set the "number of items at start" to 2
     c.) Copy the headerbar
     d.) Create an arbitrary widget or window with placeholders
     d.) Paste the headerbar to the new parent

   Result: The copied headerbar only has 1 placeholder at the start, not 2

   Observations: This only happens with the placeholders at the start,
   it seems that placeholders at the end are handled alright for some
   reason.


Regarding the above four which all have to do with the virtual "start-size"
and "end-size" properties, they need to be implemented properly, i.e.:

  - User issued commands need to effect the actual project data (i.e.
    the headerbar properties).
  - Adaptor must respond to property changes and ensure the existence
    of placeholders accordingly
  - The verify_child_property() vfunc needs to be implemented to ensure that the
    number cannot decrease to an amount which results in orphaned project
    widgets

Let's continue:

 o The 'Add slot at start' / 'Add slot at end' child actions are useful,
   they should also at least be added as widget actions

   There are 2 brands of actions, those that appear in a context menu
   when you click on a child, and those that appear when you click on
   the actual widget, this action is more appropriate for the base
   widget action, but could be useful in for both, currently its only
   declared as a 'child' action.

 o Some coding style issues.

   In glade we do not declare functions with trailing '{' and we always
   put the opening curly brace on a new line, not at the end of an if statement

   Also, we usually:

      call_function (WITH_ARG (one), ARG (two));

   Not

      call_function( WITH_ARG(one), ARG(two) );

   Code style seems to suffer from C++itis ;-)

 o To answer your question about making the decoration-layout properties
   insensitive, it is done in the normal way from the adaptor code using
   the glade_widget_property_set_sensitive() functions (as we did already
   in another one of your patches).

   Which additionally give a useful message, such as:

     "This property does not apply when decorations are disabled"

 o I can see what you did there with the property bindings in the .ui file.

   I would prefer if this was done using the standard mechanics which
   also provide the explanitory tooltip when the properties are insensitive.
   
   But more importantly, Glade does not support the property GBinding
   GtkBuilder semantics, which means that we cannot really use Glade to
   edit the GladeHeaderBarEditor, that is more of a problem than the former
   issue.

   Tip: In case you were unable to edit Glade's editors with Glade, you
        need to './configre --enable-gladeui'

        You can then use the internal assets such as GladeEditorSkeleton,
        GladePropertyLabel, GladePropertyShell etc.

        Also, if you need interaction with some of the internal editors
        which are part of the GTK+ catalog (i.e. derive or embed other editors),
        then you can also add the gtk-private catalog to glade's palette which
        is found in plugins/gtk-private/glade-gtk-private.xml

 o The show-window-controls property should be set to 'ignore'

   Enabling the show-window-controls property in the workspace causes the
   automagick 'close' button to appear.

   Clicking on the automagick 'close' button on a headerbar in the workspace
   causes Glade's application close handling to occur (either Glade unexpectedly
   closes, or a popup comes up asking you if you are sure you want to close
   without saving).

In conclusion, the glade-gtk-headerbar.c code needs attention more than
anything else, especially wrt how placeholders are created, how the
adaptor add_child()/remove_child()/replace_child() methods are implemented,
and how the virtual "start-size" and "end-size" properties are broken.

I would be willing to merge this without the "start-size"/"end-size" *IF* it
can be done without crashes/errors, one drawback is that it would be impossible
to acheive copy/cut/paste while preserving the number of placeholders in the 
copied headerbars (a copied headerbar would either have project widgets or
a default of at least 1 placeholder on each side).

However this is not how we've ever done things, so I can't really guarantee
that following the road without "start-size" / "end-size" would not end in
tears, so to speak. The correct/tried way is to drive properties with
GladeCommands, and to drive placeholders in the adaptor when start-size/end-size
is changed (in GladeWidgetAdaptor->set_property()).
Comment 36 Matthias Clasen 2014-11-05 03:00:42 UTC
Not sure I got it all 100% right, but I've pushed a number of fixes, and undo/redo feels a lot more stable now.
Comment 37 Tristan Van Berkom 2014-11-05 11:51:57 UTC
Today I worked on your headerbar branch, I needed to rebase and push
a new branch because of a dependant core fix I pushed to master.

First I fixed property sensitivity issues as you were calling
glade_widget_property_set_sensitive() from the editor loading code
and not from the adaptor code.

See the following commit for details:

commit c4fccac0405d88c7d3296c5047ad75401792f59b
Author: Tristan Van Berkom <tristan@upstairslabs.com>
Date:   Wed Nov 5 18:38:55 2014 +0900

    GtkHeaderBar: Fixed property sensitivity issues


Then I fixed the core drag & drop logic so that packing properties
are properly recorded from the source parent when dragging away,
annoyingly it seems that drag & drop was not properly tested before
merging that.

The fix is in the following commit:

commit 3f5e029817028334b349d80b7286d79426dff43c
Author: Tristan Van Berkom <tristan@upstairslabs.com>
Date:   Wed Nov 5 19:55:07 2014 +0900

    GladeCommand: Experimental - properly record child type for removal 
    commands.


Then I went on to try to fix your adaptor implementation and made a lot
of progress, but it's still not quite done (note that the crashes I mentioned
in my last review, the really critical part, were not yet fixed), the
improvements are on your headerbar branch in the commit:

commit f0092f1a7e38072eb193f005fe93136e0def2ab8
Author: Tristan Van Berkom <tristan@upstairslabs.com>
Date:   Wed Nov 5 20:08:32 2014 +0900

    GtkHeaderBar: Improving child add/remove/replace support


There are still serious issues with the adaptor, and it mostly revolves
around the adaptor not creating placeholders dynamically to fit the
data model's expectations of the "start-size" and "end-size" properties,
and also not positioning child widgets properly when the "position"
packing property is set.

Here is a simple procedure to follow to reproduce the issues I'm talking about:

  o Create a new headerbar in the workspace

  o Add a button at the end of the headerbar

  o Drag the button from the end position and drop it in the start position
    (NOTE: You will need to select the headerbar first, since Drag&Drop wont
    take effect on an already selected widget)

  o Undo -> Widget goes back to the end as expected

  o Redo -> Widget goes back to the beginning as expected

  o Undo -> Now the widget goes back to the end, but now there is
    a button and a placeholder at the end, and no placeholder at the start

  o Redo -> Now the button is still in the project, but it's not added to
    the headerbar (it's just missing/gone/orphaned), and we have a headerbar
    with one placeholder on each side.


The way things work in Glade with regards to children and positioning is that 
all data about child positioning is stored in packing properties on the modal.

When Copy/Paste/Drag/Drop/Undo/Redo occurs, whenever a child is added to
a container:

  o First time around ->replace_child() is called, this continues to be
    called so long as the placeholder exists, however the placeholder is
    transient and later can be destroyed (it's not project data).

  o After the core uses ->replace_child() the first time, packing property
    values after the widget insertion are introspected and saved for later
    use

  o Every subsequent time we add children, Glade follows the following rule:

     - Use GladeWidgetAdaptor->add_child() to get the widget into the container

     - Set the previously saved packing properties so that the widget is
       in the correct position and whatnot.

From the adaptor side, it's your choice if you want to clear all the 
placeholders and fill in the gaps with new placeholders at ->add_child()
and ->remove_child() time, or if you want to preserve the existing placeholders.

The GtkGrid implementation is extreme in this regard and just wipes all
existing placeholders and adds a complete new set of placeholders only in
positions where no actual project child exists.

Since the headerbar's position property is read-only for whatever reason,
this presents a difficulty in preserving child order, there are two paths
which can be followed:

  a.) Fix GtkHeaderBar so that the "position" property is writable and
      properly reorders it's children

  b.) Whenever ->add_child() or ->remove_child() is called on your adaptor,
      and whenever ->child_set_property() is called on your adaptor:

      You could remove everything, destroying the placeholders and re-add
      widgets in the proper order, adding a placeholder for every 'gap'
      so that you satisfy the 'start-size' / 'end-size' and that actual
      project widgets are found at the "position" which is stored in their
      packing property values (those which are stored in Glade's datamodel).


I realize that this was originally somebody else's patch, and it's annoying
to have to actually understand how adaptors work - however as you can see
the previous patch was rather naive and the implementation of something like
this is complex (hence why we never got around to writing all that code to
support headerbar until this point).

So, all I can say is that I'm sorry that it really costs work to get this
working, I wish I could spend additional days working on this myself but
I probably not do this again this year.

I hope the guidance and work that I've put in so far is enough to push this
through to a mergable state soon, it's very close aside for these important
missing pieces.
Comment 38 Tristan Van Berkom 2014-11-05 12:04:17 UTC
Additional note:

In glade-gtk-headerbar.c I've added a very simple debugging path,
i.e. a commented out #define d(x) x, you can uncomment that to enable
the tracing in that file if you find that usefull.

Additionally, you can run Glade with:

   GLADE_DEBUG=commands:properties glade

To get more tracing about what is happening when widgets are added and
removed, and what (packing) properties are set to what values at undo/redo time.

Hope this is useful.
Comment 39 Tristan Van Berkom 2014-11-06 13:54:37 UTC
Steps to reproduce the orphaned widget and critical message in the console:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

 - Create a headerbar in the workspace by dragging it from the palette into the workspace

 - Set the "size" property to 2

 - Set the Custom Title property to TRUE

 - Add a GtkButton to the second placeholder

   Now the headerbar layout is:

     Placeholder | Button | Placeholder in the custom title

 - Set the Pack Type packing property of the button to End

   Now the headerbar layout is:

     Placeholder | Placeholder in the custom title | Button 

 - Select the headerbar (so that the button is not selected)

 - Drag the button from the right side into the center

   Now the headerbar layout is:

     Placeholder | Placeholder | Button in the custom title

 - Undo until you reach the initial state of the added headerbar
   where there is only one placeholder

 - Redo one (this adds a placeholder)

   Now the headerbar layout is:

     Placeholder | Placeholder

 - Redo again (this sets custom title to TRUE)

   Now the headerbar layout is:

     Placeholder | Placeholder | Placeholder in the custom title

 - Redo again (this adds the GtkButton to the project)

Now the button is in the project (see it in the inspector) but it does not
show up in the workspace, additionally the critical message is printed
in the console:

Gtk-CRITICAL **: gtk_container_remove: assertion 'gtk_widget_get_parent (widget) == GTK_WIDGET (container) || GTK_IS_ASSISTANT (container) || GTK_IS_ACTION_BAR (container)' failed
Comment 40 Tristan Van Berkom 2014-11-07 10:21:39 UTC
I have entered the bug 739764 to track the issues with spurious ref counting
imbalances related to drag & drop in placeholders.

Noting that the above steps to reproduce only occur when dragging the button
to add it to the headerbar, I'm sufficiently satisfied that this is in fact
not a bug related to the headerbar adaptor in any way.

I've went ahead and merged the headerbar branch now, all of the critical
issues have been addressed and anything further would be an enhancement
to the currently functional headerbar.

Thanks Matthias for all your hard work on this.