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

implemented support for mutliple shell definitions #1347

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,3 @@ npm-debug.log
# optional dev config file and plugins directory
.hyper.js
.hyper_plugins

11 changes: 6 additions & 5 deletions app/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ const isDev = require('electron-is-dev');
// Ours
const AutoUpdater = require('./auto-updater');
const toElectronBackgroundColor = require('./utils/to-electron-background-color');
const createMenu = require('./menu');
const createRPC = require('./rpc');
const notify = require('./notify');
const fetchNotifications = require('./notifications');
Expand All @@ -61,6 +60,7 @@ const config = require('./config');

config.init();

const createMenu = require('./menu');
Copy link
Member

Choose a reason for hiding this comment

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

Could you move it up there where it was before? There's no need for this change 🙌

Copy link
Author

Choose a reason for hiding this comment

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

Actually there is a need for this change, the createMenu file now relies on configuration options, therefore must be required after the config is initialised.

const plugins = require('./plugins');
const Session = require('./session');

Expand Down Expand Up @@ -198,7 +198,8 @@ app.on('ready', () => installDevExtensions(isDev).then(() => {
// If no callback is passed to createWindow,
// a new session will be created by default.
if (!fn) {
fn = win => win.rpc.emit('termgroup add req');
const shellOpts = options.shellOpts || {};
fn = win => win.rpc.emit('termgroup add req', {shellOpts});
}

// app.windowCallback is the createWindow callback
Expand All @@ -217,9 +218,9 @@ app.on('ready', () => installDevExtensions(isDev).then(() => {
}
});

rpc.on('new', ({rows = 40, cols = 100, cwd = process.argv[1] && isAbsolute(process.argv[1]) ? process.argv[1] : homedir(), splitDirection}) => {
const shell = cfg.shell;
const shellArgs = cfg.shellArgs && Array.from(cfg.shellArgs);
rpc.on('new', ({rows = 40, cols = 100, shellOpts = {}, cwd = process.argv[1] && isAbsolute(process.argv[1]) ? process.argv[1] : homedir(), splitDirection}) => {
const shell = shellOpts.path || cfg.shell;
const shellArgs = shellOpts.args || (cfg.shellArgs && Array.from(cfg.shellArgs));

initSession({rows, cols, cwd, shell, shellArgs}, (uid, session) => {
sessions.set(uid, session);
Expand Down
61 changes: 60 additions & 1 deletion app/menu.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

We don't need use strict 😅

Copy link
Author

@AlmirKadric AlmirKadric Jan 28, 2017

Choose a reason for hiding this comment

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

To use ES6 features on node properly it is required
e.g. my use of for (const shellName of Object.keys(shells)) { will break without it. Other than node 6 the above behaviour change with or without use strict. Without it shellName is set on the first iteration and remains constant for all other iterations. With it shellName is a constant to the scope of the loops inner block, however changes properly with each iteration.

Copy link
Author

Choose a reason for hiding this comment

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

If anyone else is confused try this in node 5

var a = [1,2,3,4,5,6]; for (const b of a) { console.log(b); }

vs

'use strict'; var a = [1,2,3,4,5,6]; for (const b of a) { console.log(b); }

Removing the 'use strict'; would essentially mean dropping support for anything less than node 6.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it's OK now since hyper is running on node 7.4 since #1848 👌


const os = require('os');
const path = require('path');
const {app, shell, dialog} = require('electron');

const {accelerators} = require('./accelerators');
const {getConfigDir} = require('./config');
const {getConfig,getConfigDir} = require('./config');

const isMac = process.platform === 'darwin';
const appName = app.getName();
const shells = getConfig().shells || {};
const shellKeys = Object.keys(shells);

// based on and inspired by
// https://github.com/sindresorhus/anatine/blob/master/menu.js
Expand Down Expand Up @@ -59,6 +63,56 @@ module.exports = ({createWindow, updatePlugins}) => {
]
};

const otherShellsMenu = {
label: 'Other Shells',
submenu: []
};

for (const shellName of shellKeys) {
const shellOpts = shells[shellName];

otherShellsMenu.submenu.push({
label: shellName,
submenu: [
{
label: 'New Window',
click() {
createWindow(null, {shellOpts});
}
},
{
label: 'New Tab',
click(item, focusedWindow) {
if (focusedWindow) {
focusedWindow.rpc.emit('termgroup add req', {shellOpts});
} else {
createWindow(null, {shellOpts});
}
}
},
{
type: 'separator'
},
{
label: 'Split Vertically',
click(item, focusedWindow) {
if (focusedWindow) {
focusedWindow.rpc.emit('split request vertical', {shellOpts});
}
}
},
{
label: 'Split Horizontally',
click(item, focusedWindow) {
if (focusedWindow) {
focusedWindow.rpc.emit('split request horizontal', {shellOpts});
}
}
}
]
});
}

const shellOrFileMenu = {
label: isMac ? 'Shell' : 'File',
submenu: [
Expand Down Expand Up @@ -101,6 +155,11 @@ module.exports = ({createWindow, updatePlugins}) => {
}
}
},
...(
shellKeys.length > 0 ?
[{type: 'separator'}, otherShellsMenu] :
[]
),
{
type: 'separator'
},
Expand Down
12 changes: 8 additions & 4 deletions lib/actions/term-groups.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,16 @@ import getRootGroups from '../selectors';
import {setActiveSession, ptyExitSession, userExitSession} from './sessions';

function requestSplit(direction) {
return () => (dispatch, getState) => {
return options => (dispatch, getState) => {
const {shellOpts} = options || {};
const {ui} = getState();
dispatch({
type: SESSION_REQUEST,
effect: () => {
rpc.emit('new', {
splitDirection: direction,
cwd: ui.cwd
cwd: ui.cwd,
shellOpts
});
}
});
Expand All @@ -37,8 +39,9 @@ export function resizeTermGroup(uid, sizes) {
};
}

export function requestTermGroup() {
export function requestTermGroup(options) {
return (dispatch, getState) => {
const {shellOpts} = options || {};
const {ui} = getState();
const {cols, rows, cwd} = ui;
dispatch({
Expand All @@ -48,7 +51,8 @@ export function requestTermGroup() {
isNewGroup: true,
cols,
rows,
cwd
cwd,
shellOpts
});
}
});
Expand Down
12 changes: 6 additions & 6 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,16 +86,16 @@ rpc.on('session clear req', () => {
store_.dispatch(sessionActions.clearActiveSession());
});

rpc.on('termgroup add req', () => {
store_.dispatch(termGroupActions.requestTermGroup());
rpc.on('termgroup add req', options => {
store_.dispatch(termGroupActions.requestTermGroup(options));
Copy link
Member

Choose a reason for hiding this comment

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

Same

Copy link
Author

Choose a reason for hiding this comment

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

what do you mean??? that's a function call

});

rpc.on('split request horizontal', () => {
store_.dispatch(termGroupActions.requestHorizontalSplit());
rpc.on('split request horizontal', options => {
store_.dispatch(termGroupActions.requestHorizontalSplit(options));
Copy link
Member

Choose a reason for hiding this comment

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

Same

Copy link
Author

Choose a reason for hiding this comment

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

what do you mean??? that's a function call

});

rpc.on('split request vertical', () => {
store_.dispatch(termGroupActions.requestVerticalSplit());
rpc.on('split request vertical', options => {
store_.dispatch(termGroupActions.requestVerticalSplit(options));
Copy link
Member

Choose a reason for hiding this comment

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

Same

Copy link
Author

Choose a reason for hiding this comment

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

what do you mean??? that's a function call

});

rpc.on('reset fontSize req', () => {
Expand Down