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

Jump to bottom

Add support for block and underline style cursor in DOM rendering mode #1661

Merged
merged 2 commits into from Sep 8, 2018
Merged

Add support for block and underline style cursor in DOM rendering mode #1661

merged 2 commits into from Sep 8, 2018

Conversation

hindol
Copy link
Contributor

@hindol hindol commented Sep 6, 2018 •
edited

First PR, so suggestions welcome. Fixes #1640.

mofux
mofux requested changes Sep 6, 2018
View reviewed changes
Copy link
Contributor

@mofux mofux left a comment

Choose a reason for hiding this comment

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

Thanks for you PR! I've tried it out but can only see the cursor styles being applied while the terminal is unfocused. Once it gets focused the block cursor comes back. Check out my comments to see why. In general it would be good if we could use the same cursor behaviour as with the canvas renderer, which is: If the terminal is unfocused, we will always show the outline of the block cursor. If the terminal is focused, we will show whatever cursor style is set.

src/renderer/dom/DomRenderer.ts Outdated Show resolved Hide resolved
src/renderer/dom/DomRenderer.ts Outdated Show resolved Hide resolved
src/renderer/dom/DomRenderer.ts Outdated Show resolved Hide resolved
src/renderer/dom/DomRenderer.ts Outdated Show resolved Hide resolved
src/renderer/dom/DomRenderer.ts Outdated Show resolved Hide resolved
@hindol
Copy link
Contributor Author

hindol commented Sep 6, 2018

@mofux Thanks for the suggestions. Fixed!

@mofux
Copy link
Contributor

mofux commented Sep 6, 2018

Thanks for addressing the remaining issues and helping out closing the related issue. This works fine for me now!

@Tyriar Please merge if you're okay with the changes (and want to have it in the upcoming 3.7.0 release).

@Tyriar Tyriar added this to the 3.8.0 milestone Sep 6, 2018
Tyriar
Tyriar requested changes Sep 6, 2018
View reviewed changes
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Looks good, just 2 comments to speed things up a little 😃

src/renderer/dom/DomRendererRowFactory.ts Outdated Show resolved Hide resolved
src/renderer/dom/DomRenderer.ts Outdated Show resolved Hide resolved
Tyriar
Tyriar requested changes Sep 7, 2018
View reviewed changes
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Great! Just one more minor change and then this is good to go.

src/renderer/dom/DomRendererRowFactory.ts Outdated Show resolved Hide resolved
@hindol
Updates as per code review comments
2716413 
Getting rid of the ICursorOptions interface to improve performance
@Tyriar
Copy link
Member

Tyriar commented Sep 8, 2018

Thanks @hindol! Merging 😃

@Tyriar Tyriar merged commit 4ffc076 into xtermjs:master Sep 8, 2018
@hindol hindol deleted the fix/1640 branch September 8, 2018 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tyriar Tyriar Tyriar approved these changes

@mofux mofux mofux approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.8.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hindol @mofux @Tyriar