Skip to content

Disable being able to set the automatic adjustment of indistinguishable text#12160

Merged
5 commits merged intomainfrom
dev/pabhoj/disable_adjust_text
Jan 18, 2022
Merged

Disable being able to set the automatic adjustment of indistinguishable text#12160
5 commits merged intomainfrom
dev/pabhoj/disable_adjust_text

Conversation

@PankajBhojwani
Copy link
Contributor

Summary of the Pull Request

Disables the automatic adjustment of indistinguishable text (added in #11095) because of the concerns brought up in #11917. Also, the setting is hidden in the SUI for as long as this feature remains disabled.

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated.
  • I work here

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.

  • update the schema plz
  • update the docs plz
  • Curious: why not keep this feature in preview, but with it being disabled by default? I get there's a bug, but if the behavior is opt-in, wouldn't that be acceptable for a preview-level feature?

@j4james
Copy link
Collaborator

j4james commented Jan 13, 2022

Sorry, I think my PR #12127 has messed things up a bit for you here. The color adjustment code has now been moved into a new RenderSettings class. Hopefully it shouldn't be too much effort to reincorporate these changes there.

@PankajBhojwani PankajBhojwani changed the title Disable automatic adjustment of indistinguishable text by default Disable being able to set the automatic adjustment of indistinguishable text Jan 14, 2022
@miniksa
Copy link
Member

miniksa commented Jan 14, 2022

Was going to merge, but want to see @PankajBhojwani address @carlos-zamora's questions.

@DHowett DHowett changed the title Disable being able to set the automatic adjustment of indistinguishable text [1.12] Disable being able to set the automatic adjustment of indistinguishable text Jan 14, 2022
@DHowett DHowett changed the title [1.12] Disable being able to set the automatic adjustment of indistinguishable text Disable being able to set the automatic adjustment of indistinguishable text Jan 14, 2022
@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 18, 2022
@ghost
Copy link

ghost commented Jan 18, 2022

Hello @zadjii-msft!

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 f605adc into main Jan 18, 2022
@ghost ghost deleted the dev/pabhoj/disable_adjust_text branch January 18, 2022 11:23
@DHowett
Copy link
Member

DHowett commented Jan 18, 2022

image

oop!

@PankajBhojwani
Copy link
Contributor Author

PankajBhojwani commented Jan 18, 2022

My bad, this slipped my mind while trying to get #12144 ready for the bug bash. Will file a separate PR to make this a preview-only feature that's disabled by default as per @carlos-zamora's suggestion

EDIT: after team discussion, we are going to make this a dev-only setting for now

@ghost
Copy link

ghost commented Feb 3, 2022

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

Handy links:

ghost pushed a commit that referenced this pull request Feb 9, 2022
…or Dev builds (#12444)

## Summary of the Pull Request
Followup from our discussion during team sync, the 'automatic adjustment of indistinguishable text' setting is going to be enabled for dev builds only.

## References
#12160 

## PR Checklist
* [ ] Closes #xxx
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [x] I work here

## Validation Steps Performed
Setting appears on dev build and the feature works
ghost pushed a commit that referenced this pull request Jul 14, 2022
…13343)

## Summary of the Pull Request
Re-enable the setting to adjust indistinguishable text, with some changes:

- We now pre-calculate the nudged values for 'default bright foreground' as well
- When a color table entry is manually set (i.e. via VT sequences) we re-calculate the nudged values

## References
#11095 #12160

## PR Checklist
* [x] Closes #11917
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.

## Validation Steps Performed
Indistinguishable text gets adjusted again
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AutoMerge Marked for automatic merge by the bot when requirements are met

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants