implemented support for mutliple shell definitions #1347

AlmirKadric · 2017-01-06T07:51:53Z

I've implemented an additional menu item in File which allows you to select other shells that may have been configured inside your config file. My use case was for Windows (Bash vs Powershell), but this would apply to macos and linux for anyone who uses more than a single shell flavour. I wanted to make this a plugin, however the work was too integrated for this to be possible (or rather so simple enough).

Possible enhancements to this PR:

don't show menu item if shells is not defined in config
add possibility of accelerator shortcut definition from config file

Please let me know if any of the above or other fixes are required, and I'll jump on it as soon as I can.

P.S. PR checks are failing due to external code which I didn't touch. Seems someone pushed bad code to master...

albinekb · 2017-01-06T10:10:35Z

.gitignore

@@ -11,3 +11,5 @@ npm-debug.log
 .hyper.js
 .hyper_plugins

+# IDEs
+.idea


Add this to your global .gitignore

@AlmirKadric https://help.github.com/articles/ignoring-files/#create-a-global-gitignore

ah good point, thanks for that
will do 👍

matheuss · 2017-01-28T01:39:56Z

.gitignore

@@ -10,4 +10,3 @@ npm-debug.log
 # optional dev config file and plugins directory
 .hyper.js
 .hyper_plugins
-


Could you put that blank line back again? 😅

sure, sorry for not noticing that XD

actually it was the other way around, the new line was missing in the repo
my IDE inserted it according to the .editorconfig

matheuss · 2017-01-28T01:40:25Z

app/index.js

@@ -50,6 +49,7 @@ const config = require('./config');

 config.init();

+const createMenu = require('./menu');


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

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.

matheuss · 2017-01-28T01:40:45Z

app/menu.js

@@ -1,11 +1,15 @@
+'use strict';


We don't need use strict 😅

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.

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.

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

matheuss · 2017-01-28T01:42:28Z

lib/actions/term-groups.js

@@ -12,14 +12,16 @@ import getRootGroups from '../selectors';
 import {setActiveSession, ptyExitSession, userExitSession} from './sessions';

 function requestSplit(direction) {
-  return () => (dispatch, getState) => {
+  return (options) => (dispatch, getState) => {


Please remove the parenthesis around options – it's not needed for a single function argument

matheuss · 2017-01-28T01:42:34Z

lib/index.js

@@ -64,16 +64,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) => {


Please remove the parenthesis around options – it's not needed for a single function argument

matheuss · 2017-01-28T01:42:39Z

lib/index.js

-rpc.on('termgroup add req', () => {
-  store_.dispatch(termGroupActions.requestTermGroup());
+rpc.on('termgroup add req', (options) => {
+  store_.dispatch(termGroupActions.requestTermGroup(options));


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

matheuss · 2017-01-28T01:42:42Z

lib/index.js

 });

-rpc.on('split request horizontal', () => {
-  store_.dispatch(termGroupActions.requestHorizontalSplit());
+rpc.on('split request horizontal', (options) => {


matheuss · 2017-01-28T01:42:47Z

lib/index.js

-rpc.on('split request horizontal', () => {
-  store_.dispatch(termGroupActions.requestHorizontalSplit());
+rpc.on('split request horizontal', (options) => {
+  store_.dispatch(termGroupActions.requestHorizontalSplit(options));


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

matheuss · 2017-01-28T01:42:50Z

lib/index.js

 });

-rpc.on('split request vertical', () => {
-  store_.dispatch(termGroupActions.requestVerticalSplit());
+rpc.on('split request vertical', (options) => {


matheuss · 2017-01-28T01:42:56Z

lib/index.js

-rpc.on('split request vertical', () => {
-  store_.dispatch(termGroupActions.requestVerticalSplit());
+rpc.on('split request vertical', (options) => {
+  store_.dispatch(termGroupActions.requestVerticalSplit(options));


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

matheuss

@AlmirKadric Thanks for the PR and sorry for the delay!

Please fix the minor issues I outlined in the review 👌

matheuss · 2017-01-28T01:50:27Z

Besides that:

Could you please explain what I should have in my .hyper.js? I tried a couple of combinations for shells, but couldn't get it to work – the new session was always opened with my default shell
You would need to add checks to the ui reducer for the possible values of shell – we wouldn't want people to set a empty '' path for the shell, for example. Check here for an example of how to do it
don't show menu item if shells is not defined in config definitely! 😄

AlmirKadric · 2017-01-28T07:39:55Z

My .hyper.js shells configuration looks like this:

    // the shell to run when spawning a new session (i.e. /usr/local/bin/fish)
    // if left empty, your system's login shell will be used by default
    shell: 'C:\\cygwin64\\bin\\bash.exe',

    // for setting shell arguments (i.e. for using interactive shellArgs: ['-i'])
    // by default ['--login'] will be used
    shellArgs: ['--login'],

    // for environment variables
    env: {},

    // Additional shells
    shells: {
        bash: {
            path: 'C:\\cygwin64\\bin\\bash.exe',
            args: ['--login'],
            env: {}
        },
        powershell: {
            path: 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe',
            args: [],
            env: {}
        }
    },

AlmirKadric · 2017-01-28T07:40:58Z

@matheuss I'll fix the other suggestions as well. Caught up with something else right now, will jump on it as soon as I'm done with that.

AlmirKadric · 2017-01-28T08:07:11Z

@matheuss after taking another look at the code, a reducer isn't required.
Currently if an empty shell path is provided it will be falsey causing it to fall back to the default shell (behaviour of default shell options as it falls back to system default if falsey).
If an invalid shell path is provided...well we can't really check that (unless we check if that file really exists?)
Also for the other options, empty entries are fine, if invalid options well it will break, though not sure how i can validate environment variables and shell startup arguments.

Any other suggestions or corrections to my above statements welcome.

AlmirKadric · 2017-02-16T03:04:23Z

BUMP @matheuss @albinekb
any movement on this?
are there any actionable items left on my side?

ppot · 2017-02-16T03:06:30Z

app/index.js

@@ -186,7 +186,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;


Need to test if options.shellOpts exist

AlmirKadric · 2017-03-23T00:32:53Z

bump????

henrikruscon · 2017-03-23T03:24:17Z

lib/index.js

 });

-rpc.on('split request horizontal', () => {
-  store_.dispatch(termGroupActions.requestHorizontalSplit());
+rpc.on('split request horizontal',options => {


Lack of space rpc.on('split request horizontal', options => { ✌️

@AlmirKadric

albinekb · 2017-03-23T07:50:41Z

Sorry @AlmirKadric have not had time to look at this, will look trough it tonight 🤘

Stanzilla · 2017-05-02T07:04:26Z

@albinekb did you have time yet? Really would love to get this in.

albinekb · 2017-05-19T17:05:46Z

app/menu.js

+    submenu: []
+  };
+
+  for (const shellName of Object.keys(shells)) {


I would save Object.keys(shells) as a const since you're using it twice in this file? 🤔

const shellKeys = Object.keys(shells)

?

AlmirKadric · 2017-05-21T13:28:11Z

Pulled from master and squashed into a single commit

albinekb

👌 for me this is ready for merge

albinekb · 2017-05-26T06:44:23Z

ping @matheuss

Hypercubed · 2017-05-30T03:56:25Z

I just gave this a try... works well. Needs a rebase and some error trapping (for example bad paths).

MartyGentillon · 2017-05-30T18:53:19Z

One other minor change: this needs to be documented (probably in the default config file), otherwise people won't find the new feature.

tulsileathers · 2017-06-28T13:27:52Z

Is this going to be merged in anytime soon?

jack-guy · 2017-08-05T22:06:41Z

Anything specifically holding this PR up? I want this badly enough that I'd take it on if @AlmirKadric doesn't have the time to move forward with it.

AlmirKadric · 2017-08-08T02:23:30Z

@Harangue from what I'm reading above, the remaining items are

error trapping for bad paths (though I would prefer he commented on that on the actual lines)
documentation in the default config file
merge develop and rebase

@albinekb @matheuss can you please confirm this is the last of the changes
I'm happy to make them, but in all fairness this PR has been sitting here for a terribly long time (6+ months)

I've actually made my own terminal multiplexer using similar technologies as a result since I couldn't wait for my changes to be merged, and branching from hyper didn't make sense as I wanted to make some fundamental design changes (would have been a complete rewrite of the underlying architecture)

cameron-pascal · 2017-12-04T16:29:56Z

@albinekb, @matheuss Has this PR stalled / what it's current status? This would be an amazing feature to have on windows.

eamodio · 2018-02-06T04:26:57Z

I would really love to see this in as well -- what is still left to be done, since this was started about a year ago?

AlmirKadric · 2018-02-06T05:43:06Z

@CameronMaxwell @eamodio
I'm still waiting on feedback from the core team as to finalizing what is needed to get this PR merged. I don't fancy fixing things if only to have more comments made for fixes. Once they confirm what's required, I'll updated the PR for merging.

Alternatively feel free to take a look at my own terminal multiplexer here:
https://github.com/AlmirKadric-Published/exTerm-electron
Its not as advanced/clean/pretty in some regards, but is ahead in others, when compared to hyperterm. If it suits your needs but you need this or some other minor features, I'm happy to make the changes for you. Its been more than enough for my current needs, albeit one really annoying bug that I haven't had the time to fix completely. I'm more than happy to be more active on it if there is a community of users.

jordandrako · 2018-06-07T19:56:16Z

Did this die with version 2's release?

Stanzilla · 2019-10-15T00:12:29Z

@AlmirKadric willing to pick this up again? I think a solution like Terminus https://github.com/Eugeny/terminus would be nice

AlmirKadric · 2019-10-23T02:04:54Z

@Stanzilla unfortunately too much time has passed and I have since moved await from hyper and build my own terminal multiplexer using similar tech. The current beta version fills all my needs including the above feature. Even if I were to pick this up again, I believe enough code has changed that I would be essentially re-implementing the feature. As such I would suggest anyone who needs the feature to start again.

albinekb reviewed Jan 6, 2017

View reviewed changes

matheuss reviewed Jan 28, 2017

View reviewed changes

matheuss requested changes Jan 28, 2017

View reviewed changes

AlmirKadric mentioned this pull request Feb 16, 2017

Add Windows support and first-class Linux support #946

Merged

13 tasks

ppot reviewed Feb 16, 2017

View reviewed changes

henrikruscon reviewed Mar 23, 2017

View reviewed changes

This was referenced Apr 15, 2017

Multiple shell configurations #1718

Closed

Support for multiple shells #1147

Closed

albinekb reviewed May 19, 2017

View reviewed changes

albinekb added the Status: Rebase Needed label May 19, 2017

AlmirKadric force-pushed the feature/moreShells branch from dcc5409 to bac6c0c Compare May 21, 2017 13:23

implemented support for multiple shell definitions

638f461

AlmirKadric force-pushed the feature/moreShells branch from bac6c0c to 638f461 Compare May 21, 2017 13:27

albinekb approved these changes May 21, 2017

View reviewed changes

albinekb added 👀 Awaiting Response Issue or PR is awaiting a response from the author and removed Status: Rebase Needed labels May 21, 2017

rauchg added the WIP label Dec 4, 2018

juancampa force-pushed the master branch from 571231e to 34fb41c Compare May 6, 2019 05:49

Stanzilla mentioned this pull request Oct 15, 2019

Add support for platform-specific shell config #2684

Closed

1 task

AlmirKadric closed this Oct 23, 2019

		@@ -50,6 +49,7 @@ const config = require('./config');

		config.init();

		const createMenu = require('./menu');

implemented support for mutliple shell definitions #1347

implemented support for mutliple shell definitions #1347

Conversation

AlmirKadric commented Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlmirKadric Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlmirKadric Jan 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlmirKadric Jan 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matheuss left a comment

Choose a reason for hiding this comment

matheuss commented Jan 28, 2017 • edited Loading

AlmirKadric commented Jan 28, 2017

AlmirKadric commented Jan 28, 2017

AlmirKadric commented Jan 28, 2017 • edited Loading

AlmirKadric commented Feb 16, 2017

Choose a reason for hiding this comment

AlmirKadric commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albinekb commented Mar 23, 2017

Stanzilla commented May 2, 2017

albinekb May 19, 2017 • edited Loading

Choose a reason for hiding this comment

AlmirKadric commented May 21, 2017

albinekb left a comment • edited Loading

Choose a reason for hiding this comment

albinekb commented May 26, 2017

Hypercubed commented May 30, 2017

MartyGentillon commented May 30, 2017

tulsileathers commented Jun 28, 2017

jack-guy commented Aug 5, 2017

AlmirKadric commented Aug 8, 2017 • edited Loading

cameron-pascal commented Dec 4, 2017 • edited Loading

eamodio commented Feb 6, 2018

AlmirKadric commented Feb 6, 2018 • edited Loading

jordandrako commented Jun 7, 2018

Stanzilla commented Oct 15, 2019 • edited Loading

AlmirKadric commented Oct 23, 2019

AlmirKadric commented Jan 6, 2017 •

edited

Loading

AlmirKadric Jan 7, 2017 •

edited

Loading

AlmirKadric Jan 28, 2017 •

edited

Loading

AlmirKadric Jan 28, 2017 •

edited

Loading

matheuss commented Jan 28, 2017 •

edited

Loading

AlmirKadric commented Jan 28, 2017 •

edited

Loading

albinekb May 19, 2017 •

edited

Loading

albinekb left a comment •

edited

Loading

AlmirKadric commented Aug 8, 2017 •

edited

Loading

cameron-pascal commented Dec 4, 2017 •

edited

Loading

AlmirKadric commented Feb 6, 2018 •

edited

Loading

Stanzilla commented Oct 15, 2019 •

edited

Loading