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

Improve naming and documentation around attribute data #2603

Closed
Tyriar opened this issue Dec 4, 2019 · 6 comments · Fixed by #4327
Closed

Improve naming and documentation around attribute data #2603

Tyriar opened this issue Dec 4, 2019 · 6 comments · Fixed by #4327
Assignees
Labels
type/debt Technical debt that could slow us down in the long run
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Dec 4, 2019

Current state:

/** Attribute data */
export interface IAttributeData {
fg: number;
bg: number;
clone(): IAttributeData;
// flags
isInverse(): number;
isBold(): number;
isUnderline(): number;
isBlink(): number;
isInvisible(): number;
isItalic(): number;
isDim(): number;
// color modes
getFgColorMode(): number;
getBgColorMode(): number;
isFgRGB(): boolean;
isBgRGB(): boolean;
isFgPalette(): boolean;
isBgPalette(): boolean;
isFgDefault(): boolean;
isBgDefault(): boolean;
// colors
getFgColor(): number;
getBgColor(): number;
}

Problems:

  • fg and bg are very ambiguous
  • getFgColor and getBgColor return a number, not an IColor
  • Can we improve getFgColorMode and getBgColorMode to return a type ColorMode = CM_DEFAULT | CM_P16 | CM_P256 | CM_RGB instead of number?
  • This whole interface could do with some detailed docs, especially since it's probably going to get exposed in xterm.d.ts in Add foreground/background color support for SerializeAddon #2369
@Tyriar Tyriar added the type/debt Technical debt that could slow us down in the long run label Dec 4, 2019
@Tyriar Tyriar self-assigned this Dec 4, 2019
@Tyriar Tyriar added this to the 4.4.0 milestone Dec 4, 2019
@JavaCS3
Copy link
Contributor

JavaCS3 commented Dec 5, 2019

If getFgColor, getBgColor returns an Object, it will decrease performance a little bit.

@Tyriar
Copy link
Member Author

Tyriar commented Dec 5, 2019

@JavaCS3 it would still be a number, just a more precise one:

type ColorMode = CM_DEFAULT | CM_P16 | CM_P256 | CM_RGB
// equivalent to this but with names:
type ColorMode = 0 | 0x1000000 | 0x2000000 | 0x3000000

@JavaCS3
Copy link
Contributor

JavaCS3 commented Dec 6, 2019

@JavaCS3 it would still be a number, just a more precise one:

type ColorMode = CM_DEFAULT | CM_P16 | CM_P256 | CM_RGB
// equivalent to this but with names:
type ColorMode = 0 | 0x1000000 | 0x2000000 | 0x3000000

Great

@jerch
Copy link
Member

jerch commented Dec 6, 2019

@JavaCS3 Just drop me a note if you have issue to restyle it (since I have created that convoluted mess).

@Tyriar
Copy link
Member Author

Tyriar commented Dec 6, 2019

@jerch I'm planning on doing this one, but we can carry the changes over to #2369 after

@jerch
Copy link
Member

jerch commented Dec 6, 2019

@Tyriar Ah ok, either way would work (and does not bloat #2369 with too much offtopic stuff).

@Tyriar Tyriar removed this from the 4.4.0 milestone Feb 3, 2020
@Tyriar Tyriar added this to the 5.1.0 milestone Dec 16, 2022
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/debt Technical debt that could slow us down in the long run
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants