Fixes #2597. Enables standard glyphs to be set via ConfigurationManager #2595
Conversation
tig
changed the title
| /// - unicode char (e.g. "☑") | ||
| /// - U+hex format (e.g. "U+2611") | ||
| /// - \u format (e.g. "\\u2611") | ||
| /// A number |
There was a problem hiding this comment.
Nice! I like that we are flexible about what we allow.
Terminal.Gui/Views/CheckBox.cs
Outdated
| Checked = is_checked; | ||
| HotKeySpecifier = new Rune ('_'); | ||
| HotKeySpecifier = Glyphs.HotKeySpecifier; |
There was a problem hiding this comment.
Is HotKeySpecifier really a Glyph?
I thought it was more like an escape character that indicates to Terminal.Gui where to change color when rendering (and to pick up the next character as the shortcuts?).
I think if the end user was to edit this to another symbol quite a lot of stuff might break in Terminal.Gui apps especially if the applications author just assumes that its _ .
There was a problem hiding this comment.
If all the _ are replaced with the Glyphs.HotKeySpecifier there aren't any issue. That will imply the users have to change their code to $"{Glyphs.HotKeySpecifier}whatvever". But isn't a big deal with the editors replacement tools.
There was a problem hiding this comment.
Ill remove it from Glyphs.
I included it as a mental model test and forgot to remove it.
| return _historyText.IsDirty (Text); | ||
| } | ||
| set { | ||
| _historyText.Clear (Text); |
There was a problem hiding this comment.
This change is unrelated to glyphs? Could you give some insight into what it's for?
Not a problem just curious.
Maybe update the xmldoc too to describe the set use case?
There was a problem hiding this comment.
Well I'm also curious with this. Do you want, if the value is set to false then clear the _historyText? If so, there is the ClearHistoryChanges method for that.
There was a problem hiding this comment.
Fixes a bug in config editor where i want isdirty to be cleared after saving. Will use the method BDisp suggested. Didn't see that.
|
Looks good. Just a few minor points. This is a great feature, awesome work! 🚀 |
|
It's a very great work, congrats. |
|
I got carried away and went further.
|
|
Maybe a static value that is changed in configs but not reset after the test or something? Or just a copy/paste typo? [xUnit.net 00:00:24.06] Expected: ╒═╕ \n╔╡1╞╗\n║╘═╛║\n╚═══╝
[xUnit.net 00:00:24.06] Actual: ╒═╖ \n╔╡1╞╗\n║╘═╜║\n╚═══╝ |
|
Typo. I blame chatgpt who I used for some of this. |
|
It'll be fun to extend the LineDrawing scenario to do more. Issues
|
|
Yeah that would be cool!
|
|
Ok. This puppy is ready for final review. |
Fixes #2597
As a distraction from working on the new view architecture, and because i've lost hope that we'll ever be able to ask terminals if they support particular glyphs, I decided to refactor how the standard glyphs are set and enable users to change them via config.
See microsoft/terminal#1040 (comment)
Pull Request checklist:
CTRL-K-Dto automatically reformat your files before committing.dotnet testbefore commit///style comments)