Retain the color of a character for underline and bar cursor styles #781

mofux · 2017-07-11T19:22:46Z

Fixes #774
Fixes #789
Fixes #790
Fixes #820
Related #783

⚠️ Warning
I had to change the cursor related css to get this feature in, and to resolve styling related issues. Consumers that have overwritten the default styles may have to adopt their stylesheet.

🚀 Improvements
The.terminal element will always have a class with the cursor style, previously, only bar and underline cursors had a class.

🐞 Fixes

Retain the color of a character with the cursor on it, for underline and bar cursor styles #774, Line and underline terminal cursors show as a block when unfocused #783: bar and underline cursors now render correctly and do not alter the foreground color of the text,
Retain the color of a character with the cursor on it, for underline and bar cursor styles #774: blink is now working correctly if the block cursor is over a background,
Terminal focus class is lost under certain conditions #789: focus class is kept when interacting with programs that support mouse pointer integration
Terminal is initialised with the wrong cursor style #790: The cursor style passed through options is now respected
Underline and blink cannot be rendered in parallel #820: Allow blink and underline flags to be rendered in parallel

🎮 Testing
I found it very useful to nvim README.md in the xterm.js demo because the Markdown syntax highlighter of nvim will create almost all visual test cases (background, foreground, underline, bold).

coveralls · 2017-07-11T19:54:01Z

Coverage decreased (-0.06%) to 69.388% when pulling 0d570cd on mofux:cursor_color into 86c5aa0 on sourcelair:master.

Tyriar · 2017-07-11T19:57:49Z

@mofux we don't want to use the same color when it's a block cursor, instead we want to use the terminal's background color. Is this case covered by this?

mofux · 2017-07-11T20:24:39Z

@Tyriar I think you mean we want the foreground color, not the background color? Yes, the css rule takes precedence.

coveralls · 2017-07-11T20:30:55Z

Coverage decreased (-0.06%) to 69.388% when pulling 7869440 on mofux:cursor_color into 86c5aa0 on sourcelair:master.

mofux · 2017-07-11T20:33:05Z

I've just stumbled over a couple of issues while testing:

In the unfocused state, the block outline is always drawn, no matter which cursor style is selected. What should be the preferred look of the those cursor styles in the unfocused state?
The line and underline cursors also need the background color, otherwise there will be a black square whenever the cursor is over a background (fix is on the way)
When I open midnight command mc or nvim and perform a click, the focus class on the terminal is removed for some reason. It only comes back after I close the app. I believe this happens if the program supports mouse pointer integration. I'll try to investigate...

coveralls · 2017-07-11T21:07:11Z

Coverage decreased (-0.1%) to 69.333% when pulling 7b5bd20 on mofux:cursor_color into 86c5aa0 on sourcelair:master.

Tyriar · 2017-07-11T21:08:13Z

@mofux

Yeah I knew about this but I don't think there is an issue for it. gnome-terminal just shows the line/underline solid when unfocused, Terminal.app acts as it is now. I think aligning with gnome-terminal here is ideal. Created Line and underline terminal cursors show as a block when unfocused #783
👍
Does it affect the cursor though? Most styles are applied to :focus and .focus

mofux · 2017-07-12T16:17:33Z

This PR is ready for review. Please see the PR description for updated details.

coveralls · 2017-07-12T16:51:50Z

Coverage decreased (-0.2%) to 69.263% when pulling 8043c11 on mofux:cursor_color into 01453e9 on sourcelair:master.

coveralls · 2017-07-12T17:24:07Z

Coverage decreased (-0.7%) to 68.682% when pulling 2db0ac8 on mofux:cursor_color into 01453e9 on sourcelair:master.

mofux · 2017-07-13T00:25:56Z

@Tyriar I can see this running into conflicts with #756. I propose to hold this back until #756 is merged. Once it's in I'll update this PR.

If you want, you could already include the changes made here (804051e) into #756, which should solve #774.

Tyriar · 2017-07-13T02:11:09Z

@mofux fill hopefully get to this in the next couple of days, sorry about the delay

coveralls · 2017-07-13T02:21:58Z

Coverage decreased (-0.2%) to 69.37% when pulling 0a3b221 on mofux:cursor_color into 25fc01c on sourcelair:master.

coveralls · 2017-07-14T18:25:12Z

Coverage decreased (-0.2%) to 69.292% when pulling 855f73a on mofux:cursor_color into c94fdae on sourcelair:master.

Tyriar · 2017-07-14T18:32:16Z

src/Renderer.ts

              currentElement.classList.add('reverse-video');
              currentElement.classList.add('terminal-cursor');
+              if (cursorFg >= 0 && cursorFg < 256) {


I think we actually want to do more here as well, bold, underline, invisible (maybe?). Perhaps the cursor part should try share more code with the regular part?

Some things don't make sense here: blink, inverse

And some things need to be done differently: bg/fg application

Tyriar · 2017-07-14T18:32:45Z

src/Renderer.ts

        const ch = line[i][1];
        const ch_width: any = line[i][2];
        if (!ch_width) {
          continue;
        }

        if (i === x) {
+          cursorData = [data & 0x1ff, (data >> 9) & 0x1ff];


On the topic of sharing code, it would be good to reuse this line:

let bg let fg = (data >> 9) & 0x1ff;

Tyriar · 2017-07-14T18:33:17Z

src/Renderer.ts

@@ -168,13 +168,15 @@ export class Renderer {
      for (let i = 0; i < width; i++) {
        // TODO: Could data be a more specific type?
        let data: any = line[i][0];
+        let cursorData;


Perhaps this should go outside the look and instead be let isCursor = false;

Tyriar · 2017-07-14T18:34:48Z

src/xterm.js

@@ -1044,6 +1045,9 @@ Terminal.prototype.bindMouse = function() {
  }

  on(el, 'mousedown', function(ev) {
+    // prevent the focus on the textarea from getting lost
+    ev.preventDefault();


I think this might need to go after the self.mouseEvents check? Can you verify by running tmux, vim, etc. in mouse mode and see if they get the events?

It's good to have it at the start, because we will want to prevent the focus of the textarea from being lost in either case. I have tested it in nvim and mc and it works as expected. I've also made sure it doesn't cause a regression for the selection manager and linkifier.

I think we can and should be removed once #828 is merged, because #828 adds a focus() at the top of the handler.

Update
No, it is still required. Sorry for the hassle.

coveralls · 2017-07-25T11:24:39Z

Coverage decreased (-0.02%) to 70.375% when pulling e5158ad on mofux:cursor_color into 2e9177a on sourcelair:master.

mofux · 2017-07-25T11:33:24Z

@Tyriar I have merged the rendering of the cursor with the existing logic, which avoids code duplication and allows for all flags to be applied to the cursor (with the exception of xterm-hidden which I believe is something we don't want for the cursor).

I have also fixed #820 on the way.

Check the updated PR head notes for running some tests (using nvim).

coveralls · 2017-07-25T11:40:03Z

Coverage decreased (-0.03%) to 70.375% when pulling 51bed8e on mofux:cursor_color into f36bd24 on sourcelair:master.

mofux · 2017-07-25T17:50:34Z

This is ready for re-review 😁. The diff for Renderer.ts is a little confusing because I have removed a nested if, so the whole inner block has shifted one indentation to the left now.

parisk

Thanks for this PR @mofux 😄!

In what manner does this fix #783? This issue is closed now, as we decided to keep the current behavior.

mofux · 2017-07-26T12:39:39Z

@parisk It respects the decisions made in #783 - which means showing an outlined block cursor in the unfocused state, no matter what cursor style is currently active (basically the same behaviour as we had before). When I first started this PR it seemed that #783 would go into the direction of showing the selected cursor in unfocused state.

See commit e5158ad for details.

Tyriar · 2017-07-31T16:57:05Z

Sorry haven't had time to get to this yet. I've been swamped with other tasks plus I was pretty ill all last week 🙁

parisk · 2017-08-02T07:57:55Z

@mofux can you please rebase with master? It would be great to include this in 2.9.0 😄.

coveralls · 2017-08-02T12:16:27Z

Coverage decreased (-0.03%) to 70.442% when pulling 164f785 on mofux:cursor_color into bcff69b on sourcelair:master.

coveralls · 2017-08-02T13:02:10Z

Coverage decreased (-0.03%) to 70.442% when pulling 469d167 on mofux:cursor_color into bcff69b on sourcelair:master.

mofux · 2017-08-02T13:04:46Z

@parisk I have just rebased the branch 😅

parisk

Works fine for me and code looks good as well.

I think this is OK to merge now.

Let's wait a few hours for @Tyriar to take a look as well, as he was the main reviewer of this PR, or else I plan to merge this for the 2.9.0 release.

Great job 👍. Thanks!

mofux · 2017-08-02T13:30:02Z

@parisk I have removed the ev.preventDefault() in the mousedown handler - which has fixed #789, because it should now be handled by your #828 PR.

coveralls · 2017-08-02T13:33:43Z

Coverage decreased (-0.01%) to 70.456% when pulling 4ed0a9b on mofux:cursor_color into bcff69b on sourcelair:master.

mofux · 2017-08-02T13:38:01Z

Sorry, I was wrong, just tested with your changes from #828 and ev.preventDefault() is still required to prevent the focus from getting lost when in mouse interactive mode. I have put it back now.

Prevent focus from getting lost when in vt mouse mode Fix: cursor style not applied on init, set a class for block cursor Make all cursors work under all conditions (focused, unfocused, blink) Unify cursor rendering with normal rendering code Make sure underline AND blink can be used in co-existence Always show outlined block cursor in unfocused state

coveralls · 2017-08-02T13:42:19Z

Coverage decreased (-0.03%) to 70.442% when pulling 30a9068 on mofux:cursor_color into bcff69b on sourcelair:master.

coveralls · 2017-08-02T13:52:36Z

Coverage decreased (-0.03%) to 70.442% when pulling 5bc0ad8 on mofux:cursor_color into bcff69b on sourcelair:master.

mofux · 2017-08-02T15:44:15Z

I have updated this PR to resolve the merge conflict from #828. Ready for merge if @Tyriar has no objections 😊

coveralls · 2017-08-02T16:03:37Z

Coverage decreased (-0.03%) to 70.497% when pulling 106eda7 on mofux:cursor_color into f134559 on sourcelair:master.

This was referenced Jul 12, 2017

Terminal is initialised with the wrong cursor style #790

Closed

Terminal focus class is lost under certain conditions #789

Closed

Tyriar requested changes Jul 14, 2017

View reviewed changes

parisk reviewed Jul 26, 2017

View reviewed changes

parisk approved these changes Aug 2, 2017

View reviewed changes

parisk added this to the 2.9.0 milestone Aug 2, 2017

Merge changes from #828

106eda7

Tyriar modified the milestones: 2.9.0, 3.0.0 Aug 3, 2017

Tyriar approved these changes Aug 3, 2017

View reviewed changes

Tyriar merged commit 0079677 into xtermjs:master Aug 3, 2017

mofux deleted the cursor_color branch August 3, 2017 21:02

Retain the color of a character for underline and bar cursor styles #781

Retain the color of a character for underline and bar cursor styles #781

Conversation

mofux commented Jul 11, 2017 • edited by Tyriar Loading

coveralls commented Jul 11, 2017 • edited Loading

Tyriar commented Jul 11, 2017

mofux commented Jul 11, 2017

coveralls commented Jul 11, 2017 • edited Loading

mofux commented Jul 11, 2017 • edited Loading

coveralls commented Jul 11, 2017 • edited Loading

Tyriar commented Jul 11, 2017

mofux commented Jul 12, 2017

coveralls commented Jul 12, 2017 • edited Loading

coveralls commented Jul 12, 2017

mofux commented Jul 13, 2017 • edited Loading

Tyriar commented Jul 13, 2017

coveralls commented Jul 13, 2017 • edited Loading

coveralls commented Jul 14, 2017 • edited Loading

Tyriar Jul 14, 2017

Choose a reason for hiding this comment

Tyriar Jul 14, 2017

Choose a reason for hiding this comment

Tyriar Jul 14, 2017

Choose a reason for hiding this comment

Tyriar Jul 14, 2017

Choose a reason for hiding this comment

mofux Jul 20, 2017

Choose a reason for hiding this comment

mofux Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Jul 25, 2017 • edited Loading

mofux commented Jul 25, 2017 • edited Loading

coveralls commented Jul 25, 2017 • edited Loading

mofux commented Jul 25, 2017

parisk left a comment

Choose a reason for hiding this comment

mofux commented Jul 26, 2017

Tyriar commented Jul 31, 2017

parisk commented Aug 2, 2017

coveralls commented Aug 2, 2017 • edited Loading

coveralls commented Aug 2, 2017 • edited Loading

mofux commented Aug 2, 2017

parisk left a comment

Choose a reason for hiding this comment

mofux commented Aug 2, 2017

coveralls commented Aug 2, 2017 • edited Loading

mofux commented Aug 2, 2017

coveralls commented Aug 2, 2017 • edited Loading

coveralls commented Aug 2, 2017 • edited Loading

mofux commented Aug 2, 2017

coveralls commented Aug 2, 2017 • edited Loading

mofux commented Jul 11, 2017 •

edited by Tyriar

Loading

coveralls commented Jul 11, 2017 •

edited

Loading

coveralls commented Jul 11, 2017 •

edited

Loading

mofux commented Jul 11, 2017 •

edited

Loading

coveralls commented Jul 11, 2017 •

edited

Loading

coveralls commented Jul 12, 2017 •

edited

Loading

mofux commented Jul 13, 2017 •

edited

Loading

coveralls commented Jul 13, 2017 •

edited

Loading

coveralls commented Jul 14, 2017 •

edited

Loading

mofux Aug 2, 2017 •

edited

Loading

coveralls commented Jul 25, 2017 •

edited

Loading

mofux commented Jul 25, 2017 •

edited

Loading

coveralls commented Jul 25, 2017 •

edited

Loading

coveralls commented Aug 2, 2017 •

edited

Loading

coveralls commented Aug 2, 2017 •

edited

Loading

coveralls commented Aug 2, 2017 •

edited

Loading

coveralls commented Aug 2, 2017 •

edited

Loading

coveralls commented Aug 2, 2017 •

edited

Loading

coveralls commented Aug 2, 2017 •

edited

Loading