Bug 16115 – [GTK] ContextMenu and ContextMenuItem lacks an implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Holger Freyther]

The classes ContextMenu and ContextMenuItem lack an implementation. These should be implemented using GtkMenu and GtkMenuItem but integrating the menu into the API needs discussions.

[Holger Freyther]

Created attachment 17507 [details]
ContextMenu Implementation WIP

Known issues or not yet checked:
  - Is the ref counting done right? Should we sink it? Should we just ref it?
  - We exploit the GObject to attach ContextMenuAction to a GtkMenuItem. We could keep them mapping in a hash map but would we store it in ContextMenu or ContextMenuItem.
  - How to map that into an API?
  - The code to activate the context menu by keyboard (in webkitgtkpage.cpp) is mostly taken from the Windows port. It would be nice to share some of the calculations to find the right element.
  - We need to morph a GtkMenuItem to a GtkCheckMenuItem on the fly. So it might make sense to not create a GtkMenuItem in ContextMenuItem until we need it or not at all. With the internal API we do not know what we are going to be.

Anyway with this patch you can right click and see a context menu.

[Christian Dywan]

Nice work, apparently you were faster than me with this one. :)

> - How to map that into an API?
The "populate-popup" seems to be a natural choice. It provides a pointer to a GtkMenu to append or prepend custom items. This of course only really makes sense once we have means of retrieving the element in question.

We probably want imageMenuItem's and make the edit items match those of a GtkEntry. And connect to "popup-menu" to show the menu via keyboard.

> - We need to morph a GtkMenuItem to a GtkCheckMenuItem on the fly. So it
might make sense to not create a GtkMenuItem in ContextMenuItem until we need
it or not at all. With the internal API we do not know what we are going to be.
On the fly creation of the items makes the most sense to me.

[Luca Bruno]

Created attachment 18153 [details]
updated patch

I just updated the patch for the new API.

[Alp Toker]

Comment on attachment 18153 [details]
updated patch

This didn't build when applied to SVN TOT. WebKit/gtk/WebCoreSupport/ContextMenuClientGtk.cpp needed some modifications to make it build.

After the modifications, I wasn't able to get a menu up. Is this patch meant to get that far?

[Luca Bruno]

Not really, it doesn't handle the mouse button. So i was just finding out a cleaner way of doing things. The purpose of this patch was to port the previous patch to the new API for now.

[Luca Bruno]

Created attachment 18186 [details]
done port of context menu

I think the port from the original patch is done.
The menu shows but it's really really buggy, the problem is might be caused by the wrong use of ref/unref on items.

[Luca Bruno]

Created attachment 18187 [details]
base of context menu

Sorry was my mistake.

[Alp Toker]

Comment on attachment 18187 [details]
base of context menu

r=me

ChangeLog entry next time please.

Will fix up one or two minor coding style issues eg. event.button()+1 and also the old terminology "gtkPage"

[Alp Toker]

Landed in r29080. There are a few FIXMEs and TODOs we'll have to look into.

[Holger Freyther]

Created attachment 18240 [details]
Create the GtkMenuItem on the fly.

This patch creates the GtkMenuItem on the fly. This allows to change the type, title and action easily. These values can be populated from an existing GtkMenuItem, e.g. when an item has been activated.

ContextMenuItem::setChecked is a pretty bad for the Gtk+ platform. We must know if an item is supposed to be checkable or not as we have GtkMenuItem and GtkCheckMenuItem and both get drawn differently. We should add another ActionType for checkable menuitems and make use of them.

[Christian Dywan]

Do you plan to also implement support for icons for appropriate menu items? The menus feel very alienated like this and icons would make a big difference. If you prefer I can just as well defer this to a new bug.

[Alp Toker]

Comment on attachment 18240 [details]
Create the GtkMenuItem on the fly.

r=me