Added support of Native Menus in Linux #602

saurabh95 · 2017-04-20T05:47:29Z

To test this global.brackets.nativeMenus in https://github.com/adobe/brackets/blob/master/src/utils/Global.js#L84 needs to be set to true.
// cc @abhijitapte for review.

@eyelash Could you also review the part added by me?

Fixes adobe/brackets#7236

Screenshot for Ubuntu 16.04

…ets-shell into ingo/linux-native-menus

eyelash · 2017-04-20T11:21:49Z

it's been 3 years since I wrote that code 😄

zaggino · 2017-04-24T01:09:39Z

sorry, can't review linux cpp code

eyelash

I don't have the time to set up a build of brackets-shell on my machine, so I just leave a few comments in the hope that they are useful.

eyelash · 2017-04-24T06:16:47Z

appshell/appshell_extensions_gtk.cpp

+            }
+        }
+        gchar* val = (gchar*)shortcut.c_str();
+        gtk_accelerator_parse (val, keyVal, modifier);


nit: everywhere else there is no space between function name and parameters.

eyelash · 2017-04-24T06:31:23Z

appshell/appshell_extensions_gtk.cpp

+        accel_group = gtk_accel_group_new();
+
+        std::vector<std::string> tokens;
+        std::istringstream f(key.c_str());


Is the .c_str() necessary here?

eyelash · 2017-04-24T06:33:02Z

appshell/appshell_extensions_gtk.cpp

+        std::istringstream f(key.c_str());
+        std::string s;
+        while (getline(f, s, '-')) {
+            tokens.push_back(s);


I'm wondering whether the tokens could be parsed here directly instead of copying them to a vector first and the parsing the vector.

Actually I have used tokens in creating shortcut of type A and also in the fallback, so I won't be able to have fallback if there is no tokens vector.

eyelash · 2017-04-24T06:39:52Z

appshell/appshell_extensions_gtk.cpp

+}
+
+
+int32 ParseShortcut(CefRefPtr<CefBrowser> browser, GtkWidget* entry, ExtensionString& key, GdkModifierType* modifier, guint* keyVal, ExtensionString& commandId) {


This function could probably be static if it is not used outside this file.

saurabh95 · 2017-04-24T07:37:27Z

Thanks @eyelash for reviewing this PR 😄
I have updated the PR as per your comments.

eyelash · 2017-04-24T08:31:30Z

@saurabh95 GetDisplayKeyString could probably be static as well. The rest looks fine to me. As I said, I only took a quick look at your code without compiling and I'm also no expert on GTK+ shortcuts.

abhijitapte · 2017-04-25T07:40:25Z

appshell/appshell_extensions.cpp

@@ -606,6 +606,9 @@ class ProcessMessageDelegate : public ClientHandler::ProcessMessageDelegate {
                bool enabled = argList->GetBool(2);
                bool checked = argList->GetBool(3);
                error = NativeMenuModel::getInstance(getMenuParent(browser)).setMenuItemState(command, enabled, checked);
+                if (error == NO_ERROR) {
+                    error = SetMenuItemState(browser, command, enabled, checked);


this seems to be common code (i.e. windows, mac and win).
Isn't the menu item state being set in line 608?

Actually NativeMenuModel::getInstance(getMenuParent(browser)).setMenuItemState(command, enabled, checked); does not work for Linux as it only sets a flag, so I have added SetMenuState function which is meant only for Linux and for case of Windows and MAC it is No Op

ficristo · 2017-04-26T08:29:29Z

I'm sorry, but I cannot review cpp code

abhijitapte · 2017-04-26T18:13:24Z

appshell/appshell_extensions_gtk.cpp

+        return -1;
+    else if (positionString == "first")
+        return 0;
+    else if (relativeId.size() > 0) {


use empty() instead

abhijitapte · 2017-04-26T18:15:34Z

appshell/appshell_extensions_gtk.cpp

+        if (iter == NULL)
+            position = -1;
+        else if (positionString == "before")
+            ;


can't you instead set position as 0 here.

We can't set position to 0 here, as in https://github.com/adobe/brackets-shell/pull/602/files#diff-10aa13d798996c2d964d38b1f328ae62R698 we are incrementing value of position, so setting it to 0 will always add the menu in the first index, which is not desired.

abhijitapte

please address the comments.

abhijitapte · 2017-05-02T13:27:24Z

appshell/appshell_extensions_gtk.cpp

+        }
+        else
+            position = -1;
+        g_list_free(children);


can you point to the documentation in GTK where they specify that you need to explicitly free it?

https://developer.gnome.org/glib/stable/glib-Doubly-Linked-Lists.html#g-list-free

also https://developer.gnome.org/gtk3/stable/GtkContainer.html#gtk-container-get-children says:

Returns a newly-allocated list of the container’s non-internal children.

and the tooltip of [transfer-container] says Free data container after the code is done.

Just checked the tooltip indeed conveys that we need to free.
I looked at documentation of https://developer.gnome.org/gtk3/stable/GtkContainer.html#gtk_container_get_focus_chain and noticed the explicit free mentioned there.

abhijitapte · 2017-05-02T13:27:46Z

appshell/appshell_extensions_gtk.cpp

 }

 int32 AddMenu(CefRefPtr<CefBrowser> browser, ExtensionString title, ExtensionString command,
-              ExtensionString position, ExtensionString relativeId)
+              ExtensionString positionString, ExtensionString relativeId)


why can't these parameters be passed by const ref?

These are actually implementation of interface which is used by all platforms, so changing this would mean changing in all the platforms implementation.
We can take this for later PR.

abhijitapte · 2017-05-02T13:29:31Z

appshell/appshell_extensions_gtk.cpp

+        std::string shortcut = "";
+        // convert shortcut format
+        // e.g. Ctrl+A converts to <Ctrl>A whose entry is stored in accelerator table
+        for (int i=0;i<tokens.size();i++) {


isn't it better to take the size() in a local and use that instead?

abhijitapte · 2017-05-02T13:30:04Z

appshell/appshell_extensions_gtk.cpp

+                shortcut += "<";
+            }
+            shortcut += tokens[i];
+            if (i != tokens.size() - 1) {


why is the last token excluded?

Actually in this we are changing the token string from Ctrl+A to A whose entry is stored in accelerator table, so for the last token there is no need to enclose it in <>

abhijitapte · 2017-05-02T13:30:53Z

appshell/appshell_extensions_gtk.cpp

+                    modifier = GdkModifierType(modifier | GDK_SHIFT_MASK);
+                } else if (tokens[i] == "Enter") {
+                    keyVal = GDK_KEY_KP_Enter;
+                } else if (tokens[i] == "Up" || tokens[i] == "\u2191") {


It is better to use macros or constants instead of these string literals

swmitra · 2017-05-03T06:18:29Z

Great work @saurabh95 👍

eyelash · 2017-07-04T11:13:08Z

I don't know if that was intentional but while merging this work to master all the commits were squashed (aa03360). Just to give you guys some feedback for the future: Seeing all your work merged without getting credit for it is not very motivating for a community contributor like me.

saurabh95 · 2017-07-04T11:36:50Z

@eyelash it was by mistake and we will be having Release Notes in which we highlight top features and their authors and we will have your name in that.
We will definitely take care of this from next time.

eyelash · 2017-07-04T13:10:43Z

@saurabh95 Thank you, I'm glad to hear that.

eyelash and others added 7 commits May 18, 2014 09:56

fix a memory leak

221b330

linux-native-menus: implement separators

2216c01

linux-native-menus: use the native menu model

83f3748

linux-native-menus: implement the callback

c47819f

linux-native-menus: position the items correctly

04f56c7

linux-native-menus: implement some more functions

6c09c32

Merge branch 'linux-native-menus' of https://github.com/eyelash/brack…

354c10e

…ets-shell into ingo/linux-native-menus

saurabh95 requested review from ingorichter, zaggino, ficristo, swmitra, nethip and marcelgerber and removed request for marcelgerber April 20, 2017 05:47

Added support of native menu shortcuts in Linux

2f15dfd

zaggino removed their request for review April 24, 2017 01:09

eyelash reviewed Apr 24, 2017

View reviewed changes

saurabh95 added 2 commits April 24, 2017 18:35

Addressed review comments

ff6abd4

Added functionality of enabling and disabling menu items

0f67000

abhijitapte reviewed Apr 25, 2017

View reviewed changes

Code Cleanup

98ccf5b

saurabh95 mentioned this pull request Apr 26, 2017

Changed Global.js to enable Native Menus for Linux adobe/brackets#13339

Merged

ficristo removed their request for review April 26, 2017 08:29

abhijitapte reviewed Apr 26, 2017

View reviewed changes

Addressed review comment

1524d7b

abhijitapte suggested changes May 2, 2017

View reviewed changes

Addressed review comments

0ecc5c8

abhijitapte approved these changes May 3, 2017

View reviewed changes

swmitra approved these changes May 3, 2017

View reviewed changes

swmitra merged commit 0f813d8 into linux-1547 May 3, 2017

swmitra deleted the ingo/linux-native-menus branch May 3, 2017 06:18

saurabh95 mentioned this pull request May 25, 2017

add a function that gets called when the menu item state was changed #447

Closed

core-ai-bot mentioned this pull request Aug 17, 2021

[CLOSED] add a function that gets called when the menu item state was changed brackets-archive/brackets-shell#292

Open

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Changed Global.js to enable Native Menus for Linux brackets-archive/bracketsIssues#11293

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support of Native Menus in Linux #602

Added support of Native Menus in Linux #602

saurabh95 commented Apr 20, 2017 •

edited

Loading

eyelash commented Apr 20, 2017

zaggino commented Apr 24, 2017

eyelash left a comment •

edited

Loading

eyelash Apr 24, 2017

eyelash Apr 24, 2017

eyelash Apr 24, 2017

saurabh95 Apr 24, 2017

eyelash Apr 24, 2017

saurabh95 commented Apr 24, 2017

eyelash commented Apr 24, 2017

abhijitapte Apr 25, 2017

saurabh95 Apr 25, 2017

ficristo commented Apr 26, 2017

abhijitapte Apr 26, 2017

abhijitapte Apr 26, 2017

saurabh95 Apr 27, 2017

abhijitapte left a comment

abhijitapte May 2, 2017

saurabh95 May 2, 2017

eyelash May 2, 2017 •

edited

Loading

abhijitapte May 3, 2017

abhijitapte May 2, 2017

saurabh95 May 2, 2017

abhijitapte May 2, 2017

abhijitapte May 2, 2017

saurabh95 May 2, 2017

abhijitapte May 2, 2017

swmitra commented May 3, 2017

eyelash commented Jul 4, 2017

saurabh95 commented Jul 4, 2017

eyelash commented Jul 4, 2017

		}


		int32 ParseShortcut(CefRefPtr<CefBrowser> browser, GtkWidget* entry, ExtensionString& key, GdkModifierType* modifier, guint* keyVal, ExtensionString& commandId) {

Added support of Native Menus in Linux #602

Added support of Native Menus in Linux #602

Conversation

saurabh95 commented Apr 20, 2017 • edited Loading

eyelash commented Apr 20, 2017

zaggino commented Apr 24, 2017

eyelash left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabh95 commented Apr 24, 2017

eyelash commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ficristo commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhijitapte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyelash May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swmitra commented May 3, 2017

eyelash commented Jul 4, 2017

saurabh95 commented Jul 4, 2017

eyelash commented Jul 4, 2017

saurabh95 commented Apr 20, 2017 •

edited

Loading

eyelash left a comment •

edited

Loading

eyelash May 2, 2017 •

edited

Loading