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

[Settings UI] Clamp vintage cursor height and history size #9370

Merged
2 commits merged into from
Mar 4, 2021
Merged

[Settings UI] Clamp vintage cursor height and history size #9370

2 commits merged into from
Mar 4, 2021

Conversation

eugenesmlv
Copy link
Contributor

@eugenesmlv eugenesmlv commented Mar 3, 2021 •
edited by zadjii-msft
Loading

Summary of the Pull Request

Add Minimum and Maximum for the cursor height numberbox in the SUI.
Add Minimum for the history size numberbox in the SUI.

PR Checklist

Validation Steps Performed

Manual validation

@ghost ghost added Area-Settings UI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Mar 3, 2021
zadjii-msft
zadjii-msft approved these changes Mar 3, 2021
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.

@ the rest of the team - I think we should stick with 1-100 here, despite 25-100 being the valid values currently, because we should accept 1-100 as reasonable values. See #9175

@eugenesmlv
Copy link
Contributor Author

@zadjii-msft I found out that the history size number box also has no bottom limit and allows the user to set negative values. Should we fix that? I could do it in this PR or file an issue.

HistorySizeNumbox

@zadjii-msft
Copy link
Member

Should we fix that?

Yep we definitely should! I wouldn't mind if you included that in this PR. If this merges before you add that though, I'll make sure to file a follow up. Good catch!

The min for that one should just be 0.

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Mar 4, 2021
@ghost ghost added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. labels Mar 4, 2021
carlos-zamora
carlos-zamora approved these changes Mar 4, 2021
View reviewed changes
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

We should consider representing "Cursor Size" a slider if we're introducing a minimum/maximum. I'll file a follow-up task.

Thanks for doing this!

@carlos-zamora carlos-zamora mentioned this pull request Mar 4, 2021
Closed
@carlos-zamora carlos-zamora changed the title Clamp vintage cursor height value between 1 and 100 [Settings UI] Clamp vintage cursor height and history size Mar 4, 2021
@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 4, 2021
@ghost
Copy link

ghost commented Mar 4, 2021

Hello @carlos-zamora!

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.

@ghost ghost merged commit ac3fecb into microsoft:main Mar 4, 2021
@zadjii-msft zadjii-msft mentioned this pull request Mar 4, 2021
Closed
This was referenced Mar 4, 2021
Closed
Merged
ghost pushed a commit that referenced this pull request Mar 9, 2021
@eugenesmlv
[Settings UI] Represent Cursor Height as a slider (#9386)
48d59e8 
Change the vintage cursor height number box to a slider.

## References
Related:  #9370

## PR Checklist
* [x] Closes #9377
* [x] zadjii-msft edit: Now _this one_ closes #9175
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Schema updated.
* [ ] 

## Detailed Description of the Pull Request / Additional comments

It seems like the cursor height couldn't be lower than 25 percent regardless of the given value, so I've changed the `MinCursorHeightPercent` in CustomTextRenderer header file.

## Validation Steps Performed
Manual validation

![CursorHeightSlider](https://user-images.githubusercontent.com/39456018/110041939-bf076080-7d66-11eb-8d58-ba9a84922803.gif)
DHowett pushed a commit that referenced this pull request Apr 13, 2021
@eugenesmlv @DHowett
[Settings UI] Clamp vintage cursor height and history size (#9370)
f67db39 
## Summary of the Pull Request
Add `Minimum` and `Maximum` for the cursor height numberbox in the SUI.
Add `Minimum` for the history size numberbox in the SUI.

## PR Checklist
* [x] Closes #9357, Closes #9175
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA

## Validation Steps Performed
Manual validation

(cherry picked from commit ac3fecb)
@ghost
Copy link

ghost commented Apr 14, 2021

🎉Windows Terminal v1.7.1033.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Apr 14, 2021
Closed
@ghost
Copy link

ghost commented Apr 14, 2021

🎉Windows Terminal Preview v1.8.1032.0 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlos-zamora carlos-zamora carlos-zamora approved these changes

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

@miniksa miniksa Awaiting requested review from miniksa

@DHowett DHowett Awaiting requested review from DHowett

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

Assignees
No one assigned
Labels
Area-Settings UI Anything specific to the SUI Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Second It's a PR that needs another sign-off Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Vintage cursor height in SUI does not clamp values to 1-100
3 participants
@eugenesmlv @zadjii-msft @carlos-zamora