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 bug in selection handling in dom renderer #4681

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

PerBothner
Copy link
Contributor

If you use the dom renderer and make a selection, the visible selection extends to the end of the line. This fixes that.

This also fixes a logic weirdo: A || (!A && B) should be simplified to A || B.

Comment on lines 167 to 174
&& (
(isInSelection && oldIsInSelection)
|| (!isInSelection && cell.bg === oldBg)
|| (!isInSelection && !oldIsInSelection && cell.bg === oldBg)
)
&& (
(isInSelection && oldIsInSelection && colors.selectionForeground)
|| (!(isInSelection && oldIsInSelection && colors.selectionForeground) && cell.fg === oldFg)
|| cell.fg === oldFg
)
Copy link
Member

@jerch jerch Aug 17, 2023

Choose a reason for hiding this comment

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

Suggestion - for the sake of readability lets separate concerns here:

  // selection state must not change
  && isInSelection === oldIsInSelection
  // outside of selections BG must match
  && (!isInSelection && cell.bg === oldBg)
  // default FG must match
  // exception: in selections selectionForeground may override FG
  && (cell.fg === oldFg || (isInSelection && colors.selectionForeground))

To me thats much easier to grasp than deep nested conditions, hopefully I covered all cases 😸.

The FG rule is still nested, at least it pulls the more likely case to the left, which should result in faster processing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That re-write seems to work fine my testing. And I'm inclined to agree it is cleaner. I was aiming for a minimal fix, but I'm happy also doing minor logic simplification.

(isInSelection && oldIsInSelection && colors.selectionForeground)
|| cell.fg === oldFg
)
// selection state must not change
Copy link
Member

Choose a reason for hiding this comment

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

We have multiple comments for the same lines here, we should either move these up, or change the top comments to // Check if cells can be merged and then inline the other comments

Comment on lines 168 to 173
&& isInSelection === oldIsInSelection
// outside of selections BG must match
&& (!isInSelection && cell.bg === oldBg)
// default FG must match
// exception: in selections selectionForeground may override FG
&& (cell.fg === oldFg || (isInSelection && colors.selectionForeground))
Copy link
Member

Choose a reason for hiding this comment

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

Tests are failing pointing out less optimal results, can we handle this case too?

  1) DomRendererRowFactory
       createRow
         selectionForeground
           should force whitespace cells to be rendered above the background:

      AssertionError: expected '<span style="background-color:#ff0000…' to equal '<span style="background-color:#ff0000…'
      + expected - actual

      -<span style="background-color:#ff0000;" class="xterm-decoration-top"> </span><span style="background-color:#ff0000;" class="xterm-decoration-top">a</span>
      +<span style="background-color:#ff0000;" class="xterm-decoration-top"> a</span>
      
      at Context.<anonymous> (src/browser/renderer/dom/DomRendererRowFactory.test.ts:319:16)
      at processImmediate (node:internal/timers:476:21)

Copy link
Member

@jerch jerch Aug 17, 2023

Choose a reason for hiding this comment

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

It breaks out of the merge here because FG is not set on the empty cell. I think this should fix it:

&& ((chars !== ' ' && cell.fg === oldFg) || (isInSelection && colors.selectionForeground))

This adds another merge chance - if there is no content in a cell, it can be merged regardless of FG.
Edit: Oh wait, not sure if FG is free of other SGR attributes, that would also show up...
Edit2: Nope that doesnt work, as INVERSE is also stored in FG. So this needs a slightly different patch. (Ideally we'd have on fg only fg-related attributes, well INVERSE deals with fg & bg stuff)

Copy link
Member

@jerch jerch Aug 17, 2023

Choose a reason for hiding this comment

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

Looking at it again, I think the simplifications are all slightly off because of the way we store SGR flags on bg and fg. In fact we'd need color inspection for the selection tests here. The difference in certain flags should still break out of the merge (otherwise we lose some SGR flag decorations during selection).
Thats the reason why I went with fg == oldFg && bg === oldBg in the first place as an opportunistic approach, that, if in doubt, falls back to single span rendering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe for now just go with my original minimal fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW In my buffer-cell-cursor branch I added to IAttributeData new accessor functions getFg and getBg which return combined getFgColor+getFgColorMode and getBgColor+getBgColorMode as a 26-bit int. I think that is a cleaner way of dealing with colors, with a new getStyleFlags function for the SGR flags and more (up to 28 bits). Maybe we can revisit further optimizations/cleanups later.

Copy link
Member

@jerch jerch Aug 17, 2023

Choose a reason for hiding this comment

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

The initial commit does look safer, fixes problems and passes the tests so that's cool with me.

Im pretty sure that INVERSE gets swallowed atm by selections (edit: and when a selecton fg is defined), its just that we dont have a test for it (not tested though, just guessing). But that might be fine? No clue whats expected to happen with INVERSE in that case...

Copy link
Member

Choose a reason for hiding this comment

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

Inverse when there's no selectionForeground won't get covered 🤯

Copy link
Member

@jerch jerch Aug 17, 2023

Choose a reason for hiding this comment

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

I guess we need to consider FgFlags.INVERSE for the background check and BgFlags.DIM for the foreground check? Not sure if extended underlines are handled. We need some failing tests to fix against.

Yeah I think we should cleanup the flag association a bit and put any font/content altering flag into fg.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@jerch jerch Aug 17, 2023

Choose a reason for hiding this comment

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

Can do the nasty flag rework (that affects quite some code places), but it has to wait, as I am still on Per's big grapheme PR (only halfway through so far 😺).

So for now I am fine if we add this PR as Per suggested initially.

@PerBothner
Copy link
Contributor Author

Not sure the best way to undo the last commit. Can I remove the last commit from the PR? Should I use a git revert - which will make a needless-complicted history - or can you clean that up on merge?

@jerch
Copy link
Member

jerch commented Aug 17, 2023

@PerBothner
If you want a clean repo log afterwards, imho revert + a force push should do.

@Tyriar
Copy link
Member

Tyriar commented Aug 17, 2023

Since we've decided what to do you can go either way. I don't normally care about many commits going in PRs as you can always revert by merge commit in the off chance we need to.

@PerBothner
Copy link
Contributor Author

@jerch "imho revert + a force push should do".

Did you mean git reset + git push --force? I did that, and it seemed to work. However, the tests reported failures, which I'm looking into.

@jerch
Copy link
Member

jerch commented Aug 17, 2023

Well its either:

git revert <commit to revert>
git push --force

or (HEAD~1 assumes to undo last commit):

git reset --soft HEAD~1
git push --force

@PerBothner
Copy link
Contributor Author

[I did a plain git reset 957e3e0f08606678be8e9346113b0aaefba4e433 followed by git push --force. After I cleaned up the local directoyr (git diff|git apply -R -) the tests passed.

@PerBothner
Copy link
Contributor Author

I think it's ready for merge now.

@Tyriar Tyriar added this to the 5.3.0 milestone Aug 17, 2023
@Tyriar Tyriar self-assigned this Aug 17, 2023
@Tyriar Tyriar merged commit 9d7b929 into xtermjs:master Aug 17, 2023
9 checks passed
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