The patch in bug #535090 is required for vertical text or compact layout in GtkToolItems. 

Also, Mathias Clasen said in an email to Mathias:

"
I had a quick look at the egg code, and so far saw a few small issues:
 - hardcoded header color (try running testtoolpalette with hicontrast)
 - animation should follow the gtk-enable-animations setings we have
"

Bug #535095 might also still have something that needs to be done.

Created attachment 129304 [details] [review]
Adds GtkToolPalette to gtk+

* Namespace changed from Egg to Gtk
* Implement gtk-enable-animation
* Complete documentation and integrate into gtk+ documentation system (image in next attachment). Marked all methods as "Since: 2.18"

I wasn't sure about the fixed color this Matthias pointed out in comment #1, maybe someone can give me a hint here

Created attachment 129305 [details]
Image for reference documentation (put into docs/reference/gtk/images

The patch still contains the #ifdefs for bug #535090 which hasn't been decided upon yet. The other #ifdefs (for older gtk+, etc.) have been removed.

(In reply to comment #3)
> Created an attachment (id=129304) [edit]
> Adds GtkToolPalette to gtk+
> 
> * Namespace changed from Egg to Gtk
> * Implement gtk-enable-animation
> * Complete documentation and integrate into gtk+ documentation system (image in
> next attachment). Marked all methods as "Since: 2.18"
> 
> I wasn't sure about the fixed color this Matthias pointed out in comment #1,
> maybe someone can give me a hint here
> 
I think this only is a problem of the test program, as I parse some RC string in its main function to highlight the palette header buttons.

(In reply to comment #5)
> The patch still contains the #ifdefs for bug #535090 which hasn't been decided
> upon yet. The other #ifdefs (for older gtk+, etc.) have been removed.

What functionality is missing if that API is not available? Or, what bug is present if it isn't available?

AFAIK, text-orientation and ellipsize mode cannot be set for individual tool item. So, some features are simply unavailable though I wouldn't consider this a bug.

I guess Mathias can give you more details.

Ok, some things noticed in passing (not a full review yet):

- GtkToolPalette should implement GtkOrientable 

- GtkToolbar has settings for icon size and toolbar style which are used unless explicitly overridden. I think GtkToolPalette should do the same. Steps:
  - Move gtk-toolbar-style and gtk-toolbar-icon-size settings to GtkSettings
  - Add icon-size-set/toolbar-style-set boolean properties like GtkToolbar has
  - Use the setting unless the -set property is TRUE

- GtkToolPaletteClass should have padding

- Can't these be no-window widgets ? Honest question, I haven't looked closely for reasons why not.

- It would be great to see an example of this in gtk-demo

Briefly looked at the patch, all around it looks like
a big step up from the code we use in Glade.

One minor thing: the GtkToolItemGroup should allow
you to set a custom label as the expander does.

also found a little bug: PROP_ELLIPSIZE is
getting assigned to the "header-relief" property.

Actually I wonder if ellipsize should be exposed
at all in this case or if that should be left entirely
up to the child label.

Reading the code its hard to tell, is there some
size request/allocation magic in this label/ellipsize
code that would otherwise break if left open to
the user ?

Oops, also GtkToolItemGroup:name property conflicts with GtkWidget:name

(In reply to comment #9)
> - It would be great to see an example of this in gtk-demo

I have put the patch in a toolpalette branch at git.gnome.org, and I added a demo. The rest is still to do. I might have time tomorrow, but Johannes, feel free to fix it instead.

(In reply to comment #9)
> Ok, some things noticed in passing (not a full review yet):
> 
> - GtkToolPalette should implement GtkOrientable 
> 
> - GtkToolPaletteClass should have padding

> also found a little bug: PROP_ELLIPSIZE is
> getting assigned to the "header-relief" property

I did these easy things too, in the toolpalette branch.

Thanks, I fixed a small issue with your demo.
Here are some more things I noticed playing with it:

I notice that changing the orientation only affects the orientation of groups, not the placements of items inside groups - the items are always organized in rows. I wonder if that ought to be flipped to always be opposite to the group orientation ? Or do we need a separate property for that (with a special 'opposite' value, maybe) ?

I was surprised by the behaviour of the items in the 'advanced features' tab in horizontal mode - are they not to supposed to expand in that mode ?

> One minor thing: the GtkToolItemGroup should allow
> you to set a custom label as the expander does.

Done.

Minor nitpick I noticed in the commit logs. You probably don't want to add empty entries in docs/reference/ChangeLog.

Matthias wrote:
> - GtkToolbar has settings for icon size and toolbar style which are used unless
> explicitly overridden. I think GtkToolPalette should do the same. Steps:
>   - Move gtk-toolbar-style and gtk-toolbar-icon-size settings to GtkSettings
>   - Add icon-size-set/toolbar-style-set boolean properties like GtkToolbar has
>   - Use the setting unless the -set property is TRUE

Done, based on the code in GtkToolbar, though I don't think it works and I think I broke it for GtkToolbar too. Changing the preference in the Appearance control panel no longer causes the Main Window demo's toolbar to change.
http://git.gnome.org/cgit/gtk+/commit/?h=toolpalette&id=eb76666f1c25d4cd38d231dca49c960c6d5e3b4d

I was surprised that I had to patch GtkSettings to let me use an enum for a setting - maybe that has something to do with it.

I did not add a toolbar-style-set property to GtkToolPalette (though I added the function and the behaviour), because GtkToolbar doesn't have one. I don't know why it has icon-size-set but not toolbar-style-set properties.

(In reply to comment #14)
> Thanks, I fixed a small issue with your demo.
> Here are some more things I noticed playing with it:
> 
> I notice that changing the orientation only affects the orientation of groups,
> not the placements of items inside groups - the items are always organized in
> rows. I wonder if that ought to be flipped to always be opposite to the group
> orientation ? Or do we need a separate property for that (with a special
> 'opposite' value, maybe) ?

The current behaviour feels OK to me:
- Vertical groups, with horizontal rows of icons.
- Horizontal groups, with horizontal rows of icons.

But I can't say why. Even if we make it optional, we'd have to choose a default. 

I was about to write a few lines about how important this
was when I remembered the way the palette scales.

Currently whats important to me is that the user can use it
in a way that saves him space on the desktop, which means
he may want a square area, he may want a single row of icons,
or a single column of icons.

If Im not mistaken, this palette; regardless of which order
the icons are populated in, will fit the icons into the allocated
space by moving them around, making it just as possible in both
modes to get a single line of icons both vertically or horizontally.

... Ok I'll go run off and build the branch and newer demo hopefully
later today...

(In reply to comment #18)
> The current behaviour feels OK to me:
> - Vertical groups, with horizontal rows of icons.
> - Horizontal groups, with horizontal rows of icons.

Thinking again, I can't see any problem with us changing it to:
- Vertical groups, with vertical columns of icons, flowing down and then to the right.
- Horizontal groups, with horizontal rows of icons, flowing left-to-right and then down.

Agreed? That wouldn't need any new API.

(In reply to comment #19)
> Currently whats important to me is that the user can use it
> in a way that saves him space on the desktop, which means
> he may want a square area, he may want a single row of icons,
> or a single column of icons.

Well, the width (in vertical mode) or height (in horizontal mode) can't currently be less than the size of the group expander's label. That's the same as in glade-3.

Which brings up the question of what to do with the group labels in horizontal mode. At the moment we rotate the text by 90 degrees, which looks a little strange.

(In reply to comment #21)
> (In reply to comment #19)
> > Currently whats important to me is that the user can use it
> > in a way that saves him space on the desktop, which means
> > he may want a square area, he may want a single row of icons,
> > or a single column of icons.
> 
> Well, the width (in vertical mode) or height (in horizontal mode) can't
> currently be less than the size of the group expander's label. That's the same
> as in glade-3.

Yeah we can live with this limitation.

(but I wouldnt object to some complex code that tries to do
a better job of laying out the arrow/text in those cases either ;-))

> Which brings up the question of what to do with the group labels in horizontal
> mode. At the moment we rotate the text by 90 degrees, which looks a little
> strange.
> 

I think ultimately we should expose the orientation of the layout
and the orientation of the expanders separately, because I just think
I cant wrap my head around all the better use cases.

Here is just an idea about how a horizontalish layout might be 
visually pleasing (Lets hope text width is still reliable for my 
little ascii demonstration ;-)

Collapsed:

+-----------------------------+
| Group 1 | Group 2 | Group 3 |
+-----------------------------+

Expanded:

+------------------------------------------------------+
| Group 1 | item 1 | item 2 | item 3 | item 4 | item 5 |
|------------------------------------------------------+
| Group 2 | item 1 | item 2                            |
|------------------------------------------------------+
| Group 3 | item 1 | item 2 | item 3 | item 4 | item 5 |
|------------------------------------------------------+
|         | item 6 | item 7 | item 8                   |
+------------------------------------------------------+

Expanded and collapsed:

+------------------------------------------------------+
| Group 1 | item 1 | item 2 | item 3 | item 4 | item 5 |
|------------------------------------------------------+
| Group 2 | item 1 | item 2 | Group 3                  |
+------------------------------------------------------+

Now Im not sure what I would do with vertical text, it would
actually interest me to use vertical text for collapsed groups
in order to save space, assuming my text is short enough
to still ensure the possibility of a single row of items.

Im not sure how much of that is achievable with what we
have, personally I would be satisfied with vertical vs.
horizontal mode of item layout, and I would rather
just keep my text horizontal and readable at all times.

> > Well, the width (in vertical mode) or height (in horizontal mode) can't
> > currently be less than the size of the group expander's label. That's the same
> > as in glade-3.
> 
> Yeah we can live with this limitation.

OK, so let's punt that issue to a future version. It's not a regression for Glade.

> I think ultimately we should expose the orientation of the layout
> and the orientation of the expanders separately, because I just think
> I cant wrap my head around all the better use cases.

And let's punt that too. It could be additional API later. The current behaviour matches Glade's current behaviour at least.

> Done, based on the code in GtkToolbar, though I don't think it works and I
> think I broke it for GtkToolbar too. Changing the preference in the Appearance
> control panel no longer causes the Main Window demo's toolbar to change.
> http://git.gnome.org/cgit/gtk+/commit/?h=toolpalette&id=eb76666f1c25d4cd38d231dca49c960c6d5e3b4d

I tried with the latest version of the toolpalette branch installed and the appearance applet worked fine for all applications that I tried. Maybe the demo is setting it's own style somehow. This was with glib master in case that's somehow interesting.

Johannes, so did the demo show the bug for you?

(In reply to comment #25)
> Johannes, so did the demo show the bug for you?

The demo does not change it's appearance, not even with stock gtk+ 2.16 so I don't think you introduced a bug here. I am still locking for the reason that gtk-demo does not behave like anything else though.

Johannes, I suspect that you are just not linking to the self-built Gtk+ when not using the demo.

I even installed the toolpalette branch system-wide (in /usr) to be sure it's really used so I am quite sure I used the correct libraries. And as I said, even the demo that got installed with gtk+ 2.16 (and with only gtk+ 2.16 installed) doesn't change the style when it is changed in the appearance capplet.

OK, so maybe there's no problem there.

mclasen, I think this is ready to go into GTK+. But I guess we've missed the current cycle, right?

Yes, it'll go in as soon as the next cycle opens

* branch toolpalette is now up-to-date with master
* fixed a bug with handling of custom label widgets in GtkToolItemGroup
* fixed API docs to say 2.20

From my side this is ready to merge. I didn't find anymore bugs while testing and client-side-windows don't seem to a problem.

Ok, built it and ran the demo. I notice two things that feel slightly odd to me.

1.In horizontal mode, switching toolbar-style from anything else to 'text' seems to change the content packing from rows with horizontal text to columns with vertical text, which feels quite disorienting and, frankly unusable, to me.
I think 'text' should have same layout as 'both', just without the icons. If we think we need vertical text there, that should probably be done via some separate knob.

2. In vertical mode, the fade-out when collapsing is nice and smooth, but in horizontal mode, it looks like it is rearranging items, which makes it feel a bit jumpy. Not sure there is much that can be done about it...

Code review to follow...

One more comment on the demo: it would be good to have a demo that shows DND with the palette. Could be in the same demo, or a separate one.


Some small things I noticed while looking over the code, starting in gtktoolitemgroup.c:

Both gtktoolitemgroup and gtktoolpalette have a private member for settings. We don't do that elsewhere; if gtk_widget_get_settings() performance is a problem, the caching should happen in GtkWidget itself.


  if (! strcmp (pspec->name, "gtk-enable-animations"))
  {
    animation_change_notify (group);
  }

Indentation is off here.
We don't to {} in this case anyway.
I personally prefer strcmp () == 0.
There's more instances of these 3 issues elsewhere in the code.


static void
gtk_tool_item_group_header_clicked_cb (GtkButton *button G_GNUC_UNUSED,
                                       gpointer   data)

We don't have G_GNUC_UNUSED annotations elsewhere in the code. I personally would prefer to leave them out here, and introduce them separately, if we decide to do so.


Inconsistent spacing between the cases in gtk_tool_item_group_set_property


Since: 2.18 should be Since: 2.20 now, throughout


      /*
      gtk_label_set_use_underline (GTK_LABEL (child), group->priv->use_underline);
      gtk_label_set_use_markup (GTK_LABEL (child), group->priv->use_markup);
      */

Commented out code like this should be removed


static gboolean
gtk_tool_item_group_animation_cb (gpointer data)

Looks like this callback is missing GDK_THREADS_ENTER/LEAVE protection, since it is not using the gdk_threads_add_timeout functionality,


/* Ensure that all composited windows and child windows are repainted, before
   * the parent widget gets its expose-event. This is needed to avoid heavy
   * rendering artifacts. GTK+ should take care about this issue by itself I
   * guess, but currently it doesn't. Also I don't understand the parameters
   * of this issue well enough yet, to file a bug report.
   */
  gdk_window_process_updates (GTK_WIDGET (group)->window, TRUE);

I think we should make sure we understand this issue well enough to at least file a bug report before merging...


* Returns: the label of @group. The label is an internal string of @group and must not be modified.

Only if you haven't used set_label_widget() before, right ?


Needs to be hooked up to the gtk.symbols machinery - I can take care of that.


On to gtktoolpalette.c:

application/x-GTK-tool-palette-item

I would prefer to use application/x-gtk-tool-palette-item here, since all our other dnd targets seem to be  all-lowercase.


I don't really believe that the complicated business with groups_size, groups_length and sparse_groups is worth it. Why not just have a straight array that is resized by +/-1 as needed ? It also seems to cause bugs, e.g. it looks to me like dispose() assumes that the groups array is not sparse ?
Or maybe use a GPtrArray.


  if (palette->priv->hadjustment)
    g_object_unref (palette->priv->hadjustment);
  if (palette->priv->vadjustment)
    g_object_unref (palette->priv->vadjustment);

  if (hadjustment)
    g_object_ref_sink (hadjustment);
  if (vadjustment)
    g_object_ref_sink (vadjustment);

Should probably do this the other way around, otherwise people will file bugs about not being save for re-setting the same adjustment.


   * The ::set-scroll-adjustments when FIXME

Needs fixing :-)


  /**
   * GtkToolbar:icon-size-set:
   *
   * Is %TRUE if the icon-size property has been set.
   *
   * Since: 2.10
   */

Copy-paste oversight.


There's some trailing whitespace throughout the code. While we are not very consistent about this at all, newly added files should preferably be free of that.


* Sets the tool palette as drag source (see gtk_tool_palette_set_drag_source) 

This should be gtk_tool_palette_set_drag_source() to get proper linkification by gtk-doc.

(In reply to comment #33)
> One more comment on the demo: it would be good to have a demo that shows DND
> with the palette. Could be in the same demo, or a separate one.

I think there is/was such a demo in the eggtoolpalette code. I will port it over to gtk+.

> 
> Some small things I noticed while looking over the code, starting in
> gtktoolitemgroup.c:
> 
> Both gtktoolitemgroup and gtktoolpalette have a private member for settings. We
> don't do that elsewhere; if gtk_widget_get_settings() performance is a problem,
> the caching should happen in GtkWidget itself.

It is used to compare the "old" settings to the current settings in some places. I wasn't sure how to replace that correctly. At least it is not used for caching.

> Since: 2.18 should be Since: 2.20 now, throughout

Oh, forgot to push this last time...

> static gboolean
> gtk_tool_item_group_animation_cb (gpointer data)
> 
> Looks like this callback is missing GDK_THREADS_ENTER/LEAVE protection, since
> it is not using the gdk_threads_add_timeout functionality,

I hope I got this right now.
 
> 
> /* Ensure that all composited windows and child windows are repainted, before
>    * the parent widget gets its expose-event. This is needed to avoid heavy
>    * rendering artifacts. GTK+ should take care about this issue by itself I
>    * guess, but currently it doesn't. Also I don't understand the parameters
>    * of this issue well enough yet, to file a bug report.
>    */
>   gdk_window_process_updates (GTK_WIDGET (group)->window, TRUE);
> 
> I think we should make sure we understand this issue well enough to at least
> file a bug report before merging...

I need to have a closer look at this.

 
> I don't really believe that the complicated business with groups_size,
> groups_length and sparse_groups is worth it. Why not just have a straight array
> that is resized by +/-1 as needed ? It also seems to cause bugs, e.g. it looks
> to me like dispose() assumes that the groups array is not sparse ?
> Or maybe use a GPtrArray.

I used a GPtrArray and refactored the code. This also removes all this sparse handling and makes the code more readable. Reordering of groups could be a bit slower now as it uses sorting but I don't think this is a noticeable performance issue.
 
The text alignment is now always horizontal so it fits with the icon+text style. The text layout can't be changed in vertical though if someone would ever want to do this.

For the animation, I couldn't see a difference between horizontal and vertial mode. Possibly it's too fast or too slow to notice this here.

I hope all the minor style issues mentioned are now also fixed in the toolpalette branch.

> I think there is/was such a demo in the eggtoolpalette code. I will port it
> over to gtk+.

There is now an extended dnd example in the gtk-demo.
 
> > /* Ensure that all composited windows and child windows are repainted, before
> >    * the parent widget gets its expose-event. This is needed to avoid heavy
> >    * rendering artifacts. GTK+ should take care about this issue by itself I
> >    * guess, but currently it doesn't. Also I don't understand the parameters
> >    * of this issue well enough yet, to file a bug report.
> >    */
> >   gdk_window_process_updates (GTK_WIDGET (group)->window, TRUE);
> > 
> > I think we should make sure we understand this issue well enough to at least
> > file a bug report before merging...
> 

I removed that code and tested the whole thing without noting ANY difference so I left this out for now.

I've done some final integration and cleanup work and merged the branch into master. Thanks !

Why do we have, for instance,
gtk_tool_palette_set_group_position (GtkToolPalette *palette,
                                     GtkWidget      *group,
                                     gint            position)

instead of 
gtk_tool_palette_set_group_position (GtkToolPalette *palette,
                                     GtkToolItemGroup *group,
                                     gint            position)

(The group parameter is documented as being a GtkToolItemGroup.)

I know that there's a (strange to me) GTK+ convention of using GtkWidget* for return types, but surely we take the actual type for an input paramter?

May I fix this in git?

mclasen, may I please fix this?

Yeah, sounds ok to fix

Done. Thanks. There's some inconsistency here too. Either they should all return GtkWidget* or they should all return the actual types that they are documented as returning:

GtkToolItem*                   gtk_tool_palette_get_drop_item         (GtkToolPalette            *palette,
                                                                       gint                       x,
                                                                       gint                       y);
GtkWidget*                     gtk_tool_palette_get_drop_group        (GtkToolPalette            *palette,
                                                                       gint                       x,
                                                                       gint                       y);
GtkWidget*                     gtk_tool_palette_get_drag_item         (GtkToolPalette            *palette,
                                                                       const GtkSelectionData    *selection);

Created attachment 151653 [details] [review]
0001-GtkToolPalette-Change-gtk_tool_palette_get_drop_grou.patch

This is the patch I'd like to commit. The other function can actually returns two possible types, so GtkWidget is an appropriate common denominator.

Comment on attachment 151653 [details] [review]
0001-GtkToolPalette-Change-gtk_tool_palette_get_drop_grou.patch

Ok, please commit it.

Created attachment 151688 [details] [review]
Fix compilation warnings

Here a patch to fix compilation warnings introduced in the latest patch