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

Added Terminal.getRangeAsHTML() #2223

Closed
wants to merge 2 commits into from
Closed

Conversation

Eugeny
Copy link
Member

@Eugeny Eugeny commented Jun 14, 2019

fixes #1883

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.

I think this would fall into the same category as #2213 where we should defer this to an extension in order to keep the core slim. In order to do both properly we would need some way of exposing the color on IBufferCell if you want to propose an API for that that we can discuss?

@Eugeny
Copy link
Member Author

Eugeny commented Jun 14, 2019

I suppose you could expose methods to obtain cell attributes in BufferLineApiView?

@jerch
Copy link
Member

jerch commented Jun 14, 2019

How about this:

interface ... {
  // flags
  bold: boolean;
  underline: boolean;
  ...
  // colors
  fgColorMode: ColorModeEnum;  // enum reflecting CM_P16, CM_P256 and CM_RGB
  fgColor: number; // actual color number (-1 means default color)
  fgColorHex: string; // property for hex repr (empty string means default color)
}

Imho the number color value is needed to avoid costly number ---> hex conversions for every cell when comparing the values. (Maybe the same goes for the flag numbers, not sure).

Edit: If we expose only final hex colors from the current theme, we can omit the colorMode, and maybe the number value for color.

Edit2:
Adopted to the current API interface it might look like this:

interface IBufferCell {
  readonly char: string;
  readonly width: number;
  readonly color: IBufferCellColor;
  readonly flags: IBufferCellFlags;
}
interface IBufferCellColor {
  readonly fg: string;
  readonly bg: string;
}
interface IBufferCellFlags {
  readonly bold: boolean;
  readonly underline: boolean;
  ...
}

Edit3:
Lol 3rd edit - I think omitting the colorMode is no good idea as it will not work for an escape sequence output needed for #2213.

@Tyriar
Copy link
Member

Tyriar commented Jun 14, 2019

We could use union types for the color?

interface IBufferCell {
  readonly color: BufferCellColorAnsi16 | BufferCellColorAnsi256 | BufferCellColor24Bit;
}

@jerch
Copy link
Member

jerch commented Jun 14, 2019

@Tyriar Sure, just wondering how each type would look like?

@Tyriar
Copy link
Member

Tyriar commented Jun 14, 2019

Something like this?

interface IBufferCell {
	readonly color: IBufferCellColorAnsi16 | IBufferCellColorAnsi256 | IColor;
}

interface IBufferCellColorAnsi16 {
	ansi16Code: number;
	actualColor: IColor;
}

interface IBufferCellColorAnsi256 {
	ansi256Code: number;
	actualColor: IColor;
}

interface IColor {
	red: number;
	green: number;
	blue: number;
	hex: string;
}

@Tyriar
Copy link
Member

Tyriar commented Jun 14, 2019

Alternatively:

interface IBufferCell {
	readonly colorMode: ColorMode;
	/**
	 * Ansi16: 0-15
	 * Ansi256: 0-255
	 * TrueColor: 0 (or undefined?)
	 */
	readonly colorCode: number;
	readonly color: IColor;
}

enum ColorMode {
	Ansi16,
	Ansi256,
	TrueColor
}

interface IColor {
	red: number;
	green: number;
	blue: number;
	hex: string;
}

@jerch
Copy link
Member

jerch commented Jun 14, 2019

@Tyriar Hmm maybe the second one is easier to work with? For the first one I see troubles to distingish the color modes - this would only work by instanceof check or by explicit member checks against ansi16Code etc?

@Tyriar
Copy link
Member

Tyriar commented Jun 14, 2019

Yeah I'm leaning towards the second one too, moved this to #2225 and #2226

Let's close this off for now as the solution would look pretty different to this PR. @Eugeny would love your help on those APIs if you have bandwidth 😃

@Tyriar Tyriar closed this Jun 14, 2019
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.

Copying HTML-formatted output
3 participants