[CLOSED] Linux native menus lost shortcuts

[core-ai-bot]

Issue by pelatx
Wednesday Apr 11, 2018 at 05:28 GMT
Originally opened as adobe#637

---

This PR makes the shortcuts to be shown again in the Linux native menus. Which were lost in my previous PR relative to the switchable menu entries.

A few shortcuts are still not shown. And honestly, at the moment I could not find the reason. Maybe someone can point out what I've missed.

The latter is more the reason for opening this PR than a merge in this state.

Thank you.

---

pelatx included the following code: https://github.com/adobe/brackets-shell/pull/637/commits

[core-ai-bot]

Comment by saurabh95
Wednesday Apr 11, 2018 at 12:46 GMT

---

Hey, @pelatx Could you please point to menu items for which shortcuts are not visible. It would be helpful for us in process of finding the reason for it.
Also, I would recommend that you install emmet extension and check for menu items under emmet menu. I used that extension only for my testing since it has a lot of menu items (checked + non-checked) when I worked on adding checked menu items.

[core-ai-bot]

Comment by pelatx
Thursday Apr 12, 2018 at 23:54 GMT

---

Sorry for the delay.

I've been doing a little more research and I've seen where the problem is coming from.

The menu entries that configure the shortcut when added to the menu ( in AddMenuItem()) are those that work as expected.
The problem occurs when this is not done in this way and the shortcut is then configured by calling SetMenuItemShortcut().

I have tried to solve it by setting the key in the model (model.setKey(tag, key)) within SetMenuItemShortcut(). And then recover it in SetMenuItemState() to pass it to ParseShortcut() within.

It does not work for now. I will keep trying.

PS: I do not know if I explained it well. 😅

[core-ai-bot]

Comment by saurabh95
Friday Apr 13, 2018 at 05:33 GMT

---

Thanks @pelatx, this will surely help us in finding the probable solution.

Your solution seems correct to me, not sure why it is not working. Did you check if all the keys are stored when using SetMenuItemShortcut , I mean could you please check by logging all the keys present in the model when it reaches SetMenuItemShortcut and verify if all the desired keys are present or not? This will give us an idea if we are updating the correct map.

PS: This is just a hunch, I have not tried this myself.

[core-ai-bot]

Comment by pelatx
Friday Apr 13, 2018 at 14:41 GMT

---

Right hunch, @saurabh95.

I do not know how I could look so many times at the SetMenuItemShortcut function without seeing the '&' missing in NativeMenuModel& model = ...

[core-ai-bot]

Comment by saurabh95
Saturday Apr 14, 2018 at 06:19 GMT

---

I also didn't notice that, glad you found it
The changeset looks good to me

[core-ai-bot]

Comment by saurabh95
Monday Apr 16, 2018 at 06:30 GMT

---

@nethip Could you please review the changes?
It is LGTM from my side.

[core-ai-bot]

Comment by nethip
Tuesday Apr 17, 2018 at 06:00 GMT

---

Thanks for the fix @pelatx! LGTM. Is it OK If I squash the commits and merge?

[core-ai-bot]

Comment by pelatx
Tuesday Apr 17, 2018 at 07:13 GMT

---

It is OK @nethip.

[core-ai-bot]

Comment by nethip
Tuesday Apr 17, 2018 at 07:52 GMT

---

Thanks @pelatx and thanks @saurabh95 for reviewing this as well.