GNOME Bugzilla – Bug 606831
Devhelp applications lacks right-click context menu
Last modified: 2017-12-14 17:56:18 UTC
In devhelp, there is no right-click context menu - right clicking produces no effect. It would be nice if you could add a right-click context menu with some common options like "Copy" and "Select All" since they are present in other documentation browsers. Thank you. Steps to reproduce: 1. Open devhelp 0.23. 2. Open any book. 3. Open any page. 4. Right click on the right pane.
This bug was also reported by bjd on Launchpad in Ubuntu at <https://launchpad.net/bugs/512467>. I can confirm there is still no context menu up to at least release 2.29.2.
Created attachment 179487 [details] [review] Creates a right-click popup menu
*** Bug 637786 has been marked as a duplicate of this bug. ***
*** Bug 664081 has been marked as a duplicate of this bug. ***
Review of attachment 179487 [details] [review]: Devhelp was recently ported to use GtkBuilder; the patch would need to get updated :-/
Created attachment 228227 [details] [review] Added popup via right click on web-view, added function "Select all" for popup. Added popup with items: - Copy - Select all - Back - Forward - Find - New tab
Oh. I later saw that this bug for version 3.3. The last patch is applied to version 3.6.0
Not sure if all those items are really needed in the popup menu. I personally would remove at least 'Back', 'Forward' and 'New tab'... what do others think?
Aleksander Morgado, I agree with you. I added these items because it see questions about the "Back" and "Forward" also. But without "Copy" and "Select all" is very not comfortable.
Created attachment 228259 [details] [review] Popup with "Copy", "Select all", "Find" and "New tab" I remove menu items "Back" and "Forward". For the function "New tab" added to open the link under the cursor in a new tab.
Review of attachment 228259 [details] [review]: hmm, this was a lot easier when devhelp used GtkActions. ::: src/dh-window.c @@ +608,3 @@ + window_open_new_tab (window, location, (location != NULL)); +} +#endif Why are these specific to WebKit1? @@ +1791,3 @@ + g_signal_connect (view, "hovering-over-link", + G_CALLBACK (window_web_view_hovering_over_link_cb), + window); Instead of connecting to hovering-over-link to know whether you are over a link, you can simply connect to context-menu signal that provides a WebKitHitTestResult. The signal is emitted when webkit is about to show a context menu, so you can connect to the signal, use the hit test result, build your own context menu and then return TRUE so that webkit doesn't try to show its own context menu. See: http://www.webkitgtk.org/reference/webkitgtk/stable/webkitgtk-webkitwebview.html#WebKitWebView-context-menu And for webkit2: http://www.webkitgtk.org/reference/webkit2gtk/unstable/WebKitWebView.html#WebKitWebView-context-menu
First of all: thanks for working on this. That said, I'd strongly suggest having these two context menus at the *top* of the context menu: * Back * Forward This is essential for quick navigation through documentation without having to move the mouse all over the screen. This is also the what the Gnome web browser (Web/Epiphany) does. Furthermore, the "open new tab" should read "Open in new tab", since that is what it does. (Again, this is also what Web/Epiphany does.) "Select all" and "Find" do not really make sense to me. "Copy" when text is selected would be much more useful than "Select all". "Find" requires additional keyboard input, so I don't really see why that should be in a context menu.
Thank you for what you have paid attention to this problem. Carlos Garcia Campos, Thank you for the tip. I found the hard way. I redid it. > That said, I'd strongly suggest having these two context menus at the *top* of > the context menu: > > * Back > * Forward > > This is essential for quick navigation through documentation without having to > move the mouse all over the screen. This is also the what the Gnome web browser > (Web/Epiphany) does. > I got this situation as person which usually use through google-translate. But ok, I move "Back" and "Forward" to top. No problem > Furthermore, the "open new tab" should read "Open in new tab", since that is > what it does. (Again, this is also what Web/Epiphany does.) I did not renamed this item from the next reason: if choose it without a link under pointer then be opened is empty tab. In this case the label "Open in tab" is not actual. The "Open in tab" is actual when have a link under pointer. > "Select all" and "Find" do not really make sense to me. "Copy" when text is > selected would be much more useful than "Select all". "Find" requires > additional keyboard input, so I don't really see why that should be in a > context menu. > "Select all" may not be as important, but it started this bug report. "Find" - this is my initiative and I see no problem to remove this item. I propose the next an order of the menu: - Back (visible just if can go back) - Forward (visible just if can go forward) - New tab (visible allways) - separator (visible allways) - Copy (visible just if have selected text) - Select all (visible allways) What say you, gentlemen?
Created attachment 228343 [details] [review] Context menu Changed the menu. All menu items is context dependent. Now we have next: - "Back" (hidden if no back-history) - "Forward" (hidden if no forward-history) - "Copy" (hidden if no selections) - "Open in tab" (hidden if no URI) - "New tab" (visible if no URI) "Select all", I cut. Label "Open in tab" is not translated. Thank you!
Review of attachment 228343 [details] [review]: Looks much better, thanks! I still think this is a lot easier using GtkUIManager and GtkAction. With gtk actions you can make them visible/invisible (and sensitive/unsensitive) without having to manually iterate the list of menu items. ::: src/dh-window.c @@ +897,3 @@ + window); + } else { + item = gtk_separator_menu_item_new (); This allows to add separators, but you aren't adding any separator to the menu. @@ +912,3 @@ + append_popup_item (window, menu, GTK_STOCK_GO_FORWARD, "go-forward", TRUE); + append_popup_item (window, menu, GTK_STOCK_COPY, "copy", TRUE); + append_popup_item (window, menu, _("Open in tab"), "open-in-tab", FALSE); "Open link in new tab", maybe? @@ +914,3 @@ + append_popup_item (window, menu, _("Open in tab"), "open-in-tab", FALSE); + append_popup_item (window, menu, _("New _Tab"), "new-tab", FALSE); + gtk_widget_show_all (menu); Instead of showing all the menu when created, call gtk_widget_show for every item, you are creating the menu in the window init method, we don't want the menu to be visible at that point. @@ +1698,3 @@ + WebKitHitTestResultContext context; + + g_object_get (G_OBJECT (hit_test_result), g_object_get expects a gpointer, so no need to cast @@ +1703,3 @@ + "context", + &context, + NULL); indentation looks wrong here. @@ +1706,3 @@ + + if (location != NULL && g_str_has_prefix (location, "file://")) { + g_object_set_data_full (G_OBJECT (web_view), Instead of setting this in the view, since we are keeping the popup menu, mayb it makes more sense to attach this data to the menu, or even keep it in window private struct, that would be easier than using g_object_set_data @@ +1722,3 @@ + can_forward = webkit_web_view_can_go_forward (web_view); +#ifndef HAVE_WEBKIT2 + has_selection = ((context & WEBKIT_HIT_TEST_RESULT_CONTEXT_SELECTION) != 0); hmm, we need to add this to WebKit2 API. @@ +1726,3 @@ + + popup_set_item_visible (window, "go-back", can_back); + popup_set_item_visible (window, "go-forward", can_forward); I would be weird if only back or forward action are in the menu, in this case, maybe it would be better to make them insensitive instead of invisible. @@ +1737,3 @@ + NULL, + 3, + gtk_get_current_event_time ()); This doesn't work for webkit2 where the event has happened asynchronously, gtk_get_current_event will be NULL. Use the event passed to the callback to get the even time.
> This doesn't work for webkit2 where the event has happened asynchronously, > gtk_get_current_event will be NULL. Use the event passed to the callback to get > the even time. Of course! @@ -1570,6 +1674,73 @@ window_web_view_tab_accel_cb (GtkAccelGr } } +#ifdef HAVE_WEBKIT2 +static gboolean +window_web_view_context_menu_cb (WebKitWebView *web_view, + WebKitContextMenu *context_menu, +===>> GdkEvent *event, + WebKitHitTestResult *hit_test_result, + DhWindow *window) +#else The (GdkEvent) event->time also will be NULL? > @@ +1722,3 @@ > + can_forward = webkit_web_view_can_go_forward (web_view); > +#ifndef HAVE_WEBKIT2 > + has_selection = ((context & WEBKIT_HIT_TEST_RESULT_CONTEXT_SELECTION) > != 0); > > hmm, we need to add this to WebKit2 API. Yes Add it to API of WebKit2. Also may be have reason, for add webkit_wev_view_has_selection or etc. ? > @@ +1706,3 @@ > + > + if (location != NULL && g_str_has_prefix (location, "file://")) { > + g_object_set_data_full (G_OBJECT (web_view), > > Instead of setting this in the view, since we are keeping the popup menu, mayb > it makes more sense to attach this data to the menu, or even keep it in window > private struct, that would be easier than using g_object_set_data Of course! Private structure - this is the place where we need. May be I need the create "finalize" function to freeing this data? And it is just important, in object what I save the link? > @@ +1703,3 @@ > + "context", > + &context, > + NULL); > > indentation looks wrong here. oh, yeah! Indentations of the 8 speces are normally, but it is "wrong indentation" > @@ +914,3 @@ > + append_popup_item (window, menu, _("Open in tab"), "open-in-tab", > FALSE); > + append_popup_item (window, menu, _("New _Tab"), "new-tab", FALSE); > + gtk_widget_show_all (menu); > > Instead of showing all the menu when created, call gtk_widget_show for every > item, you are creating the menu in the window init method, we don't want the > menu to be visible at that point. It is the most terrible thing which can be happened. It is more terrible thing than which simple situation which does not have the menu and not working shortcut "Ctrl+C". > @@ +912,3 @@ > + append_popup_item (window, menu, GTK_STOCK_GO_FORWARD, "go-forward", > TRUE); > + append_popup_item (window, menu, GTK_STOCK_COPY, "copy", TRUE); > + append_popup_item (window, menu, _("Open in tab"), "open-in-tab", > FALSE); > > "Open link in new tab", maybe? Maybe... Maybe... Also "Open" simply or doing without this item... Or cut this patch completely, as variation. > ::: src/dh-window.c > @@ +897,3 @@ > + window); > + } else { > + item = gtk_separator_menu_item_new (); > > This allows to add separators, but you aren't adding any separator to the > menu. Yes. I do not add separator. You wrote "I would be weird if only back or forward action are in the menu,". And no less strange for finding at the beginning of the menu of separator? > I still think this is a lot easier using > GtkUIManager and GtkAction. With gtk actions you can make them > visible/invisible (and sensitive/unsensitive) without having to manually > iterate the list of menu items. Sorry, but the UIManager and GtkAction will cut from DevHelp since before and return to UIManager and GtkAction, when DevHelp used GAction, (IMHO) litle bit strange. But the UIManager and GtkAction can not decide problem with manual control of the visiblity items of menu. I feel that I am student who have mistake for homework and teacher get angry to me.
(In reply to comment #16) > I feel that I am student who have mistake for homework and teacher get angry to > me. No, please don't! It's really great that you're working on this, and its coming along nicely from the progress I see in this report. Please keep going; this feature request is finally getting somewhere, and it's all because of your work!
(In reply to comment #16) > > This doesn't work for webkit2 where the event has happened asynchronously, > > gtk_get_current_event will be NULL. Use the event passed to the callback to get > > the even time. > > Of course! > > @@ -1570,6 +1674,73 @@ window_web_view_tab_accel_cb (GtkAccelGr > } > } > > +#ifdef HAVE_WEBKIT2 > +static gboolean > +window_web_view_context_menu_cb (WebKitWebView *web_view, > + WebKitContextMenu *context_menu, > +===>> GdkEvent *event, > + WebKitHitTestResult *hit_test_result, > + DhWindow *window) > +#else > > The (GdkEvent) event->time also will be NULL? No, it won't be NULL, that's what you have to use. > > > @@ +1722,3 @@ > > + can_forward = webkit_web_view_can_go_forward (web_view); > > +#ifndef HAVE_WEBKIT2 > > + has_selection = ((context & WEBKIT_HIT_TEST_RESULT_CONTEXT_SELECTION) > > != 0); > > > > hmm, we need to add this to WebKit2 API. > > Yes Add it to API of WebKit2. Also may be have reason, for add > webkit_wev_view_has_selection or etc. ? There's API to know when you can cut or copy that could be used to know whether ther's something selected webkit_web_view_can_execute_editing_command(). > > > @@ +1706,3 @@ > > + > > + if (location != NULL && g_str_has_prefix (location, "file://")) { > > + g_object_set_data_full (G_OBJECT (web_view), > > > > Instead of setting this in the view, since we are keeping the popup menu, mayb > > it makes more sense to attach this data to the menu, or even keep it in window > > private struct, that would be easier than using g_object_set_data > > Of course! Private structure - this is the place where we need. May be I need > the create "finalize" function to freeing this data? Yes, or you can connect to deactivate signal of the menu and free the associated data when it is dismissed. > And it is just important, > in object what I save the link? Not really, it was just a suggestion, it's data associated to the popup so it looks natural to attach the data to the popup object. > > > @@ +1703,3 @@ > > + "context", > > + &context, > > + NULL); > > > > indentation looks wrong here. > > oh, yeah! Indentations of the 8 speces are normally, but it is "wrong > indentation" It's wrong because it's not consistent with the coding style of the file. > > > @@ +914,3 @@ > > + append_popup_item (window, menu, _("Open in tab"), "open-in-tab", > > FALSE); > > + append_popup_item (window, menu, _("New _Tab"), "new-tab", FALSE); > > + gtk_widget_show_all (menu); > > > > Instead of showing all the menu when created, call gtk_widget_show for every > > item, you are creating the menu in the window init method, we don't want the > > menu to be visible at that point. > > It is the most terrible thing which can be happened. It is more terrible thing > than which simple situation which does not have the menu and not working > shortcut "Ctrl+C". I don't get this comment. > > @@ +912,3 @@ > > + append_popup_item (window, menu, GTK_STOCK_GO_FORWARD, "go-forward", > > TRUE); > > + append_popup_item (window, menu, GTK_STOCK_COPY, "copy", TRUE); > > + append_popup_item (window, menu, _("Open in tab"), "open-in-tab", > > FALSE); > > > > "Open link in new tab", maybe? > > Maybe... Maybe... Also "Open" simply or doing without this item... Or cut this > patch completely, as variation. It was just a suggestion to make it clear a new tab will be opened. > > ::: src/dh-window.c > > @@ +897,3 @@ > > + window); > > + } else { > > + item = gtk_separator_menu_item_new (); > > > > This allows to add separators, but you aren't adding any separator to the > > menu. > > Yes. I do not add separator. You wrote "I would be weird if only back or > forward action are in the menu,". And no less strange for finding at the > beginning of the menu of separator? Yes, indeed, GtkUIManager implements "smart separators" to deal with these cases, as well as the WebKitContextMenu API available in WebKit2. > > I still think this is a lot easier using > > GtkUIManager and GtkAction. With gtk actions you can make them > > visible/invisible (and sensitive/unsensitive) without having to manually > > iterate the list of menu items. > > Sorry, but the UIManager and GtkAction will cut from DevHelp since before and > return to UIManager and GtkAction, when DevHelp used GAction, (IMHO) litle bit > strange. I'm just saying it's unfortunate devhelp doesn't use GtkActions anymore, we could have a UI manager only for the popup though. > But the UIManager and GtkAction can not decide problem with manual > control of the visiblity items of menu. > I feel that I am student who have mistake for homework and teacher get angry to > me. Not at all, this is the normal process of a patch review in a free software project. Note that I'm not even the maintainer of the project, so I'm just suggesting changes to make it easier for the maintainer to review the patch and for you to receive early feedback. I really appreciate your work on this bug and I encourage you to continue improving the patch. I'm sorry if I've made you feel I'm angry, I'm indeed happy to see there's progress in this.
As the (not very active) maintainer I really appreciate what's happening here, code & reviews, please go on, it will definitively go in.
Sorry guys. if I was a bit rude. I like that. what you do in Gtk / Gnome / etc .. But the comment from Carlos I am took it not as a discussion. This is emotions. Sorry. So. Why is not "Open link in new tab"? It is very long label. Yes. It's intuitive. but even the "New tab" in some languages look very long. See PO- files. > No, it won't be NULL, that's what you have to use. Now it works even just from 0 to time. All right. I will add to WebKit2 separately using GdkEvent. This is no problem. > Yes, indeed, GtkUIManager implements "smart separators" to deal with these > cases, as well as the WebKitContextMenu API available in WebKit2. I looked at it and wanted to use the API, but I thought it was a bit unreliable for our purposes. > Not really, it was just a suggestion, it's data associated to the popup so it > looks natural to attach the data to the popup object. I added a new field to private structure for reference, also considering the option of storing it in a popup. But at that moment I had the idea that the link may be useful for other purposes, separately for each page. > I don't get this comment. At this point I did not remove gtk_widget_show_all for the same reason. that not removed separators - all work is still in the process and do not know what will be in the result. And while I did not make the context menu, I had to have visible all menu items, but without calling gtk_widget_show_all we get is a blank menu. In the form, as it is now, call gtk_widget_show_all irrelevant. How do you feel about that change the binding for the tab "Contents", to resolve the conflict between the "Copy" and "Contents"? I understand. this is another bug and that this change will need to fix it and PO-files. --- a/data/ui/devhelp.builder 2012-11-06 14:13:21.615786140 +0900 +++ b/data/ui/devhelp.builder 2012-11-09 05:26:40.665897361 +0900 @@ -151,7 +151,7 @@ <attribute name="accel"><Primary>s</attribute> </item> <item> - <attribute name="label" translatable="yes">_Contents Tab</attribute> + <attribute name="label" translatable="yes">Contents Tab</attribute> <attribute name="action">win.go-contents-tab</attribute> <attribute name="accel"><Primary>c</attribute> </item>
Created attachment 228581 [details] [review] Context menu WebKit2 built on two platforms and checked this patch. Carlos was right about GdkEvent. This is fixed. Not convenient that WebKit2 API is not possible to get the status of the selection.
Fixed with commit c4eb83815663844f416f49d84e37e561a1bb0e4b. WebKitWebView provides a default context menu.