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

Replace class-based XAML converters with bound function converters #10387

Closed
DHowett opened this issue Jun 9, 2021 · 11 comments · Fixed by #10846
Closed

Replace class-based XAML converters with bound function converters #10387

DHowett opened this issue Jun 9, 2021 · 11 comments · Fixed by #10846
Assignees
@marcelwgn
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-Settings UI Anything specific to the SUI Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal Backlog

Comments

@DHowett
Copy link
Member

DHowett commented Jun 9, 2021

With the Settings UI, we have had an immense proliferation of class-based xaml converters.

They're pretty heavy, and they clutter the metadata with classes (and, well, clutter our build with idl files!). We also have to keep instances of them laying around for every xaml page that uses them.

We should instead bind more things with x:Bind Functions!

Proposal:

Introduce Microsoft.Terminal.Settings.Editor.Converters which has static conversion functions on it.
Use them exclusively in the settings editor.
Get rid of all the converter classes.

Evaluate the need for BindBack reverse converters. 😄
@DHowett DHowett added Help Wanted We encourage anyone to jump in on these. Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Priority-3 A description (P3) Area-Settings UI Anything specific to the SUI labels Jun 9, 2021
@DHowett DHowett added this to the Terminal Backlog milestone Jun 9, 2021
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jun 9, 2021
@DHowett
Copy link
Member Author

DHowett commented Jun 9, 2021

/cc @chingucoding 😄 since you asked!

@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jun 9, 2021
@marcelwgn
Copy link
Contributor

Awesome, thank you @DHowett !

@marcelwgn
Copy link
Contributor

Out of curiosity, what is "blocking" this? Are there technical reasons or is it just that other things are more important than this?

@DHowett
Copy link
Member Author

DHowett commented Jun 9, 2021

Nothing at all, just available developer time. 😄

We've got so much to do, and only so many hours in the work week.

@marcelwgn
Copy link
Contributor

Ah haha I see, the classical developer problem! Good to know though that it's up for grabs then and not blocked by technical issues!

@DHowett
Copy link
Member Author

DHowett commented Jun 9, 2021

If you're interested in doing it, I'd be happy to assign you so that someone else doesn't come swoop in and do it! 😁

@marcelwgn
Copy link
Contributor

I currently have a lot of things on my plate, so I'm not sure when I would get to this. I'll let you know when I have more time and would be able to work this; if someone else wants to work on this, I don't want to block anyone here.

@marcelwgn
Copy link
Contributor

@DHowett I would like to take a look at this now, feel free to assign this to me :)

@DHowett
Copy link
Member Author

DHowett commented Jul 11, 2021

Awesome! Thanks :)

@marcelwgn
Copy link
Contributor

So after working on this and finally realizing how BindBack works, it seems that we will need to modify some models to provide a BindBack function. Is that fine behavior? In that case, should the converters reside in the Settings:Editor project and be shared to other projects?

@marcelwgn marcelwgn mentioned this issue Aug 1, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Aug 1, 2021
@ghost ghost closed this as completed in #10846 Aug 3, 2021
ghost pushed a commit that referenced this issue Aug 3, 2021
@marcelwgn
[settings-editor] Switch to function bindings instead of Converter ob…
8ab3422 
…jects (#10846)

## Validation Steps Performed
Clicked around, validated that settings still behave the same (as far as
I can tell with my limited terminal configuration expertise)

Closes #10387
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Aug 3, 2021
@ghost
Copy link

ghost commented Aug 31, 2021

🎉This issue was addressed in #10846, which has now been successfully released as Windows Terminal Preview v1.11.2421.0.:tada:

Handy links:

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

@marcelwgn marcelwgn

Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-Settings UI Anything specific to the SUI Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal Backlog
Development

Successfully merging a pull request may close this issue.

[settings-editor] Switch to function bindings instead of Converter objects marcelwgn/terminal
2 participants
@DHowett @marcelwgn