Created attachment 273447 [details]
with this result

*** Bug 724033 has been marked as a duplicate of this bug. ***

So here are my concerns:

 - the reuse of the icon attribute here is problematic because this is for
   noun icons and you're using it for verbs.  This is a problem in case we
   don't support the hint: we wouldn't want the cut/copy/paste icons shown
   beside the menuitems in this case

 - we may want to support some more interesting usecases here, like the
   Firefox-style menuitem-and-submenu

     [ Open    | > ]  ((recent items))

   or from Chome:

     [ - ] [zoom level] [ + ]

   and it seems like these cases are fairly related to what we have here...

I wonder how it possible to add something like this:
https://raw.github.com/gnome-design-team/gnome-mockups/master/nautilus/nautilus-next/gear-menu.png

(How to add the switch buttons and the scale for zoom.)

I'm not entirely sure about this. To me the advantage of popovers is that they are a generic container, and therefore allow creative combinations of widgets. This bug looks a bit like we are encouraging a standard popover design. I fear that we might encourage a cookie cutter approach.

Also, as Yosef points out, we won't always want the "icon buttons on top, menu below" approach.

The patch allows iconic layout for any section, not just on top.

If we want more 'crative combinations', we can investigate an approach of

"Here's a widget, please show that above/below the standard model"

Created attachment 274535 [details] [review]
Add support for extra widgets to model-based popovers

This allows to insert custom widgetry in the middle of model-based
popovers.

Created attachment 274543 [details] [review]
Add support for extra widgets to model-based popovers

This allows to insert custom widgetry in the middle of model-based
popovers.

Created attachment 274626 [details] [review]
Add support for extra widgets to model-based popovers

This allows to insert custom widgetry in the middle of model-based
popovers.

Review of attachment 274626 [details] [review]:

Just a type when building gtk+ with the patch:
gtkentry.c:2009: Warning: Gtk: multiple comment blocks documenting 'GtkEntry:inner-border:' identifier (already seen at gtkentry.c:810).
Traceback (most recent call last):

+ Trace 233485

/usr/share/gobject-introspection-1.0/Makefile.introspection:153: recipe for target 'Gtk-3.0.gir' failed
make[4]: *** [Gtk-3.0.gir] Error 1

Created attachment 274716 [details] [review]
Add support for custom widgets to model-based popovers

This allows to insert custom widgetry in the middle of model-based
popovers. The testpopover testcase includes some examples of this.`

Excerpts from a recent irc discussion:


<desrt> mclasen: imho, if we're going to grow the scope of gmenumodel here then any additions we make should try very hard to have the same flavour as the rest of the thing
<desrt> which means that all static data in comes via the model
<desrt> and all interaction goes via actions
<mclasen> you can point me at a better solutions, but I don't think you can convince me that we'll just not do it
<desrt> having weird side-channels out starts turning this into something very much 'other'
<desrt> mclasen: i told you the solution i like: some global registration system (possibly gtype) which gets data in and out via attributes on the menuitem and via actions
<desrt> then i imagine we will grow some reusable components
<desrt> a slider may be a popular one
<desrt> or a zoom in/out combined widget
<desrt> which attaches to a zoom action with state
<mclasen> it doesn't happen to be the case that larsu already implemented this solution for unity ?
* mclasen would very much like to hear about existing code
<desrt> right... that's why i wanted his opinion
<desrt> iirc the unity one is based on loading of named .so files
<desrt> which i don't really think is applicable here
<mclasen> eek
<mclasen> lets not grow a plugin system at the same time
<desrt> i agree
<desrt> it's just another way of having a namespace, though
<desrt> we spent some time talking about the way to satisfy the canonical designer's desires to shove everything and anything into their menus
<larsu> desrt: it's not based on loading .so files
<larsu> it's a global registration thing
<desrt> shows what i know :)
<larsu> (used to be based on GType btw)
<desrt> the important part is that the widgets get access to the properties on the menuitem and also (open) access to the actiongroup
<desrt> so you can have multiple actions, for example
<desrt> not just a simple action::target-based approach like with actionable/actionhelper
<larsu> I don't think we use that anywhere, though
<larsu> it's like that becasue you can't reasonably implement gtkactionable from outside of gtk
<desrt> larsu: aren't they using gmenumodel for the entire settings UI these days?
<larsu> desrt: only for some of it
<larsu> like I said, I've grown _very_ sceptical of this approach
* desrt seems to recall some conversation about all the custom widgets that would be required
<desrt> larsu: you have more experience in this area than just about anyone else....
* mclasen wonders if the GType+attributes approach will turn into 'embed GtkBuilder fragments'
<desrt> mclasen: 'embed gtkbuilder fragments' is not an awful approach, if you mean templates...
<larsu> heh, that's actually a neat idea
* desrt has been doing a bit of that lately and it works pretty nicely
<larsu> desrt: what for?
<desrt> (side project: adding support for dynamic instantiation of gtklistboxrow for populating a gtklistbox)
<desrt> define the layout of your row widgets via gtkbuilder...
<larsu> how do you need templates for that?
<desrt> you don't
<desrt> it just turns out to be convenient
<larsu> ah, okay
<desrt> my rows are a bit complex....
<desrt> i have the play/pause icon... an album art image... a couple of labels for track/title/etc.
<desrt> oh... and the song length display on the right
<larsu> right - I've done the same, it's quite a neat approach
<mclasen> I just meant that these look very similar:         http://ur1.ca/h4z3k
<desrt> ya.  they do.
<desrt> sorta makes larsu's point, though
<desrt> "why not just use gtkbuilder?"
<larsu> definitely. There's no real point in having both
<larsu> just more stuff to learn
<larsu> maybe we should just add a <widget> child tag to <menuitem> *cough*
<desrt> if that's the answer then we definitely need to make the actionmuxer more accessible, though
<larsu> which is what I've been talking about for two cycles now...
<desrt> the observation that actionable is impossible to implement from outside is completely true... and if we want to have sliders and such hooked to actions then we want to fix that
<mclasen> desrt: because GtkModelButton is not public and gtkbuilder doesn't know how to turn menu models into popovers ?
<desrt> larsu: so wait.....
<desrt> larsu: we embed gmenumodel markup inside of gtkbuilder and then gtkbuilder inside of gmenumodel again? :)
<larsu> yes?!
<desrt> mclasen: right... another longterm goal is that gtkmodelbutton should _be_ gtkbutton...
<desrt> i'm not sure how we would expose this now without introducing serious confusion into the choice of which button to use
<desrt> the good news is that for simple uses, if you already know what type of appearance you want your button to have, you don't have to use the modelbutton...
<desrt> since normal gtkbutton is actionable
<larsu> what does modelbutton add?
<mclasen> a checkbutton looks different than a modelbutton with action-type check, unfortunately
<desrt> it's the dream of a future where we have only one class that does all the things depending on the kind of action you have inside
<desrt> mclasen: is that theming, or...?
<mclasen> no, the placement of the checkbox is hardcoded in the draw() implementations
<desrt> too bad you won't be in berlin next week :/
<desrt> what would you need in order to get to a place where you could do the popovers with straight gtkbuilder?
<desrt> does it really just come down to a different look for checkboxes?
<mclasen> all the items are different
<desrt> it's also a bit lame that we tell people that they can use menumodel until they get to a certain point and then they have to convert everything
<desrt> i think what would help me a lot here: a (complete as possible) list of items we want to put into popovers, from the designers
<desrt> sliders, cut/copy/paste, zoom selection widget, grouping of buttons with icons...
<desrt> is that everything?  will there be more?
<LRN> can i put popovers into popovers?
<desrt> it would help to have some concrete cases to argue
<mclasen> I don't think you're going to get that list out of them
<mclasen> thats a very un-designer-y thing to do
<desrt> mclasen: why?
<owen> desrt: wouldn't you think that nomatter what the app designers want to do know, application authors will want to do other things?
<mclasen> because they want the freedom to explore the half-menu-half-dialog space
<desrt> owen: gtk apparently takes the tack that we don't allow 'just anything'
<desrt> mclasen: ya.. this is a fair argument
<owen> desrt: really?
<owen> desrt: we might not allow just anything with built-in pieces - but there's always been a considerable amount of ability to roll-your-own
<owen> desrt: popovers seem (to me) to be a flexible mechanism - not like some sort of menu-on-steroids
<mclasen> menus always had restrictions... popover menus inherited that
<desrt> owen: i'm inclined to agree with that
<desrt> which sort of begs the question of why we have menumodel-based popovers
<desrt> aside from the fact that it seems to be the most convenient thing to use if you're interested in making a popover that seems mostly like a traditional menu
<desrt> and it's also a very convenient upgrade path
<mclasen> we have them because we replace menus with popovers in a bunch of places: app-menu fallback, gears, ...
<desrt> sure.... so some popovers are menus
<desrt> but some are not
<owen> desrt: Having menu-model based popovers seems different than saying that you should be able to do everything with menu-model based popovers
<desrt> owen: i think we may be arguing the same point here...
<mclasen> sadly, a lot of the machinery to turn models into popovers (or menus) is locked away
<desrt> mclasen: i'm starting to lean toward a bit of a combined strategy here
<mclasen> recreating the stack-based popover submenus manually will be awkward
<desrt> i think expanding the power of menumodels to handle _some_ usecases is a good idea
<mclasen> maybe we need a popover subclass that has some of that built in
<desrt> the idea of having combined sections into buttons (with icons or text or whatever) seems like a great place to start, as an example
<desrt> maybe we even want to add a slider too
<desrt> but i see this being done in an "official" way -- ie: gtk provides the functionality
<desrt> if people want to break out of that mold and do more advanced things then they can -- via gtkbuilder -- and we should make sure that they have the ability to match us feature-for-feature when they decide to go their own way
<desrt> ie: look at what we need to do to open the muxer to external access
<mclasen> and look at button unification

Created attachment 274857 [details] [review]
Add support for scale widgets to model-based popovers

This allows to use scales for actions with double state in model-based
popovers. While the value for the scale is kept in the action, the
static properties such as minimum, maximum, step, marks, are provided
by attributes of the menuitem.
The testpopover testcase includes an example of this.

The last patch adds non-generic support for using scales for double-valued actions in popovers. It looks like this:

      <item>
        <attribute name="type">scale</attribute>
        <attribute name="action">top.zoom</attribute>
        <attribute name="minimum" type="d">0.0</attribute>
        <attribute name="maximum" type="d">10.0</attribute>
        <attribute name="step" type="d">1.0</attribute>
        <attribute name="marks" type="i">3</attribute>
      </item>

The implementation could be made a bit more elegant by turning this into a GtkModelScale subclass with properties for min, max, step and marks.

The type attribute is only needed because the code can't handle switching from role=normal to role=scale after widget construction.

No 'registration' system, yet.

I've pushed some improvements to the original popover-menu-buttons branch that make iconic display work for check and radio actions as well.

Some future enhancements that would be nice:

- put section title inside the row for iconic sections

- allow grouping the linked buttons via subsections

Created attachment 274893 [details]
buttons in action

Created attachment 274894 [details]
scale in action

Created attachment 275285 [details] [review]
GtkMenuTrackerItem: Add support for verb-icons

When rendering iconic sections, we want to use icons for verbs,
and we want to differentiate these in the menu model, to keep
the icon attribute reserved for nouns.

Created attachment 275286 [details] [review]
Add an iconic mode to model buttons

In iconic mode, model buttons will be styled like regular icon
buttons, preferring to show only the icon if one is set, falling
back to showing the label.

Created attachment 275287 [details] [review]
Add a display-hint to menu tracker sections and items

This section attribute will be used to indicate that iconic
rendering is preferred in the following commits.

Created attachment 275288 [details] [review]
GtkPopover: Use display-hint for menus

When an 'iconic' display-hint is specified on a section,
render the actions as a row of iconic model buttons, instead
of like menu items. We're using the verb-icon attribute
here, to keep noun and verb icons cleanly separated.

This works with regular actions, as well as with toggle and
radio actions.

Created attachment 275289 [details] [review]
Add an example of iconic rendering

testpopover now shows several examples of icon buttons.

Created attachment 275290 [details] [review]
Document iconic section support

Created attachment 275291 [details] [review]
Add support for scale widgets to model-based popovers

This allows to use scales for actions with double state in model-based
popovers. While the value for the scale is kept in the action, the
static properties such as minimum, maximum, step, marks, are provided
by attributes of the menuitem.

The testpopover testcase includes an example of this.

Review of attachment 275285 [details] [review]:

Looks good.

Review of attachment 275286 [details] [review]:

Looks good to me, in theory, but I am not the best person to review style-type stuff...

::: gtk/gtkmodelbutton.c
@@ +176,3 @@
+    {
+      gtk_style_context_remove_class (gtk_widget_get_style_context (GTK_WIDGET (button)),
+                                      GTK_STYLE_CLASS_MENUITEM);

Thanks for changing this.

support for icon buttons has landed; retitling for what is left

Comment on attachment 275287 [details] [review]
Add a display-hint to menu tracker sections and items

slightly different patch was committed

Comment on attachment 275288 [details] [review]
GtkPopover: Use display-hint for menus

different patch was committed

Created attachment 289227 [details] [review]
Add support for scale widgets to model-based popovers

This allows to use scales for actions with double state in model-based
popovers. While the value for the scale is kept in the action, the
static properties such as minimum, maximum, step, marks, are provided
by attributes of the menuitem.

The testpopover testcase includes an example of this.

I'm now favoring a different approach, that is developed on the popover-menu branch: Make GtkModelButton public, and add a GtkPopoverMenu class that has some infrastructure for menu-like popovers built-in, and allows arbitrary content.