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.

Sign up for GitHub

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 doubleUnderscore cursor style #7827

Merged
9 commits merged into from Jan 20, 2021
Merged

Add doubleUnderscore cursor style #7827

9 commits merged into from Jan 20, 2021

Conversation

rhorber
Copy link
Contributor

@rhorber rhorber commented Oct 5, 2020 •
edited by DHowett

Adds a new cursor type "doubleUnderscore". Tested manually.

Closes #6786

@ghost ghost added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Oct 5, 2020
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 5, 2020
@zadjii-msft zadjii-msft changed the title Feature/issue 6786 Add doubleUnderscore cursor style Oct 5, 2020
@rhorber
Remove paint type HorizontalOnly in DxRenderer
135dca3 
To draw the double underscore cursor draw the upper line directly.
For this, I moved the initialisation of brush up to the paintType.
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 6, 2020
@rhorber rhorber mentioned this pull request Oct 6, 2020
Merged
@microsoft-github-updates microsoft-github-updates bot changed the base branch from master to main October 22, 2020 00:27
@rhorber
Copy link
Contributor Author

rhorber commented Oct 23, 2020

As far as I know all necessary changes are made and to put it "on radar" I marked the PR "Ready for review".

@rhorber rhorber marked this pull request as ready for review October 23, 2020 05:56
miniksa
miniksa requested changes Oct 28, 2020
View reviewed changes
src/renderer/dx/CustomTextRenderer.cpp Show resolved Hide resolved
src/inc/conattrs.hpp Show resolved Hide resolved
src/renderer/dx/CustomTextRenderer.cpp
Comment on lines +329 to +330
upperLine.top -= 2;
upperLine.bottom -= 2;
Copy link
Member

Choose a reason for hiding this comment

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

Does this look correct at all DPIs or does it need to be adjusted by a scale factor?

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 switched "Custom scaling" to 200% in the settings. Is this the correct/preferred procedure?

Single underline is IMHO also thin:
underscore

And double underscore for comparison:
doubleUnderscore

(Clicking the images helps.)

I'm open whether to scale or not to scale 😊

Copy link
Member

Choose a reason for hiding this comment

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

Oof, can you file a follow on issue (or search the repo for an existing one) to make sure these things get scaled? I bet its just 1px and it looks bad for all options.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I couldn't find one on searching. Maybe my boolean-fu is bad. @j4james this might be a thing you know about? I don't know why I think that... but something rings a bell in my head that you might have noticed cursors aren't DPI scaled...

Copy link
Collaborator

Choose a reason for hiding this comment

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

No. I was looking at the grid line rendering (underlines, overlines, etc.). Those do now track the font style and also scale appropriately, but I don't think the cursors do (at least not all of them).

src/renderer/gdi/paint.cpp Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 28, 2020
@rhorber
Move setting cursor color befor cursorType switch
047c554 
Because the doubleUnderscore cursor directly draws a line, the color
needs to be set before the switch. Otherwise one line would be white.
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 4, 2020
zadjii-msft
zadjii-msft approved these changes Nov 5, 2020
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I'm okay with this because the DX code looks good, and the GDI code is technically unreachable, so I'm not really worried about how that looks.

src/renderer/gdi/paint.cpp Show resolved Hide resolved
@zadjii-msft zadjii-msft assigned miniksa and unassigned rhorber Nov 5, 2020
@zadjii-msft zadjii-msft assigned rhorber and unassigned miniksa Nov 17, 2020
@zadjii-msft
Copy link
Member

@rhorber heads up, this merge-conflicted in TerminalSettingsSerializationHelpers, but I really doubt the conflict will be that hard to resolve...

@miniksa
Copy link
Member

miniksa commented Dec 3, 2020

I just want to get a bit of closure on whether we need to file a follow on for cursor shape scaling or if there's an existing one. Then I guess there's no sense on standing in the way of this any further.

@miniksa miniksa added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 17, 2020
@zadjii-msft
Copy link
Member

The merge conflict here should be trivial, but I don't trust myself with git to push the fix to someone else's fork.

@DHowett would you mind resolving this and merging it? Or @rhorber you could too and get it merged faster ☺️

@zadjii-msft zadjii-msft assigned DHowett and unassigned miniksa Jan 19, 2021
DHowett
DHowett approved these changes Jan 20, 2021
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Thank you for doing this. I've updated the value for the settings UI, as well, and changed the order of the enum values so that they show up nicely.

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 20, 2021
@ghost
Copy link

ghost commented Jan 20, 2021

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@DHowett
Copy link
Member

DHowett commented Jan 20, 2021

I apologize for us taking so long to get to this. Sometimes, quick wins from our community slip through the cracks. I'm making sure this goes out in 1.6. 😄

@ghost ghost merged commit b7a7aa0 into microsoft:main Jan 20, 2021
@cinnamon-msft cinnamon-msft mentioned this pull request Jan 22, 2021
Closed
15 tasks
@rhorber
Copy link
Contributor Author

rhorber commented Jan 27, 2021

I apologize too for not responding. 😔 (I had some busy months and was not on GitHub for work either.)

I am very happy I could help.

If it's okay, I will sometimes check the open issues if there is something I can do (and is not time critical 😆).

@rhorber rhorber deleted the feature/issue-6786 branch January 27, 2021 08:01
@DHowett
Copy link
Member

DHowett commented Jan 27, 2021

You’re very welcome to do that! Thanks again!

mpela81 pushed a commit to mpela81/terminal that referenced this pull request Jan 28, 2021
@rhorber @mpela81
Add doubleUnderscore cursor style (microsoft#7827)
92752bd 
Adds a new cursor type "doubleUnderscore". Tested manually.

Closes microsoft#6786
@j4james j4james mentioned this pull request Jul 22, 2023
Open
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vadimkantorov vadimkantorov vadimkantorov left review comments

@j4james j4james j4james left review comments

@DHowett DHowett DHowett approved these changes

@miniksa miniksa miniksa approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

Assignees

@DHowett DHowett

@rhorber rhorber

Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-Settings Issues related to settings and customizability, for console or terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add new cursor types ‗
6 participants
@rhorber @zadjii-msft @miniksa @DHowett @vadimkantorov @j4james