search

brackets-archive / brackets-shell

CEF3-based application shell for Brackets.
http://brackets.io
MIT License
0 stars 0 forks source link

[CLOSED] Linux native menus lost shortcuts #418

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by pelatx Wednesday Apr 11, 2018 at 05:28 GMT Originally opened as https://github.com/adobe/brackets-shell/pull/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 commented 3 years ago

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 commented 3 years ago

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. :sweat_smile:

core-ai-bot commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

It is OK @nethip.

core-ai-bot commented 3 years ago

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

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