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

Fixed issues with hotkeys #42

Merged
merged 10 commits into from Apr 27, 2021
Merged

Fixed issues with hotkeys #42

merged 10 commits into from Apr 27, 2021

Conversation

PJEstrada
Copy link
Contributor

Seemed that the real problem was not so much with hot keys but with the selected prop of the instances. When we changed frames the watcher would always reset to false the instance select and that would cause some of the hotkeys for related to selection to stop working (like copy and paste).

This was cause due to not updating the this.previous_current_frame value in the watcher. Now the select is working properly and the hotkeys are following

I also tested the w,a,s,d the space bar, the escape keys, numkeys 1-9 and they all seem to be working fine.

Please let me know if you are aware of any other hotkeys malfunctioning so I can dive deeper into them here.

Seemed that the real problem was not so much with hot keys but with the selected prop of the instances. When we changed frames the watcher would always reset to false the instance select and that would cause some of the hotkeys for related to selection to stop working (like copy and paste). 

Please let me know if you are aware of any other hotkeys malfunctioning so I can dive deeper into them here.
@cypress

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

If we do want to - then let's make sure L175 is false to re-enable hotkeys
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

context of these events also potentially blocking hotkeys
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

1) A user may click outside (not use close button) We need to propagate that event back up, and then call `close_share_dialog()`
2) Issue panel in of itself is not great reason, putting it closer to actual usage
@anthony-sarkis
Copy link
Member

Hey,
I tested with the directory select in media bar and was able to reproduce issue of hotkeys. Inspecting state it showed that the flag we check $store.state.user.is_typing_or_menu_open was remaining in true even when deselecting. Watching the mutations I see that it's only firing one event.
menu is not setting state
I committed d715b85

  1. This will cover the blur event (no change) case.
  2. I moved the "did select something" case into the change event so it's more clear the relationship of that (vs having it seperate in the update:return value -> which made it look as though it was the mirror of the focus when it's not.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@anthony-sarkis
Copy link
Member

I found a few more
2dfb268
Covers

  1. Share menu dialog is clicked outside (not close button)
  2. Moves the issue dialog creation closer to the actual interface, for menus that don't fill full screen I think this is probably better

userscript select was missing blur
b6c151a

Please double check ok with
f354470
As far as I can see there isn't anything in instance history that needs to disable this,
if there is, then prefer to put it closer to source like in other commits
3e559f7

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@anthony-sarkis
Copy link
Member

@PJEstrada I think this is ready for a fresh look when ready

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@PJEstrada
Copy link
Contributor Author

PJEstrada commented Apr 27, 2021

Hey Anthony thanks for catching the onBlur issue :) I checked the code and its looking good. I added 2ce3b59 which just adds the delete hotkey in the global context so it still works if the mouse is not inside the canvas.

I will wait for tests to pass and we can merge this to master

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@anthony-sarkis anthony-sarkis merged commit e71aad2 into master Apr 27, 2021
@anthony-sarkis anthony-sarkis deleted the improve-hotkeys branch April 27, 2021 23:39
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link

Unit Test Results

  1 files  ±0    1 suites  ±0   52s ⏱️ ±0s
84 tests ±0  84 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit e71aad2. ± Comparison against base commit e71aad2.

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

Successfully merging this pull request may close these issues.

None yet

2 participants