Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maybe also affected by this issue (see description)? #7

Open
dinkumoil opened this issue Oct 18, 2018 · 6 comments
Open

Maybe also affected by this issue (see description)? #7

dinkumoil opened this issue Oct 18, 2018 · 6 comments
Labels
enhancement New feature or request

Comments

@dinkumoil
Copy link

The Pythonscript plugin had an issue with menu entries created by the plugin itself, see here. This issue was eliminated by introducing a new Npp event, see here. This event is available since Npp v7.5.9.

I guess NppExec could also be affected.

@dinkumoil dinkumoil changed the title Maybe also affected by this issue? Maybe also affected by this issue (see description)? Oct 18, 2018
@d0vgan
Copy link
Owner

d0vgan commented Oct 18, 2018

Yes, NppExec is also affected by this.
Indeed, the NPPM_REMOVESHORTCUTBYCMDID needs to be used.

@d0vgan
Copy link
Owner

d0vgan commented Oct 30, 2018

Well, an internal array of menu item ids for NppExec's scripts can be introduced, and the menu items listed in NppExec's Advanced Options can internally be associated with the menu item ids.
It will help in case of menu item deletion.
In case of menu items moving (up or down), however, I have no idea what to do. It does not look like NPPM_REMOVESHORTCUTBYCMDID helps here.
In general, all this situation does not look good to me. Currently, a plugin's internal logic does not deal with (and had no knowledge of) its menu item ids, so NPPM_REMOVESHORTCUTBYCMDID or similar is not an obvious way anyway.

@chcg
Copy link
Contributor

chcg commented Oct 30, 2018

@d0vgan Are you talking about to modify the shortcut from within a nppexec script?
This is not done by PS, the user has to do it manually under Settings->Shortcut mapper -> Plugin commands. And a new entry is just available after a N++ restart.

For user changes via the n++ menu there is NPPN_SHORTCUTREMAPPED, which is already implemented by your plugin as far as I see.

Pythonscript could remove menu items calling python scripts in a setting menu and if a shortcut is assigned to that menu item it is also removed automatically.

@d0vgan
Copy link
Owner

d0vgan commented Oct 30, 2018

NppExec's Advanced Options additionally contains the following two buttons: [Move up] and [Move down].
Now imagine that the menu items being moved (one of them is moved down and another one is moved up) already have their shortcut keys. From NppExec's perspective, absolutely nothing is changed with regard to shortcut keys (because NppExec itself is just not aware of shortcut keys) of the corresponding menu items. However, from Notepad++'es perspective, the two menu items are switched whereas their corresponding shortcut keys are still mapped to the old menu item positions, not to items themselves. Saying the same in other words, this leads to the shortcut keys to become switched. Let me "illustrate" this:

Initial Position:
Menu Item A - Shortcut A
Menu Item B - Shortcut B

Position after moving the items:
Menu Item B - Shortcut A
Menu Item A - Shortcut B

And I have no idea how this can be fixed, because NPPM_REMOVESHORTCUTBYCMDID does not allow to deal with it - because the item is moved (its position is changed) rather than deleted.
That is why I'm saying that NPPM_REMOVESHORTCUTBYCMDID is not enough to deal with it and, moreover, it adds additional complication to plugins because now the plugin's code must be aware of the menu items positions (created by the plugin) and their associated shortcut keys. Currently NppExec does neither of both: it does not store the association between the scripts and their corresponding menu item positions, as well as it does not store the association between the scripts and their shortcut keys for the corresponding menu items.

@d0vgan d0vgan added the enhancement New feature or request label Oct 30, 2018
@chcg
Copy link
Contributor

chcg commented Oct 30, 2018

NPPM_REMOVESHORTCUTBYCMDID can't help with that. You would need a new callback to modify the shortcut <-> menu item mapping at n++ shortcut code.

@d0vgan
Copy link
Owner

d0vgan commented Nov 7, 2018

I was thinking about this issue again and again, and here is my conclusion: the proposed approach with NPPM_REMOVESHORTCUTBYCMDID is a completely wrong direction.
Here is why.
The source of a problem is not in plugins, it's in Notepad++ itself. Here is what I mean: the origin, the cause of the problem comes from the way how Notepad++ stores shortcuts for the plugins. Notepad++ maps the shortcut keys to the value of internalID that is not e.g. a unique hash of plugin's menu item name, but something like a menu item's position. Obviously, when this position is changed, the mappings between these positions and shortcuts becomes wrong.
Now I'm asking a question: is it a plugin's fault? What if Notepad++ stored some unique hash keys to identify plugin's menu items disregarding the menu items' positions, would we face this problem?
And, by introducing NPPM_REMOVESHORTCUTBYCMDID , aren't we in fact trying to fix Notepad++'es internal issue in an external plugin's code? Isn't it strange?
Moreover, as it was discussed here, anyway some different fix is needed to deal with moving (not deleting) of menu items.
So my proposal is to fix it in Notepad++ itself: do it once, and for all the plugins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants