Add basics of how I'd like to see this packaged / structured

[nathansobo]

This branch represents some of how I'd prefer to see this package structured:

• It has a TerminalSession, which can be opened from RootView.open and Project.open via a custom opener with "terminal://" URIs.
• The TerminalSession encapsulates communication with a subprocess via pty.js
• The TerminalView has a TerminalSession as its model object

Here's where I'd like to see the overall structure of this package move: @mutle please let me know if you disagree with any of these ideas:

I think the flow of data should be event-driven rather than oriented around polling, similar to how the editor works. The TerminalSession should forward data from the pty to the TerminalBuffer, and then when the TerminalBuffer is updated it should emit a change event with the start row, end row and delta just like we do with the editor. The view receives this event and immediately repaints the lines. I'm not sure I understand the reason to have a delay or to say when lines are dirty or not. Why not just emit a change event and repaint the screen? I'm imagining the flow of events to look like this:

PTY => TerminalSession => TerminalBuffer => TerminalView

[nathansobo]

Also, I should mention that I'd really like to see the coding style blend in with Atom more in general... biggest nitpicks: spaces between methods and omit () on functions with no parameters.

[mutle]

It has a TerminalSession, which can be opened from RootView.open and Project.open via a custom opener with "terminal://" URIs.

Yea, that's great. I didn't fully understand how this works.

The TerminalSession encapsulates communication with a subprocess via pty.js

Great!

The TerminalView has a TerminalSession as its model object

Makes sense.

and then when the TerminalBuffer is updated it should emit a change event with the start row, end row and delta just like we do with the editor. The view receives this event and immediately repaints the lines. I'm not sure I understand the reason to have a delay

Unfortunately repainting on every event is too slow. When for example the screen is cleared and then filled with new data there is no point where it says "I'm done, update the screen", instead all changes (each character insert or other operation) requires a repaint. A good way to test this is using "telnet towel.blinkenlights.nl". I added the delay to make sure more stuff happened when the next repaint is called.

or to say when lines are dirty or not. Why not just emit a change event and repaint the screen? I'm imagining the flow of events to look like this: PTY => TerminalSession => TerminalBuffer => TerminalView

I didn't realize the change event can provide information on the dirty lines, but now that I think of it, that makes a lot of sense.

[nathansobo]

Yea, that's great. I didn't fully understand how this works.

Actually, it's a feature I added for this package, so didn't miss anything.

Unfortunately repainting on every event is too slow. When for example the screen is cleared and then filled with new data there is no point where it says "I'm done, update the screen", instead all changes (each character insert or other operation) requires a repaint.

Ah, I see. That makes sense. We actually have a similar issue in the editor, but slightly different. The editor view receives the change events from the display buffer ({startRow, endRow, delta}), but it doesn't update the screen right away because we might make multiple changes in the same turn of the event loop. Instead it aggregates all change events, then updates all at once with nextTick. In your case the delay would be longer (setTimeout rather than nextTick) since the pty process is sending multiple events outside of a single event loop cycle. Once we're ready to update the screen, we figure out which rows need to be repainted and read the corresponding lines of the underlying DisplayBuffer to perform the update. So the DisplayBuffer says "this change happened", then the Editor aggregates those changes, figures out what rows need updating, and reads the lines it needs out of the underlying buffer. Seems like that could work well for the case where the same line gets mutated multiple times. You'll get a bunch of update events, but they're all for the same row, but they can all be aggregated and will still only result in the line for that row being updated once. One advantage here is you don't have to track dirty state as a property of the lines. It's more of a concern that the TerminalView handles.

All that said, the way you're approaching it may be fine. It's a bit different from the editor, and my instinct is to want similar problems to be treated similarly, but perhaps it really doesn't matter.

Another thing: in Atom we have a distinction between the words "line" and "row". Row is always numeric, refering to a location in the buffer or on screen. Line is always an object... referring to the potential contents of a row. It would be cool if the terminal package followed the same convention... maybe it does already, not sure.

[mutle]

@nathansobo Thanks, I'll try imlementing it like you suggested and if I notice any performance issues I'll try doing it similar to what I did before. I'll also try to keep the line/row distinction in mind.

[mutle]

I implemented updates using events and nextTick, similar to what the Editor class does, in 6824edc and the first performance test looks very promising! I think this approach will work great going forward.

[mutle]

@nathansobo I seem to have one issue with this revamp: When you open multiple terminal sessions (using terminal:open) they seem to end up having the same view instance (or for some other reason showing the exact same content), with the exception that input doesn't work. My guess is that Atom is trying smart, so when opening a url like terminal://~/project twice it will use the same TerminalSession instance, just like when showing the same EditSession in multiple tabs. What is the best way to solve that problem and allow multiple TerminalSession instances with their own TerminalsViews? In ec51621dfc44d64a25dcdfce1cbf256ce3a52d20 I tried to change the url scheme to be unique but that didn't seem to help.

[nathansobo]

There's a couple issues here I think. One is that Atom currently won't open a duplicate resource for the same URI. We could add an option to open like allowDuplicates or something. The other is that currently we always recycle the same view instance when we have multiple resources of the same type. We just call setModel on it when switching to a new resource. This could be fixed by creating duplicate views in Pane if that particular view class does not possess a setModel instance method. Or you can make setModel work by updating the view when the model changes.

On Fri, May 31, 2013 at 10:55 AM, Mutwin Kraus notifications@github.comwrote:

@nathansobo https://github.com/nathansobo I seem to have one issue with this revamp: When you open multiple terminal sessions (using terminal:open) they seem to end up having the same view instance (or for some other reason showing the exact same content), with the exception that input doesn't work. My guess is that Atom is trying smart, so when opening a url like terminal://~/project twice it will use the same TerminalSession instance, just like when showing the same EditSession in multiple tabs. What is the best way to solve that problem and allow multiple TerminalSession instances with their own TerminalsViews? In ec51621https://github.com/atom/terminal/commit/ec51621dfc44d64a25dcdfce1cbf256ce3a52d20I tried to change the url scheme to be unique but that didn't seem to help.

— Reply to this email directly or view it on GitHubhttps://github.com/atom/terminal/pull/2#issuecomment-18757295 .