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

Allow embedders to decorate ranges in the buffer #1852

Closed
Tyriar opened this issue Dec 21, 2018 · 50 comments · Fixed by #3630
Closed

Allow embedders to decorate ranges in the buffer #1852

Tyriar opened this issue Dec 21, 2018 · 50 comments · Fixed by #3630
Assignees
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Dec 21, 2018

Creating this to centralize discussion on @mofux's idea to support monaco-like decorations as discussed in #1380 (comment) and #791 (comment).

@Tyriar Tyriar added type/enhancement Features or improvements to existing features area/api labels Dec 21, 2018
@Tyriar
Copy link
Member Author

Tyriar commented Dec 27, 2018

Was just thinking, once #1176 is done if we had some view part that could be inserted then the 1337 iterm sequence to insert an image could be completely implemented within an addon.

@thesoftwarephilosopher
Copy link
Contributor

thesoftwarephilosopher commented Nov 27, 2019

@Tyriar I'm particularly interested in an xterm.js feature where plugins can get DOM elements added into the "flow" of the terminal grid (and maybe even between rows), which the plugin can then completely control by appending interactive elements into it. Thanks to your comment to the issue I just opened, I found and have been reading up on a lot of discussion between you, @mofux, @jerch, and @PerBothner, and I see that xterm.js is very well positioned to be able to add this feature, but that it's not quite there yet. I may be able to allocate several days or perhaps even weeks of full time work towards helping to implement this feature within xterm.js, pending we can come up with a reasonable and concrete plan, and can come up with a way to coordinate effectively on it. Please let me know your thoughts. Thanks for your time :)

@Tyriar
Copy link
Member Author

Tyriar commented Nov 28, 2019

@sdegutis glad to hear you're interested in contributing! It's been a while since we talked about this but here are some thoughts:

  • We probably don't want to get into the business of inserting elements in between rows as that would involve a lot of changes in how things are rendered and also mess with the interactive viewport at the bottom of the buffer
  • I'm not too familiar with the Monaco decoration API but it's tried and true and worth taking inspiration from. Note that unlike in Monaco, we don't necessarily have DOM elements for the cells (depends on rendererType)
  • I'm not sure what the right behavior would be when rows/cell with decorations get resized and get wrapped, I guess the embedder is responsible for listening to Terminal.onResize and reacting/recreating for that as appropriate

Given this, my current thinking is that we could maybe have a couple of flavors of decorations:

  • Arbitrary DOM elements whose positioning is handled by xterm.js, for example you could use an <img> to decorate some cell, when the user scrolls the viewport then xterm.js will position that (with the help of Markers). This type would probably need an extra container with overflow:hidden such that a decoration could span multiple rows.
  • Changing the background color of a cell, this would need to be handed over to the renderer internally to override the regular background color so it's drawn underneath the text.

FYI my main use case for wanting this is to enable visual indicators for VS Code's command tracking feature where it flags rows as have run a command. This would include a static indicator (Terminal.app uses a small [ icon in a gutter on the left of the cell) and flag a row by changing its background color and fading back briefly when navigating to commands.

@thesoftwarephilosopher
Copy link
Contributor

thesoftwarephilosopher commented Nov 29, 2019

@Tyriar Hmm I think what I'm looking for may be slightly different than this issue then. What you're describing sounds a lot more like Monaco's deltaDecorations API (example), whereas what I want is closer to their addContentWidget functionality.

Specifically, what I'd like to do is add an empty <div> (which the user will be able to add more elements to) over a rectangle of the grid, as if it were part of the character grid itself, so that it scrolls when the rest of the grid scrolls.

I didn't realize they meant two different things until I looked closer at Monaco's API and tried them both out in their playground. I'm still having some difficulties getting this working, but I can explain more in a new issue for widgets rather than this one for decorators, unless you think maybe there's enough implementation/feature overlap between the two that they could be combined.

@jerch
Copy link
Member

jerch commented Nov 29, 2019

@sdegutis I think you want something like the "grid-holes" idea here. This is a bad idea in terms of compliance to the current terminal interface (mainly for cursor and viewport issues) and portability to other emulators in general. If you really need this, I fear it wont make into xterm.js itself due to these incompatibilities (thus only possible as your own xterm.js addon).

The second idea there ("make room in the grid") is conform to the terminal interface, maybe this would work for you. Ofc this cannot be extended after creation, thus no user appends possible or even freely resized. Prolly not what you are looking for.

@thesoftwarephilosopher
Copy link
Contributor

@jerch That's a similar idea but my plan involved making sure that the process "made room" for the widget, and currently widgets are full-width (e.g. width: 100% in CSS), so "making room" just means the process has to print "\n\n\n\n" for a 4-row-tall widget. There are strange questions like "what happens if the user moves the cursor onto a part of the grid where the widget is" but I think those can semantically be figured out...

@jerch
Copy link
Member

jerch commented Nov 29, 2019

@sdegutis Could you give a short explanation what you want to place there (content type and such)? With a terminal there are several things to consider, esp. security-wise.

Example:
If you want to place there HTML content retrieved from terminal side, the HTML actually gets loaded in the host machine context. This will earn you an CVE, if not clearly stated or cannot be disabled (for unexpectedly "executing" remote data locally). Furthermore you would need to secure the inserted HTML (basically no JS allowed).

@thesoftwarephilosopher
Copy link
Contributor

@jerch I had in mind built-in widgets that the terminal comes with, which the calling process could "activate", i.e. make it appear in the terminal's grid. Thus it's a sort of whitelist of widgets that already have been vetted for security.

I have this already working for the most part, but I'm having surprising amount of difficulty getting scrolling to work, due to how xterm.js does its scrolling and how I'm adding my widgets to xterm's own root term.element. That's mostly what I'm stuck on solving externally.

@jerch
Copy link
Member

jerch commented Nov 29, 2019

@sdegutis Ah ok, well security aside, this sounds doable by an additional output layer. I have similar plans for an image layer to get image output working someday. Maybe have a look here, I have no scrolling issues:

private _drawToCanvas(imgId: number, tileId: number, col: number, row: number): void {

@PerBothner
Copy link
Contributor

(Slightly off-topic - but it gets relevant ...)

I've been thinking about how one could support variable-width fonts in a terminal. For some (human) languages using a single-width (or even duo-width with wide characters) is awkward and/or considered very ugly. Even for English it's considered ugly - note this "issue" page uses variable-width fonts!

It wouldn't take much to extend the traditional terminal model to support variable-width "character cells". Instead of a rectangular grid of cells, you have a list of lines, each consisting of a variable number of variable-width cells. Special escape sequence could be used to move around in this mode. I'm not going into details here, but there are some non-obvious advantages. For example the application and terminal don't have to agree on which characters are double-width, as cursor positioning would be done by character (or glyph), rather than cell. When terminal width is changed, the terminal (rather than the application) would handle re-break/re-wrapping. (The application cannot know the width the terminal will have when the re-display is done.)

Once you have variable-width characters, you might also allow variable-height characters, and thus also variable-height lines. (The height of a line is the height of the tallest character in a line.)

Back to the topic at hand: Once you have variable width and height of a character, you can use a "pseudo-character" that consists of an image. Or you can combine a character with an annotation, including a custom "paint" rule. So this gives you extreme flexibility for "decorating" a character.

Note this isn't terribly difficult to implement - and it need not break the traditional model. DomTerm mostly already supports variable-size characters and lines, while still keeping very solid xterm compatibility. The main thing missing is to specify escape sequences to make it easier for an application to benefit from this model. I'm considering modifying fish shell to take advantage of such a mode (since I already did some work on fish redisplay). (The main changes to fish would be to treat the terminal as infinite-width, delegating line-breaking to the terminal; no special handling for double-width characters; ignore sigwinch.)

@Tyriar
Copy link
Member Author

Tyriar commented Nov 29, 2019

That's a similar idea but my plan involved making sure that the process "made room" for the widget, and currently widgets are full-width (e.g. width: 100% in CSS), so "making room" just means the process has to print "\n\n\n\n" for a 4-row-tall widget.

@sdegutis This is what I was thinking in my little write up; decorating using DOM nodes would be unrestricted. Then an outer container the size of the terminal would use overflow:hidden such that it can be rendered above and part of it would be cut off:

****** < div is drawn above viewport because it's taller than 1 row
******
---------- < top of viewport
******

---------- < bottom of viewport

@thesoftwarephilosopher
Copy link
Contributor

@PerBothner that's an interesting approach. I'm not sure it's compatible with the other goals I have, but in general interesting. I think an easier middle ground for my purposes is to just "overlay" a widget on top of the grid of fixed-size-characters. But I really like the innovative thinking you have going on and have been following DomTerm with plenty of interest.

@thesoftwarephilosopher
Copy link
Contributor

@Tyriar Oh ok I must have misunderstood at first. Yeah I think that would work well. I have something similar going on right now, that allows me to do stuff like this:

Screen Shot 2019-11-29 at 2 11 22 PM

The problem I have right now is related to scrolling. I'll check out how @jerch does it but the summary of the bug I'm seeing is that, when the terminal is large enough to scroll, then hovering over the widget and trying to scroll will get "hijacked" by the terminal which then scrolls instead. This is when adding the widget to xterm.js via term.element.append(widget). I've been looking into how xterm.js implements scrolling but don't see anything unusual that would cause this, aside from the fact that .xterm-scroll-area is inside xterm-viewport which comes before xterm-screen in the DOM, so it confuses me how it gets scroll events instead of them being sent to xterm-screen or something inside it -- which points to me just really not understanding at all how xterm.js gets scrolling to work and how I can get my element to scroll with it.

@Tyriar
Copy link
Member Author

Tyriar commented Nov 29, 2019

@sdegutis if you provide the element can't you just add a higher z-index to make sure it's on top? The scroll stuff is a little weird as we're kind of hacking it a bit to get a native scroll bar on a virtualized container (the terminal buffer).

@thesoftwarephilosopher
Copy link
Contributor

@Tyriar That doesn't work for some reason. I had an idea though this morning for an alternative scrolling technique to get native scrollbars that might help fix my problems: what if you just grow the <canvas> along with the scrollable container, which it would then become a child of, and only draw the visible portion of it based on where the container is scrolled to? So we'd have something like:

|--scrollview-and-canvas--|
|                         |
|--viewport---------------|
| (only this is drawn)    |
|                         |
|                         |
|                         |
|-------------------------|
|                         |
|-------------------------|

@Tyriar
Copy link
Member Author

Tyriar commented Nov 30, 2019

@sdegutis you can't grow a canvas that big as I think you eventually (and quickly) hit the upper limit of the GPU texture size. If we did anything with scrolling I'd want to move it closer to a real custom scroll bar (integrating ScrollableElement is possible). Also there's extra overhead, more layouts, more drawing, etc. that would happen when you scroll.

@thesoftwarephilosopher
Copy link
Contributor

Ah good point.

@jerch
Copy link
Member

jerch commented Dec 2, 2019

offtopic:
@PerBothner This sounds interesting. I like the idea to think into this direction, the current model already carries way to much ifs and elses due to the fix width idea (esp. for wide chars and wrapping lines in general, both are cumbersome to deal with). Could this be made compatible with very low interfaces (like an early terminal driver during bootup directly sitting on video RAM/framebuffer)? Imho it should keep working with these low level interfaces as kinda the whole terminal interface is derived from that simplicity. Historically we had a differentiation between dumb terminals with very reduced capability set (mostly bound to the functionality of real ttys) and somewhat more intelligent ones (I'd count any VT100+ into the latter). Not sure how far the OS layers have evolved, I think all line disciplines are still stuck at the dumb level in ICANON mode* and try to anticipate the width and char handling according to the width. This is somewhat questionable, but changing it is not easy as it will raise tons of compat questions (the ldisc is used for many driver interactions). Maybe extending the TTY spec will do, something like an advanced ICANON2 mode without width notion, were the terminal is free to handle wrapping stuff on its own. This would need several additional sequences (and prolly ioctls) to be properly announced to the terminal (driver/emulator) though. Also giving up control of exact output representation needs more thinking on how to deal with that from app side.

[*] Why caring for ICANON at all? Well its true advanced apps will most likely switch right into the "single byte sink mode", still most simpler cmdline tools are bound to the line buffering mode. Those could only benefit from changes in line/width handling, if it would be changed on termios level. To avoid changes on that level, also a userland lib + ~ICANON could be used (much like using libreadline currently), maybe thats easier to accomplish (the lib usage would turn them into "advanced apps").

@thesoftwarephilosopher
Copy link
Contributor

So maybe vim just redraws everything when switching back to it?

I think this is what happens.

Just checked, yeah it does, never mind most of my tangent. But I guess the one thing to take away is that means decorations must be expected to be destroyed on buffer changes, at least in one direction, and so they should be encouraged to be stateless maybe?

@Tyriar
Copy link
Member Author

Tyriar commented Dec 6, 2019

Either way the concept is the same: when you scroll inside the widget

Scrolls fine for me 🤷‍♂

But it should be the same even when you scroll with an old mouse as long as it can scroll by at least some arbitrary amount of pixels?

On Windows a scroll "notch" scrolls 100px on Chrome which is used as an estimate to the default Windows setting of 3 lines, and a variable amount of Edge due that feature.

@thesoftwarephilosopher
Copy link
Contributor

Either way the concept is the same: when you scroll inside the widget

Scrolls fine for me 🤷‍♂

Hmm. The red widget scrolls internally? In the second demo? In Chrome?

But it should be the same even when you scroll with an old mouse as long as it can scroll by at least some arbitrary amount of pixels?

On Windows a scroll "notch" scrolls 100px on Chrome which is used as an estimate to the default Windows setting of 3 lines, and a variable amount of Edge due that feature.

Oooh, so that probably explains why #1140 isn't as popular of a feature request as I'd thought it'd be -- people on Windows with regular mice don't really have the possibility of pixel scrolling.

@thesoftwarephilosopher
Copy link
Contributor

This is the behavior I get, trying to scroll inside the box:

ScreenRecording2019-12-06at12022

This is with idea2.html.

@Tyriar
Copy link
Member Author

Tyriar commented Dec 6, 2019

@sdegutis oh I see that now, maybe I had the first one opened. That's pretty weird behavior, don't you get the event in the decoration first and you can cancel it?

@thesoftwarephilosopher
Copy link
Contributor

thesoftwarephilosopher commented Dec 6, 2019

@Tyriar It doesn't get any scroll events. Tried by adding:

    widget.addEventListener('scroll', (e) => {
      console.log(e);
    });

(Hey how come you can type

xterm.js/typings/xterm.d.ts

Lines 418 to 422 in bd0d267

/**
* (EXPERIMENTAL) Get the parser interface to register
* custom escape sequence handlers.
*/
readonly parser: IParser;
and github shows a widget but I type https://github.com/sdegutis/xterm-widget-test-case/blob/107d7131ec53df8456c919cf4efd764a40782d4b/idea2.html#L95-L97 and it just shows a link? Does it have to be in the same project to get a widget? Or maybe it's a setting in each project to allow widgetifying?)

@Tyriar
Copy link
Member Author

Tyriar commented Dec 6, 2019

@sdegutis the viewport listens to scroll, shouldn't it's children get it first? 😕

this.register(addDisposableDomListener(this._viewportElement, 'scroll', this._onScroll.bind(this)));

@thesoftwarephilosopher
Copy link
Contributor

That's what I thought too. Was hoping there was some special thing you guys are doing and that I just didn't know about it. But I guess it's a mystery to all of us 😄 Looking into it now...

@thesoftwarephilosopher
Copy link
Contributor

thesoftwarephilosopher commented Dec 6, 2019

I don't see anything strange in Lifecycle.ts except that useCapture defaults to undefined, yet is still being passed. But that probably shouldn't matter... I think.

@Tyriar
Copy link
Member Author

Tyriar commented Jan 26, 2022

We're planning on looking into this for Feb, there's 3 main ideas of what this encompasses so far:

  • Buffer decorations (the main request)
  • Gutter decorations (show things to the left of the buffer so it's not so cramped)
  • Monaco-style scroll bar annotations for the decorations. We don't need to get as granular as monaco allows, but this is the basic idea
    image

@Tyriar Tyriar added this to the 4.18.0 milestone Jan 26, 2022
@meganrogge
Copy link
Member

meganrogge commented Jan 31, 2022

We're thinking to support decorations in the buffer, scrollbar and gutter (in that order).

I have started working on the buffer decorations and am seeking feedback on this first pass at the API:

  /**
     * (EXPERIMENTAL) Adds a decoration to the terminal using
     *  @param decorationOptions, which takes a marker and an optional anchor, 
     *  width, height, and x offset from the anchor
     */
    registerDecoration(decorationOptions: IBufferDecorationOptions): IDecoration | undefined;
/**
   * Represents a disposable with an 
   * @param onDispose event listener and
   * @param isDisposed property.
   */
  export interface IDisposableWithEvent extends IDisposable {
    /**
     * Event listener to get notified when this gets disposed.
     */
    onDispose: IEvent<void>;

    /**
     * Whether this is disposed.
     */
    readonly isDisposed: boolean;
  }

  /**
   * Represents a decoration in the terminal that is associated with a particular marker and DOM element.
   */
  export interface IDecoration extends IDisposableWithEvent {
    /*
     * The marker for the decoration in the terminal.
     */
    readonly marker: IMarker;

    /**
     * An event fired when the decoration
     * is rendered, returns the dom element
     * associated with the decoration.
     */
    onRender: IEvent<HTMLElement>;

    /**
     * The HTMLElement that gets created after the 
     * first _onRender call, or undefined if accessed before
     * that.
     */
    element: HTMLElement | undefined;
  }

  export interface IBufferDecorationOptions {
    /**
     * The line in the terminal where
     * the decoration will be displayed
     */
    marker: IMarker;

    /*
     * Where the decoration will be anchored -
     * defaults to the left edge
     */
    anchor?: 'right' | 'left';

    /**
     * The width of the decoration, which defaults to 
     * cell width
     */
    width?: number;

    /**
     * The height of the decoration, which defaults to 
     * cell height
     */
    height?: number;

    /**
     * The x position offset relative to the anchor
     */ 
    x?: number;
  }

Here's what we're thinking for the gutter decorations:

    export interface IGutterDecorationOptions {
      /**
       * The line in the terminal where
       * the decoration will be displayed
       */
      startMarker: IMarker;
      /**
       * The end line in the terminal for
       * the decoration
       */
      endMarker: IMarker;
    }

@Tyriar
Copy link
Member Author

Tyriar commented Jan 31, 2022

registerDecoration(decorationOptions: IBufferDecorationOptions): IDecoration | undefined;

Could callout when undefined happens in the jsdoc

onDispose: IEvent;
onRender: IEvent;
element: HTMLElement | undefined;

These should be readonly since xterm.js creates them.

width?: number;
height?: number;

Maybe stick to 1x1 now and consider this down the line? If we did have this I think specifying the width and height in cells would make the most sense.

@Eugeny
Copy link
Member

Eugeny commented Jan 31, 2022

Specifying the width (in cells, not pixels) is a must-have for keyword highlighting, prompt decoration and other applications.

@Tyriar
Copy link
Member Author

Tyriar commented Feb 10, 2022

We decided against the gutter decoration idea as you can just use an x=0 decoration and offset it. For decorations to show up in scroll bar I created #3632

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants