-
Notifications
You must be signed in to change notification settings - Fork 59
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
use ctrl-shift-n to open new session window #185
base: master
Are you sure you want to change the base?
Conversation
mosh_app/mosh_window.js
Outdated
@@ -36,7 +36,14 @@ function execMosh() { | |||
|
|||
// Don't exit fullscreen with ESC. | |||
terminal.document_.onkeyup = function(e) { | |||
if (e.keyCode == 27) { | |||
if (e.keyCode == 78 && e.ctrlKey && e.shiftKey){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't the right way to bind this. use terminal.keyboard.bindings.addBinding
inside of terminal.onTerminalReady
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i simply stuck with the pattern from the existing code. if y'all want to overhaul the key bindings fully, that's beyond the scope of this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's simply not true. the ESC key hack here is not doable in hterm and is to workaround fullscreen behavior on some setups. that's completely different from adding new keybindings.
mosh_app/mosh_window.js
Outdated
@@ -36,7 +36,14 @@ function execMosh() { | |||
|
|||
// Don't exit fullscreen with ESC. | |||
terminal.document_.onkeyup = function(e) { | |||
if (e.keyCode == 27) { | |||
if (e.keyCode == 78 && e.ctrlKey && e.shiftKey){ | |||
chrome.app.window.create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really seems like this should be unified with the existing newSession
logic in background.js. but i'm guessing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newSession() is not available in mosh_window.js... we'd need to move that newSession() logic into a tertiary file that both mosh_window and background inherit, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, something would need reworking. @rpwoodbu would be able to provide suggestions in that regard.
mosh_app/mosh_window.js
Outdated
}); | ||
|
||
|
||
}else if (e.keyCode == 27) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also style-wise, you need spaces around the braces. } else if (....
went ahead and pushed up a change to do proper key binding.... also just removed that escape hack too, it's not necessary on latest chrome os/hterm as far as i can tell... no issues with escape leaving fullscreen. maybe that's a windows/mac thing? not sure... i only use this on chrome os. |
i'm not familiar with this Esc code, but the keyboard shortcut register part looks fine now. thanks! |
open new session window when pressing ctrl-shift-n