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

Fix inverted cells, make sure inverted cells are cleared correctly #996

Merged
merged 4 commits into from
Sep 16, 2017

Conversation

mofux
Copy link
Contributor

@mofux mofux commented Sep 15, 2017

Fixes #995

Addresses two issues:

  1. Previously, the foreground was not correctly swapped, getting the same color as the background when inversed.

  2. Removes a render optimisation that caused the white top bar in nano not to render and also caused an issue when redrawing color inverted cells that contained a space.

@mofux mofux requested a review from Tyriar September 15, 2017 13:53
@@ -124,7 +124,7 @@ export class TextRenderLayer extends BaseRenderLayer {
if (flags & FLAGS.INVERSE) {
const temp = bg;
bg = fg;
fg = bg;
Copy link
Member

Choose a reason for hiding this comment

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

Oppsy 😄

@@ -94,7 +94,7 @@ export class TextRenderLayer extends BaseRenderLayer {
// Skip rendering if the character is invisible
const isDefaultBackground = bg >= 256;
const isInvisible = flags & FLAGS.INVISIBLE;
if (!code || (code === 32 /*' '*/ && isDefaultBackground) || isInvisible) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this optimization actually saves heaps of time, especially when the viewport is very wide as we ignore space cells very early in the process. Should this instead be:

if space char AND is default bg AND is not inverted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried that first, and it restores the white top bar in nano, but it fails when nano is closed:

const isInverted = flags & FLAGS.INVERSE;
if (!code || (code === 32 /*' '*/ && isDefaultBackground && !isInverted) || isInvisible) {
    continue;
}

screen shot 2017-09-15 at 18 00 09

Copy link
Member

Choose a reason for hiding this comment

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

Oh, you need to pull this off the _state, ie. was it inverted. Similar to what's happening above with (state[CHAR_DATA_ATTR_INDEX] & 0x1ff) >= 256, this means "was default background color". I think you need to add it to the condition above:

// Clear the old character was not a space with the default background
if (state && !(state[CHAR_DATA_CODE_INDEX] === 32 /*' '*/ && (state[CHAR_DATA_ATTR_INDEX] & 0x1ff) >= 256) && <!wasInverted>) 

Pulling state[CHAR_DATA_ATTR_INDEX] out into an oldAttr var will help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I've managed to fix it 😅

@Tyriar Tyriar added this to the 3.0.0 milestone Sep 15, 2017
@coveralls
Copy link

coveralls commented Sep 15, 2017

Coverage Status

Coverage remained the same at 70.123% when pulling cce440c on mofux:995 into 1f49879 on sourcelair:v3.

@@ -83,7 +83,8 @@ export class TextRenderLayer extends BaseRenderLayer {
}

// Clear the old character was not a space with the default background
if (state && !(state[CHAR_DATA_CODE_INDEX] === 32 /*' '*/ && (state[CHAR_DATA_ATTR_INDEX] & 0x1ff) >= 256)) {
const wasInverted = state && state[CHAR_DATA_ATTR_INDEX & FLAGS.INVERSE];
Copy link
Member

Choose a reason for hiding this comment

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

It should actually be:

const wasInverted = state && ((state[CHAR_DATA_ATTR_INDEX] >> 18) & FLAGS.INVERSE);

CHAR_DATA_ATTR_INDEX & FLAGS.INVERSE is equivalent to 0 & 8 which equals 0, so currently it will return the character code.

Broken down this is:

const attr = state[CHAR_DATA_ATTR_INDEX];
const flags = attr >> 18; // Bit shift right 18 bits
const wasInverted = !!(flags & FLAGS.INVERSE); // true if flags contains the bit for 8 (0b1000)

@coveralls
Copy link

coveralls commented Sep 16, 2017

Coverage Status

Coverage decreased (-0.02%) to 70.102% when pulling 4e88979 on mofux:995 into 1f49879 on sourcelair:v3.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 70.102% when pulling 642cdf0 on mofux:995 into 1f49879 on sourcelair:v3.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 70.102% when pulling 642cdf0 on mofux:995 into 1f49879 on sourcelair:v3.

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.

None yet

3 participants