Add hook for hyper-htm plugin: https://github.com/MisterTea/hyper-htm #3026

MisterTea · 2018-05-17T06:49:18Z

Rebase of #2988 onto the canary branch. See #2988 for details.

MisterTea · 2018-05-17T06:50:25Z

@albinekb Is there an ETA on when someone can take a first pass at a review?

chabou · 2018-05-17T07:15:26Z

chabou · 2018-05-17T19:09:27Z

lib/reducers/term-groups.js

@@ -192,7 +192,10 @@ const reducer = (state = initialState, action) => {
        return setActiveGroup(state, action);
      }

-      const uid = uuid.v4();
+      let uid = uuid.v4();


Preserving const is maybe better here: const uid = action.termGroupUid || uuid.v4()

chabou · 2018-05-17T19:13:41Z

app/commands.js

@@ -10,16 +10,16 @@ const commands = {
  },
  'tab:new': focusedWindow => {
    if (focusedWindow) {
-      focusedWindow.rpc.emit('termgroup add req');
+      focusedWindow.rpc.emit('termgroup add req', {});


Argument will be destructured in the handler. I think that you can remove all these empty object params.

The empty object is needed to avoid this crash:

chabou · 2018-05-17T19:35:33Z

app/ui/window.js

-      };
-
-      initSession(sessionOpts, (uid, session) => {
+      window.initSession(sessionOpts, (uid, session) => {


I don't like to declare these functions as global to let plugins override them.
But I understand your needs.

I really appreciate all your efforts to make your changes minimal 👌

I need more time to figure out if there is another (cleaner) way to achieve this.
Any thoughts @albinekb?

We'll need some middleware mechanism (chain plugins and interrupt/enhance actions) on the main process too

FWIW the functions aren't global, but they are members of the BrowserWindow class (this isn't the same window as the global window in browser JS).

It may be cleaner to not pollute the BrowserWindow object with these things and actually use the Window object (right now only the constructor is used). If you like, I can do that in a follow-up PR.

Yeah, sorry: we should not pollute BrowserWindow object.

We should add something in our API something like: decorateSession(Session).
But I didn't study your plugin enough yet to find what is suitable or not.

chabou · 2018-05-17T19:37:13Z

app/ui/window.js

        sessions.set(uid, session);
        rpc.emit('session add', {
          rows: sessionOpts.rows,
          cols: sessionOpts.cols,
          uid,
          splitDirection: sessionOpts.splitDirection,
          shell: session.shell,
-          pid: session.pty.pid
+          pid: typeof session.pty !== 'undefined' ? session.pty.pid : null,


pid: session.pty && session.pty.pid should be ok.

chabou · 2018-05-17T19:41:41Z

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 (sessionUid, sourceUid) => (dispatch, getState) => {


I think that sourceUid is indeed a more accurate name than activeUid. But maybe, it should have the same name as here: export function addSession({uid, shell, pid, cols, rows, splitDirection, termGroupUid, activeUid}) {

It would be better to change activeUid to sourceUid but you're right to keep this name to have backward compatibility.

Yeah that makes sense. I'll change activeUid to sourceUid to keep things consistent.

Be careful, we can't rename activeUid in redux actions.
Maybe better to change sourceUid to activeUid.

Oh, because people may be intercepting activeUid... got it. Ok, I'll change sourceUid

chabou · 2018-05-17T20:02:55Z

lib/index.js

@@ -103,16 +100,16 @@ rpc.on('session break req', () => {
  store_.dispatch(sessionActions.sendSessionData(null, '\x03'));
 });

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


Will these params always be undefined without your plugin? 🤔
This is a problem. It adds some complexity than a newcomer can't understand.

That's a good point. Let me set them upstream

MisterTea · 2018-05-17T22:32:44Z

Alright this should address all of the feedback except the issue with Window/BrowserWindow.

FWIW here's my two cents on what to do:

Don't return the BrowserWindow object in the Window constructor (which is anyways gross :-) ) but let the Window object be created and add the BrowserWindow to the Window object
Instead of making rpc, sessions, & all of my new functions as members of BrowserWindow, make them members of Window.
Create a new onWindow handler that passes in the Window object and deprecate the current onWindow handler.

MisterTea · 2018-05-25T22:00:37Z

@chabou @albinekb friendly ping :-)

chabou · 2018-05-29T11:59:31Z

Window or BrowserWindow is quite the same for me.
This is not a suitable design to let plugins override some "public" methods. This is not the hyper plugin philosophy.

@albinekb What do you think about adding a decorateSession(Session) or extendSession API handler. It might return a class that extends our Session class (or an already extended by another plugin).

@MisterTea Would it be enough for your plugin to act?

MisterTea · 2018-05-29T15:58:16Z

@chabou Today I will try to remove as much of the window function overrides as possible and I'll let you know how far I can get with just changes to Session. The tricky part is handling the case where the user closes a session.

chabou · 2018-05-29T16:05:57Z

session.destroy() will be called.
Do not hesitate to explain your tricky part, if I can help

…initialization

MisterTea · 2018-05-30T03:41:55Z

Alright @chabou I was able to move all the logic into a wrapper around "new Session()". Take a look and let me know what you think. For reference, here's the plugin code (not very organized but I will clean up once this core piece is in): https://github.com/MisterTea/hyper-htm/blob/master/index.js

MisterTea · 2018-05-30T17:11:50Z

lib/index.js

-rpc.on('session data', d => {
-  // the uid is a uuid v4 so it's 36 chars long
-  const uid = d.slice(0, 36);
-  const data = d.slice(36);


@chabou Why was this done instead of using an object? Let me know if you want me to revert this part of the PR (I have no idea why to encode the uid + data in this way, but there may be a reason).

MisterTea · 2018-06-04T16:56:31Z

Friendly ping :-)

chabou · 2018-06-04T17:05:46Z

@MisterTea I began to review your changes. My first thought (but maybe not the only one) is that we need to extend Session class and not the instance.

Problem with instance extension is that an unused pty session will be created in your case.

Plugins could use this API this way:

exports.extendSession = (Session) => {
  return class MySession extends Session {
    constructor({rows, cols: columns, cwd, shell, shellArgs}) {
      // call this only if you want a real pty session:
      super({rows, cols: columns, cwd, shell, shellArgs})

      // specific code
      // ...
    }
  }
}

But maybe this is not a good idea...

MisterTea · 2018-06-04T18:19:19Z

The unused pty is destroyed when the session is garbage collected, so it doesn't live more than a few milliseconds. I could add code in the plugin to kill the pty sooner for follower sessions.

Your approach could also work but it would make it difficult to change the session constructor without breaking plugins. If we did want to avoid making the pty, maybe we have a separate startSession() function instead of starting pty in the constructor. Then, plugins could override that function and block the pty that way.

chabou · 2018-06-04T18:55:23Z

The problem is not memory footprint. But design: if we need to add an abstraction to Session class to build some powerful plugins like yours, unnecessary instantiations (like a PTY which has a big cost of time) must be avoided.

I don't see how other plugins would be broken by extending Session class. Can you give me a concrete example to help me to understand?

MisterTea · 2018-06-04T19:08:32Z

If we design the API so the plugins create the session, then we cannot change the session constructor in core without breaking these plugins.

For example, if we decide that not every session should have a current working directory, we can't remove the cwd argument from new Session(...) without breaking the plugins that would be creating these Sessions.

What about changing window.js to something like this (adding verbosity to explain the point):

       const initSession = (opts, fn_) => {
        let newSession = new Session(opts);  // Does bare minimum and nothing pty.
        newSession = app.plugins.extendSession(opts, newSession);
        newSession.init(opts);  // Plugins can override to disable pty
        fn_(opts.sessionUid, newSession);
       };

MisterTea · 2018-06-04T19:18:19Z

Another idea is we can have two API calls: extendSession and newSession. If newSession returns null, then the pty session is created and this session can be extended with extendSession. I would create followersessions with newSession and the htm detection/processing logic would live in extendSession.

MisterTea · 2018-06-05T06:38:34Z

I implemented the init() idea where the session constructor becomes lightweight. Let me know what you think of this approach.

MisterTea · 2018-06-11T22:08:20Z

Any update here?

MisterTea · 2018-06-20T23:18:31Z

Friendly ping :-)

MisterTea · 2018-06-25T18:24:23Z

Hey @chabou , not trying to rush anyone but I'm starting to forget what this PR does and I'm the author, also I would like more people to use Hyper but I can't ask them to recompile it, so we probably should get some closure here :-). Done is better than perfect.

lorencarvalho · 2018-07-16T04:41:33Z

Just chiming into this PR to say I really would love to see this feature :) <3

MisterTea · 2018-07-23T21:18:55Z

Any update here?

MisterTea · 2018-07-30T20:59:38Z

Hey, I've had several people asking me how to use hyper-htm in the interim while the hyper team thinks about how to integrate. Here are some instructions:

git clone https://github.com/MisterTea/hyper.git
cd hyper
git checkout htmcanary
yarn
yarn run dist

Then use the hyper app created in the dist folder, install the "hyper-htm" plugin, and you are good to go :-D

MisterTea · 2018-09-06T22:38:12Z

@chabou Hey, any update? If not, I'm going to close this issue and move on to something else.

MisterTea · 2018-09-10T19:11:05Z

Closing. It shouldn't take 6 months to get a ~50 line PR in. I'll see if there's another terminal that I can integrate HTM with.

chabou · 2018-09-10T19:20:09Z

I really understand that you're disappointed.
Really sorry for that.

jcklpe · 2019-01-20T05:47:52Z

Sad to see that this got closed :( tmux or multiplexing is the biggest reasons that I know of for people who aren't using hyper.

chabou · 2019-01-20T09:28:07Z

@jcklpe this PR didn't address tmux issue 🤔
What is your issue with tmux? Can you link an opened issue or open a new one? Thanks !

This PR was not the right approach to let plugins decorate Session object. We really want to make it possible.

jcklpe · 2019-01-20T09:32:09Z

@chabou I was looking into using tmux and the htm issue was what came up when I searched for it. I was under the impression that tmux doesn't work with hyper.

I was just looking to try out multiplexing etc.

chabou · 2019-01-20T17:55:40Z

@jcklpe please give it a try. A lot of people are using Hyper with tmux.

Do not hesitate to open an issue if you encounter any problem.

jcklpe · 2019-01-22T00:44:21Z

cool, I will try that. Thank you!

MisterTea · 2019-01-22T00:51:01Z

tmux works but tmux control center (where the OS manages the windows and tabs) is what this PR adds.

…

On Mon, Jan 21, 2019, 4:44 PM Aslan French ***@***.***> wrote: cool, I will try that. Thank you! — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#3026 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASuv7nH7CyN0fNSRDzP525DmF-sL3gBks5vFl7tgaJpZM4UCf91> .

chabou · 2019-01-22T08:03:22Z

Yes! And I really want to add what is missing to our API for plugins like hyper-htm.

@MisterTea I understand how it was frustrating to try to tackle this without any success because I had a clear idea of what we exactly need. I know this is too late but I will add this Session API. I am really sorry to not have merged this despite your effort.

MisterTea · 2019-01-22T18:37:06Z

@chabou Great news! Let me know whenever you have something and I will update HTM to use it.

MisterTea · 2019-02-01T02:12:59Z

@chabou Should I reopen this PR or are you working on the session API change in another?

chabou · 2019-02-01T12:26:30Z

I will work in another branch/pr. I already open an RFC/issue to discuss the best way to implement it: #3446

MisterTea mentioned this pull request May 17, 2018

Add support for Headless Terminal Multiplexer (htm) #2988

Closed

chabou self-requested a review May 17, 2018 07:15

chabou suggested changes May 17, 2018

View reviewed changes

Jason Gauci added 2 commits May 25, 2018 22:37

Add hook for hyper-htm plugin: https://github.com/MisterTea/hyper-htm

510cf45

Review feedback

8600c0c

Move hyper-htm injection point to Session creation instead of window …

b6944d5

…initialization

MisterTea force-pushed the htmcanary branch from 5d5910f to b6944d5 Compare May 30, 2018 03:38

MisterTea commented May 30, 2018

View reviewed changes

Move pty session setup to init()

451dc0e

Merge branch 'canary' into htmcanary

31e4dd9

Pass window to support multiple windows in the same plugin instance

38c65ae

Merge branch 'canary' into htmcanary

0803c5d

MisterTea closed this Sep 10, 2018

MisterTea mentioned this pull request Sep 11, 2018

HTM Integration bleenco/bterm#161

Open

Add hook for hyper-htm plugin: https://github.com/MisterTea/hyper-htm #3026

Add hook for hyper-htm plugin: https://github.com/MisterTea/hyper-htm #3026

Conversation

MisterTea commented May 17, 2018

MisterTea commented May 17, 2018

chabou commented May 17, 2018

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

MisterTea commented May 17, 2018 • edited Loading

MisterTea commented May 25, 2018

chabou commented May 29, 2018

MisterTea commented May 29, 2018

chabou commented May 29, 2018

MisterTea commented May 30, 2018

Choose a reason for hiding this comment

MisterTea commented Jun 4, 2018

chabou commented Jun 4, 2018

MisterTea commented Jun 4, 2018

chabou commented Jun 4, 2018

MisterTea commented Jun 4, 2018 • edited Loading

MisterTea commented Jun 4, 2018

MisterTea commented Jun 5, 2018

MisterTea commented Jun 11, 2018

MisterTea commented Jun 20, 2018

MisterTea commented Jun 25, 2018

lorencarvalho commented Jul 16, 2018

MisterTea commented Jul 23, 2018

MisterTea commented Jul 30, 2018 • edited Loading

MisterTea commented Sep 6, 2018

MisterTea commented Sep 10, 2018

chabou commented Sep 10, 2018

jcklpe commented Jan 20, 2019

chabou commented Jan 20, 2019

jcklpe commented Jan 20, 2019

chabou commented Jan 20, 2019

jcklpe commented Jan 22, 2019

MisterTea commented Jan 22, 2019 via email

chabou commented Jan 22, 2019

MisterTea commented Jan 22, 2019

MisterTea commented Feb 1, 2019

chabou commented Feb 1, 2019

MisterTea commented May 17, 2018 •

edited

Loading

MisterTea commented Jun 4, 2018 •

edited

Loading

MisterTea commented Jul 30, 2018 •

edited

Loading