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

Add initial decorations API #3302

Closed
wants to merge 4 commits into from
Closed

Conversation

akariv
Copy link

@akariv akariv commented Apr 8, 2021

Following the discussion in #3274

Here's a simple decorations API which adds the ability to add custom 'decorations' on top of the terminal layers, without interfering with the buffer, selections or the cursor.

API is as follows:

export interface IDecorationService {
  serviceBrand: undefined;

  addDecoration(element: IDecorationElement): IDecorationHandle;
  removeDeoration(handle: IDecorationHandle): boolean;
  forEachDecoration(callback: (decoration: IDecorationElement) => boolean) : void;
  clearDecorations(): number;
}

export type IDecorationHandle = number;
interface IDecorationElement {
  startColumn: number;
  endColumn: number;
  startRow: number;
  endRow: number;
  fillStyle: string | CanvasGradient | CanvasPattern;
  strokeStyle: string | CanvasGradient | CanvasPattern;
}

This is my first contribution to this code base so please review carefully :)


Part of #1852

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Very cool 🙂

Comment on lines +458 to +459
fillStyle: string | CanvasGradient | CanvasPattern;
strokeStyle: string | CanvasGradient | CanvasPattern;
Copy link
Member

@Tyriar Tyriar Apr 8, 2021

Choose a reason for hiding this comment

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

I was thinking to make this as flexible as possible we would just give correctly positioned DOM elements to the user and the decorations system would be completely separate from the renderers. While we would sacrifice some performance here, we would get a lot of flexibility and ease of development since we don't need to implement it separately for webgl and DOM as well.

Copy link
Author

Choose a reason for hiding this comment

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

So just to make sure I understand you correctly - addDecoration would return an Element (with position: absolute under the Terminal's element), in which the user would be able to add any content they desire. DecorationsService would then manage the top/left/visibility options for each element. Correct?

Copy link
Member

Choose a reason for hiding this comment

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

Yep that's right.

visibility options

I imagine it would detach/reattach decorations that have scrolled out/in of the viewport.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO both cases should be considered.

Inline Decoration
I think the decoration should be able to provide an inline cell style via an API (which allows to set / overwrite the background, foreground, italic, bold, underline styles). The renderer should be responsible for rendering this type of cell modification. A use-case for this kind of decoration is the rendering of underlines for Hyperlinks.

Overlay Decoration
In addition to that, a decoration should also allow to attach a custom HTML element to it. In this case the HTML Element will be rendered in a separate HTML Layer that is synchronised with the viewport. The HTML Element will by default resize to the selected range. A use-case for this kind of decoration is the hover overlay (Follow Link) when hovering a Hyperlink. This would also be the preferred choice to solve #3274

Copy link
Member

Choose a reason for hiding this comment

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

We could always add inline decorations after if we had the time to invest by adding a type? in a backwards compatible way. I like the idea of eventually being able to change both text and underline color for link hovers like how the editor in VS Code does it. Perhaps going with an IDecorationOptions is more forward thinking:

interface IDecorationOptions {
  range: IBufferRange;
  type?: a | b;
}
registerDecoration(options: IDecorationOptions): IDecoration;

public removeDeoration(handle: IDecorationHandle): boolean {
return this._core.removeDeoration(handle);
}
public clearDecorations(): number {
Copy link
Member

Choose a reason for hiding this comment

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

Don't think we need a return type here?

Copy link
Author

Choose a reason for hiding this comment

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

Sort of success value for removal - I can remove it.

Comment on lines +178 to +183
public addDecoration(element: IDecorationElement): IDecorationHandle | undefined {
return this._core.addDecoration(element);
}
public removeDeoration(handle: IDecorationHandle): boolean {
return this._core.removeDeoration(handle);
}
Copy link
Member

Choose a reason for hiding this comment

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

We've started moving towards returning IDisposables for things like this which makes it very convenient to remove the decoration. So something more like this is what I think we should go with:

registerDecoration(range: IBufferRange): IDecoration;
interface IDecoration extends IDisposable {
  // This uses a setter and is implemented by the core so changes can
  // be applied immediately
  range: IBufferRange;
  // This will give ultimate flexibility, perf will probably be fine unless the terminal is covered in decorations
  readonly element: HTMLElement;
}

Copy link
Author

Choose a reason for hiding this comment

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

👍

@@ -175,6 +175,16 @@ export class Terminal implements ITerminalApi {
public paste(data: string): void {
this._core.paste(data);
}
public addDecoration(element: IDecorationElement): IDecorationHandle | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

The actual API is declared in xterm.d.ts so the new interface will need to go there.

@Tyriar
Copy link
Member

Tyriar commented Oct 22, 2021

Closing out as this is stale and needs a pretty big rework

@Tyriar Tyriar closed this Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants