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 support for "default" colors in ScreenBuffer #31

Closed
wants to merge 7 commits into from

Conversation

samizdatco
Copy link
Contributor

@samizdatco samizdatco commented Jun 3, 2017

Since all but one of the 32 attr bits were already in use, I expanded the attr size from 4 bytes to 6 and in the process reordered the 'colors' and 'flags' such that the colors are now in the upper 16 bits and the flags in the lower 16 (which ends up being more convenient for checking bitmasks since the bitwise operators all truncate Number values to 32 bits).

After adding this extra space to the middle of the attr field I assigned two new flag bits (at 17 & 18) that indicate whether a color and/or bgColor have been selected respectively. If they were left undefined (or passed a string value of "default"), the flag bits are unset. The escape-generating methods now consult these flags to decide whether colors should be set based on the color bits in attr or the terminal's 'default' fg/bg colors should be used.

Some Caveats

  • I added 16 bits to the attr size but am only using two of them at the moment so it's a bit on the wasteful side. Unless you can imagine a use for the rest of that space, it might make more sense to set ScreenBuffer.prototype.ATTR_SIZE to 5 rather than 6. I believe that the rest of the code should adapt without error if you just change this one value.

  • Since the generateDeltaEscapeSequence() method may need to set the fg and bg colors back to the 'default' value independently I added the defaultColor and bgDefaultColor escapes to term.optimized.

  • I incremented the version number on .sbuf files to 3 but ended up modifying the load- and saveSyncV2() methods to branch based on the version number in the header rather than creating brand new (and nearly identical) V3 methods. If keeping the historical serialization formats in the source is important to you, you might want to paste the old versions back in.

  • I used the demo/spaceship.js script as my main test-case and made some minor edits to it in places where it was relying on a black bgColor being present by default.

  • I wasn't quite sure if the object2attr and attr2object definitions were repeated intentionally or not (since the implementations appeared identical). For now I'm just assigning the same function to both the 'class' and 'instance' methods...

- color & background are now the first/highest 2 bytes
- style & transparency flags are the final/lowest 2 bytes
- the middle 2 bytes are currently unused
- added 2 new flags to the attrs bitfield that are set when a `color`
or `bgColor` is assigned explicitly
- if color values aren’t provided (or are set to the string value
“default”), the terminal’s default colors will be used
- terminal.optimized now exposes the defaultColor and bgDefaultColor
functions
- rather than shifting by a fixed number of bits to access the colors
in the attr block, shift based on the ATTR_SIZE constant
@cronvel
Copy link
Owner

cronvel commented Jun 4, 2017

@samizdatco Like I said in the related issue: thanks for your contribution, but unfortunately I was already working on that.
I'm trying to avoid a SemVer Major change and aim for a backward compatible change.

I have cherry-picked "delete previous/next word keybindings" and pushed version 1.9.0 with your change to .inputField(). Thanks! ;)

@cronvel
Copy link
Owner

cronvel commented Jun 4, 2017

@samizdatco I've implemented it. It landed on v1.10.0.
I've done it so it does not break the compatibility.
I removed an unimplemented flag that was reserved for further usage, but I think that this flag would be equivalent to some combination of existing flags (mainly transparency flags).

@cronvel cronvel closed this Jun 4, 2017
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.

2 participants