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

fix: Bookmark comment shortcuts (issue #2055) #2056

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

paxcut
Copy link
Contributor

@paxcut paxcut commented Jan 6, 2025

Fixes for #2055.

Added the registrations for the editor shortcuts and reorganized the registration calls into events and handlers.

Needed a way to select which of the possibly multiple editors sees the effects of the keyboard so a pointer to the current text editor in use was added to the view bookmarks class. The pointer may be empty so the shortcuts need to check it before using it.

To detect which of the editors needs to see the effects of the shortcuts a function ws added to text editor that tells if window has focus.

To avoid errors when editors become read only, asserts were exchanged with do-nothing early returns on the text editor.

To avoid using the default language coloring (HLSL), the colorize enabling variable was set to false.

A suggestion by cppcheck simplified the logic of a conditional on view bookmarks.

On view pattern editor some icons were added to console menu entries.

paxcut and others added 2 commits January 6, 2025 08:48
Added the registrations for the editor shortcuts and reorganized the registration calls into events and handlers.

Needed a way to select which of the possibly multiple editors sees the effects of the keyboard so a pointer to the current text editor in use was added to the view bookmarks class. The pointer may be empty so the shortcuts need to check it before using it.

To detect which of the editors needs to see the effects of the shortcuts a function ws added to text editor that tells if window has focus.

To avoid errors when editors become read only, asserts were exchanged with do-nothing early returns on the text editor.

To avoid using the default language coloring (HLSL), the colorize enabling variable was set to false.

A suggestion by cppcheck simplified the logic of a conditional on view bookmarks.

On view pattern editor some icons were added to console menu entries.
@WerWolv
Copy link
Owner

WerWolv commented Jan 9, 2025

I don't think duplicating these shortcut definitions everywhere where the text editor is used is a good idea... Maybe it would make sense to add a new helper class to the ui plugin that just wraps the Text Editor with the things we need and adds the shortcuts there. That way we can just use that new class everywhere and have the shortcuts work and affect the currently focused editor

@paxcut
Copy link
Contributor Author

paxcut commented Jan 10, 2025

Putting the burden of identifying who is calling it on the shortcut seems wrong. In the simplest cases you want basic navigation only like the console. Rather than a class that merges the shortcuts with the editor it may make more sense to abstract the concept of a shortcut even further to something like copySortcut, pasteSortCut, cutShortcut, etc... that can be called from each editor user without having to specify the keys. That way you don't duplicate as much code while leaving the burden of identification to the class that using the editor like it currently does.

@paxcut
Copy link
Contributor Author

paxcut commented Jan 11, 2025

How does the hex editor define its shortcuts in ViewHexEditor when it is used outside the main hex editor (like in sections or the hex visualizer)? Is it possible to so something similar so that the bookmarks use the shortcuts of ViewPatternEditor?

@paxcut
Copy link
Contributor Author

paxcut commented Jan 11, 2025

I see how. they don't have any shortcuts either which is the same problem that the bookmarks had. There has to be a better way to handle shortcuts on a more general perspective so that we don't have entire portions of ImHex lack the functionality they should have

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

Successfully merging this pull request may close these issues.

2 participants