-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Implements Commands Key mapping #1876
Conversation
app/config.js
Outdated
|
||
const watchers = []; | ||
|
||
const scanInterval = 2000; |
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 would:
// watch for changes on config every 2s on windows
// https://github.com/zeit/hyper/pull/1772
const watchConfig = process.platform === 'win32' ? { interval: 2000 } : {}
...
gaze(confPath, watchConfig, function (err) {
easier to read?
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.
Yeah agree on that!
One thing that would be 💯^10 is to be consistent with variable names, I'm thinking of the words |
watchers.forEach(fn => fn()); | ||
} | ||
} catch (err) { | ||
dialog.showMessageBox({ |
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 PR changes from displaying a dialog to just a notification, correct?
I think it's nice with the dialog since you need to click "OK" to close it
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 won't change it since using a dialog prevent hyper from opening.
|
||
exports.extendKeymaps = function (keymaps) { | ||
if (keymaps) { | ||
cfg.keymaps = keymaps; |
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.
Hmm.. I don't like mutating stuff like this. But I think we do this in other places? Maybe that's a job for another PR.
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.
yeah we do this alot...
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'm OK with this for now, but we should clean it all up
app/config/import.js
Outdated
const {writeFileSync, readFileSync} = require('fs'); | ||
const {defaultConfig, confPath} = require('./paths'); | ||
const _init = require('./init'); | ||
const _keys = require('./keymaps'); |
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.
nit: rename this to _keymaps
, easier to understand the code
return module.exports; | ||
}; | ||
|
||
const _syntaxValidation = function (cfg) { |
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.
const _syntaxValidation = function (cfg) {
try {
return new vm.Script(cfg, { filename: '.hyper.js', displayErrors: true })
} catch (error) {
notify(`Error loading config: ${error.name}, see DevTools for more info`)
console.error('Error loading config:', error)
return
}
}
tested:
_syntaxValidation(`
const c = 'a'
const b = 'd'
const c = '2'
`)
notify: Error loading config: SyntaxError, see DevTools for more info
console:
Error loading config:
.hyper.js:4
const c = '2'
^^^
SyntaxError: Identifier 'c' has already been declared
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.
Are you commenting as an error or a test?
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.
haha the comment was to update _syntaxValidation to the code i wrote in my comment,
making it show what type of error happened, and log the row in .hyper.js to console
the blocks under is to show what it does
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.
Done 👍
app/config/paths.js
Outdated
|
||
const icon = resolve(__dirname, '../static/icon.png'); | ||
|
||
const keymapPath = resolve(__dirname, '../keymaps'); | ||
const darwinKeys = resolve(keymapPath, 'darwin.json'); |
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.
Should't these be path.join
?
http://tips.tutorialhorizon.com/2017/05/01/path-join-vs-path-resolve-in-node-js/
don't know how resolve vs join is performance/resource-wise,
it's probably so tiny that it's non-existent 😄
🤔
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.
given that they contain ..
resolve is probably better.
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.
they do not @MartyGentillon resolve(keymapPath, 'darwin.json')
keymapPath
is already expanded
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.
True enough for this particular path, but that would be the reasoning nonetheless.
app/config/keymaps.js
Outdated
}; | ||
|
||
const _import = function (customsKeys) { | ||
const path = () => { |
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.
Why not move this to app/config/paths.js
and just expose it as defaultPlatformKeyPath
?
Is there a need to get other platforms' keys?
@@ -0,0 +1,34 @@ | |||
{ |
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.
run these trough http://jsonprettyprint.com or similar 😄
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.
Done 👍
app/menus/menu.js
Outdated
pluginsMenu(updatePlugins), | ||
windowMenu(), | ||
helpMenu() | ||
shellMenu(commands, createWindow), |
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.
ES6 style:
const menu = [
...(process.platform === 'darwin' ? darwinMenu(commands) : []),
...shellMenu(commands, createWindow),
...editMenu(commands),
...viewMenu(commands),
...pluginsMenu(commands, updatePlugins),
...windowMenu(commands),
...helpMenu(commands)
]
then remove this further down:
if (process.platform === 'darwin') {
...menu.unshift...
}
💅
to avoid mutations (unshift)
app/menus/menus/shell.js
Outdated
@@ -29,7 +27,7 @@ module.exports = function (createWindow) { | |||
}, | |||
{ | |||
label: 'Split Vertically', | |||
accelerator: accelerators.splitVertically, | |||
accelerator: commands['pane:vertical'], |
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.
maybe pane:splitVertical
or pane:split:vertical
to make it clearer, same with horizontal
app/menus/menus/window.js
Outdated
}, | ||
{ | ||
role: 'zoom' | ||
role: 'zoom', |
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.
What does this actually do? 😂 never used it
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.
Apparently nothing on Windows?
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.
Aha, makes the window fill the screen (on macOS) 😄
It's the same thing as clicking the green traffc-light on macOS
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.
Given that it does nothing on Windows (and KDE), should it even be in the menus on Windows (or Linux)?
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.
Alternately, that could be a bug, It looks like that is supposed to be the same as "Maximize" in windows and linux, which don't appear anywhere in the menus on either (though they do appear in the window controls).
app/menus/menus/window.js
Outdated
@@ -71,7 +70,7 @@ module.exports = function () { | |||
}, | |||
{ | |||
role: 'togglefullscreen', | |||
accelerators: accelerators.enterFullScreen | |||
accelerators: commands['window:full'] |
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.
window:toggleFullScreen
, easier to understand
57cf0db
to
e867c29
Compare
@albinekb Next review? |
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.
💯 Nice work!
This is awesome. Is there going to be a way to see a list of available commands, or maybe have a way to allow commands to be logged to the console when you do some action (similar to Sublime Text's |
mmm Make an issue for this as a suggestion ;) something like a command invocation actions |
@ppot Does this fix Command ctrl + C does not work #1121? |
So, |
Hype! Can confirm that this is indeed working! 🎉 👏 Any idea when it'll get released? 😀 |
Ditto @computersarecool, especially given that this is linked from the v1.4.0 release-notes — what, exactly, just got added? ^_^ |
for @ELLIOTTCABLE and everyone else looking for available shortcuts look 'em up here https://github.com/zeit/hyper/blob/master/app/keymaps/ |
-> keymaps files
-> config structure change (file deconstruction)
-> plugins extends keymaps
-> update hterm to use
commands-registry