Bug 16115 - [GTK] ContextMenu and ContextMenuItem lacks an implementation
Summary: [GTK] ContextMenu and ContextMenuItem lacks an implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2007-11-24 04:47 PST by Holger Freyther
Modified: 2008-01-03 03:28 PST (History)
3 users (show)

See Also:


Attachments
ContextMenu Implementation WIP (14.51 KB, patch)
2007-11-25 08:39 PST, Holger Freyther
no flags Details | Formatted Diff | Diff
updated patch (12.81 KB, patch)
2007-12-28 14:25 PST, Luca Bruno
alp: review-
Details | Formatted Diff | Diff
done port of context menu (13.61 KB, patch)
2007-12-30 02:30 PST, Luca Bruno
no flags Details | Formatted Diff | Diff
base of context menu (13.67 KB, patch)
2007-12-30 02:49 PST, Luca Bruno
alp: review+
Details | Formatted Diff | Diff
Create the GtkMenuItem on the fly. (8.94 KB, patch)
2008-01-02 09:28 PST, Holger Freyther
alp: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Holger Freyther 2007-11-24 04:47:38 PST
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.
Comment 1 Holger Freyther 2007-11-25 08:39:06 PST
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.
Comment 2 Christian Dywan 2007-11-25 11:45:26 PST
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.
Comment 3 Luca Bruno 2007-12-28 14:25:50 PST
Created attachment 18153 [details]
updated patch

I just updated the patch for the new API.
Comment 4 Alp Toker 2007-12-28 14:56:55 PST
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?
Comment 5 Luca Bruno 2007-12-29 01:24:22 PST
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.
Comment 6 Luca Bruno 2007-12-30 02:30:41 PST
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.
Comment 7 Luca Bruno 2007-12-30 02:49:12 PST
Created attachment 18187 [details]
base of context menu

Sorry was my mistake.
Comment 8 Alp Toker 2008-01-02 02:16:24 PST
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"
Comment 9 Alp Toker 2008-01-02 02:26:25 PST
Landed in r29080. There are a few FIXMEs and TODOs we'll have to look into.
Comment 10 Holger Freyther 2008-01-02 09:28:47 PST
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.
Comment 11 Christian Dywan 2008-01-02 13:37:40 PST
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.
Comment 12 Alp Toker 2008-01-03 03:28:12 PST
Comment on attachment 18240 [details]
Create the GtkMenuItem on the fly.

r=me