-
Notifications
You must be signed in to change notification settings - Fork 9
Load icons again #208
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.
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
Load icons again #208
Conversation
michael-hawker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, couple of questions, mostly about ensuring we don't leak the DispatcherQueue and tempt folks 😋
Will defer to @niels9001 on any notes about style/re-usability of the pattern for the icon with the Border. Probably not heavy enough that we'd want to stick it in a UserControl and deal with that overhead, I'm assuming? I do like the nice encapsulation we have in the cases where it's optional and we toggle visibility with HasIcon.
src/modules/cmdpal/Microsoft.CmdPal.UI/LoadIconBehavior.xaml.cs
Outdated
Show resolved
Hide resolved
| // For inexplicable reasons, FontIconSource.CreateIconElement | ||
| // doesn't work, so do it ourselves |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that's surprising! Is there an open issue for this in the WinUI repo?
I know there was some weird discussion around resourcing and the XAML tree recently, but think that was for Geometrys, but I wonder if there'd be some other odd shared reference with the font info/glyph maybe, who knows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing that I found. I'm not sure if it's a platform bug or a "I'm holding it wrong" thing. IIRC it did end up returning an icon, but it never made it into the UI tree? 🤷
src/modules/cmdpal/Microsoft.CmdPal.UI/LoadIconBehavior.xaml.cs
Outdated
Show resolved
Hide resolved
src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ActionBar.xaml.cs
Outdated
Show resolved
Hide resolved
src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ActionBarViewModel.cs
Outdated
Show resolved
Hide resolved
| <!-- LoadIconBehavior will magically fill this border up with an icon --> | ||
| <Interactivity:Interaction.Behaviors> | ||
| <local:LoadIconBehavior Source="{x:Bind ViewModel.Details.HeroImage, Mode=OneWay}"/> | ||
| <local:HideMissingIconBehavior Source="{x:Bind ViewModel.Details.HeroImage, Mode=OneWay}"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline, this is a bit overzealous use of behavior here. We can expose a loading prop on the LoadIconBehavior, or just use the HasIcon on the VM at the moment like we did elsewhere for now.
Maybe later too, we just have a default icon that gets returned in the failure case in case devs are loading from remote resources?
(If we wanted to combine the HasIcon and HasLoaded we could bind them together with an x:Bind function on the parent border.)
michael-hawker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM



This adds a magic helper to load icons for us. Any time you want an icon, just do this:
And that'll magically give us a border filled with the icon, and updating with the binding.
I believe it'll also work with
IRandomAccessStreamReferences, but I didn't actually test that with #151 yet.I didn't actually implement the "caching" bit of this yet. That'll involve doing some locking per-key inside the factory and I didn't want to futz with that in this initial PR to restore icons