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

Create shortcut in "Actions" menu to send Ctrl+Alt+Del #775

Merged
merged 5 commits into from
Aug 27, 2021

Conversation

rw4nn
Copy link
Contributor

@rw4nn rw4nn commented Aug 26, 2021

Resolves #669.

I'm not sure if the function sendCtrlAltDel is in the right place, tell me if it should be in a separate file or something.

Tested on a real TinyPilot but the remote computer is a Chromebook on which Ctrl+Alt+Del does nothing (but I got a key event tester which shows me that the keys were correctly pressed).
So it would be great if someone could test it on a computer where Ctrl+Alt+Del actually does something.


This change is Reviewable

@mtlynch mtlynch requested a review from jotaen August 26, 2021 16:40
@rw4nn rw4nn marked this pull request as ready for review August 26, 2021 16:41
Copy link
Contributor

@jotaen4tinypilot jotaen4tinypilot left a comment

Choose a reason for hiding this comment

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

So it would be great if someone could test it on a computer where Ctrl+Alt+Del actually does something.

I’ve tested on Ubuntu (which also has this keyboard shortcut) and it didn’t work. I figured out that it makes a difference whether we activate the left or right “Alt” modifier – on Ubuntu, it only works with altLeft. I think that would be safer anyway, because some keyboards don’t have a “Right Alt” modifier, but “Alt Gr” instead. Wikipedia also shows this shortcut with “Left Alt”.

I'm not sure if the function sendCtrlAltDel is in the right place, tell me if it should be in a separate file or something.

Maybe some context around the file structure: app.js is a script that’s meant for bootstrapping the frontend app. It contains all the wiring, side-effects and basic setup. All other JS files are modules containing the actual logic. That being said, I think app.js would not be the right place for this function, because it’s too specific. I’d see two options:

  • Either we extract the function into a separate file and import it in app.js. In this case I’d only extract the procedure to create the keystroke object, but not the processKeystroke invocation, since that is a side-effect that’s better off in app.js. Conceptually speaking, the new file could be seen like a “collection” of common keyboard shortcuts.
  • Or we inline the logic in the event listener callback directly.

I personally would have done the latter probably, since I’d find a separate file slightly overkill for this small bit of logic. Should we add more combinations in the future, though, I’d refactor to a separate module. In the end, I’d be fine with both here, so feel free to make a decision.

One semi-related feedback about the UI, cc @mtlynch : I’m not sure it’s intuitive enough to place this menu item as is in the “Actions” menu. My first impression was that it looks oddly specific in there and I probably wouldn’t have been 100% certain what it does without knowing the context. As an idea, a submenu might help to give more logical structure, see below. It’s admittedly not optimal because it only contains one item, I personally would still favour it for clarity, however.

Screenshot 2021-08-26 at 19.02.09.png

Reviewable status: 0 of 1 LGTMs obtained

@jotaen jotaen removed their request for review August 26, 2021 18:18
@mtlynch
Copy link
Contributor

mtlynch commented Aug 26, 2021

I’ve tested on Ubuntu (which also has this keyboard shortcut) and it didn’t work. I figured out that it makes a difference whether we activate the left or right “Alt” modifier – on Ubuntu, it only works with altLeft. I think that would be safer anyway, because some keyboards don’t have a “Right Alt” modifier, but “Alt Gr” instead. Wikipedia also shows this shortcut with “Left Alt”.

Whoops, that's my fault. I specified RightAlt in the bug.

In my mind, I thought the way I typed this key was with RightAlt, but trying it on a keyboard from muscle memory, I see that I'm actually hitting LeftAlt.

One semi-related feedback about the UI, cc @mtlynch : I’m not sure it’s intuitive enough to place this menu item as is in the “Actions” menu. My first impression was that it looks oddly specific in there and I probably wouldn’t have been 100% certain what it does without knowing the context. As an idea, a submenu might help to give more logical structure, see below. It’s admittedly not optimal because it only contains one item, I personally would still favour it for clarity, however.

Yeah, that's a good point. I think it would make sense to have it nested as "Keyboard Shortcuts > Ctrl + Alt Del" even though it's just one shortcut for now.

Copy link
Contributor Author

@rw4nn rw4nn left a comment

Choose a reason for hiding this comment

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

Thanks a lot Jan for testing and explaining the file structure 👍 Ok so I refactored with altLeft and inlined the logic of sendCtrlAltDel, as yes this is a very small piece of logic and we can refactor later if we add more shortcuts.

Updated the menu, Ctrl + Alt + Del is now nested under "Keyboard Shortcuts".

Should be okay now, tell me what you think!

Reviewable status: 0 of 1 LGTMs obtained

Copy link
Contributor

@jotaen4tinypilot jotaen4tinypilot left a comment

Choose a reason for hiding this comment

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

:lgtm: 🎉 🥳

(altLeft fix confirmed on my device.)

Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained

@rw4nn rw4nn merged commit 31e77ce into master Aug 27, 2021
@rw4nn rw4nn deleted the shortcut-ctrl-alt-del branch August 27, 2021 09:30
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.

Include shortcut to send "CTRL + ALT + DEL" keys to remote device as an Action menu item
3 participants