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 hotkeys for Multi images (Compound files) #1561

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

sergzak022
Copy link
Collaborator

Description

Fix a bug with hotkeys that happens in compound file context with multiple images. Updated HotkeyListener documentation to make it easier to use.

Bug Description:
Pressing a hotkey makes registered hotkey callbacks fire for all the image annotation panes

@@ -1733,6 +1741,10 @@ export default Vue.extend({

this.setupHotkeys(this.hotkeyListenerScope)

if ( this.is_active ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once image annotation components are mounted, need to activate registered hotkeys ofr the active one.

}
if ( this.is_active) {
this.hotkey_listener.setScopes([this.hotkeyListenerScope])
} else {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The else block is needed for situations when user annotates in the compound context. For example text_annotation component currently uses a different mechanism for registering hotkeys. If user switches from image annotation to text in the same compound context, we needs to deactivate image annotation hotkeys.

@sergzak022
Copy link
Collaborator Author

Hey Anthony, I'm not removing old hotkey manager class because it looks like it's still used by text_annotation_core component. Please see listeners_map method of src/components/annotation/annotation_ui_factory.vue and keydown_event_listeners/keyup_event_listeners methods of text_annotation_core.vue. Once we move text_annotation_core to use the new HotkeyListener, we can get rid of HotkeyManager.

@anthony-chaudhary anthony-chaudhary changed the title Fix hotkeys for multy images Fix hotkeys for Multi images (Compound files) Oct 20, 2023
@anthony-chaudhary
Copy link
Member

Makes sense regarding keeping legacy HotkeyManager until text moves over to new one.

Tested manually and appears all working as expected now, thanks Sergey!

@anthony-chaudhary
Copy link
Member

anthony-chaudhary commented Oct 20, 2023

One small note for future is when we do remove legacy HotkeyManager, I believe we want to move the is_active scope setting to the higher level component (e.g. where HotkeyManager is currently sitting) to minimize code in "leaf" components.

(Edit: That's my interpretation and agreement of your idea (re: thin leaf nodes) from prior call)

@anthony-chaudhary anthony-chaudhary merged commit 1cc6678 into master Oct 20, 2023
3 checks passed
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.

None yet

2 participants