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.

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

Added basic support for IconSource and its subclasses #192

Merged
merged 4 commits into from
Nov 6, 2020

Conversation

ShankarBUS
Copy link
Contributor

Fixes #191

ModernWpf/Helpers/SharedHelpers.cs Outdated Show resolved Hide resolved
ModernWpf/Helpers/SharedHelpers.cs Outdated Show resolved Hide resolved
ModernWpf/Helpers/SharedHelpers.cs Outdated Show resolved Hide resolved
ModernWpf/Helpers/SharedHelpers.cs Outdated Show resolved Hide resolved
ModernWpf/IconSource/IconSource.cs Outdated Show resolved Hide resolved
ModernWpf/IconSource/PathIconSource.cs Outdated Show resolved Hide resolved
ModernWpf/IconSource/FontIconSource.cs Outdated Show resolved Hide resolved
ModernWpf/IconSource/FontIconSource.cs Outdated Show resolved Hide resolved
@ShankarBUS
Copy link
Contributor Author

Should I also add the xml documentation comments?

@ShankarBUS
Copy link
Contributor Author

@Kinnara, I would like to port the TeachingTip control from WinUI #88 once this PR is merged.

What do you say?

@ShankarBUS
Copy link
Contributor Author

Hey @Kinnara,

I explored ModernWpf's source code.
I can see that you're using bindings instead of RegisterPropertyChangedCallback() which is not available natively in WPF. You have to add additional private dependency properties instead of just storing the registered token and revoking it.

This increases the inconsistency in code between WinUI and ModernWpf.

What I suggest you is to take a look at our toolkit and see how it handles this scenario.

I've made a DependencyObjectExtensions class which will do this for.

https://github.com/ModernWpf-Community/ModernWpfCommunityToolkit/blob/master/ModernWpf.Toolkit.UI/Extensions/DependencyObjectExtensions.cs

This will help us to reduce extra code and increase the consistency between the codebases.

My workaround is imperfect but with your help, we can improve it.

What do you say?

and moved code into an existing file to resolved CS0436 warning
@ShankarBUS
Copy link
Contributor Author

@Kinnara, all done! This PR is ready to be merged.

@Kinnara
Copy link
Owner

Kinnara commented Nov 6, 2020

My workaround is imperfect but with your help, we can improve it.

What do you say?

The main road block is that we have to call RemoveValueChanged when using AddValueChanged, otherwise the DependencyObject will never be garbage collectied. When using WinRT's RegisterPropertyChangedCallback, there is no such issue.

@Kinnara Kinnara merged commit 7643681 into Kinnara:master Nov 6, 2020
@ShankarBUS
Copy link
Contributor Author

ShankarBUS commented Nov 6, 2020

We have to unregister the callbacks when unloading the control.

https://stackoverflow.com/a/45160026

This behaviour is same in WinRT, isn't it.

I've a little experience porting Controls from WCT so I can confirm they do the callbacks unregistering on unload.

If you say so, it must correct be correct 🤷‍♂️.

Thanks for merging this PR!

@ShankarBUS ShankarBUS deleted the IconSource branch November 6, 2020 16:33
ShankarBUS added a commit to ShankarBUS/ModernWpf that referenced this pull request Nov 6, 2020
Added basic support for IconSource and its subclasses (Kinnara#192)

* Ported IconSource and its sub classes from WinUI

* Resolved suggestions

* Added headers with copyright & license notices to avoid potential licensing issues

* Removed an unnecessary duplicate class

and moved code into an existing file to resolved CS0436 warning

Co-authored-by: Yimeng Wu <me@kinnara.cn>
ShankarBUS added a commit to ShankarBUS/ModernWpf that referenced this pull request Nov 6, 2020
* Ported IconSource and its sub classes from WinUI

* Resolved suggestions

* Added headers with copyright & license notices to avoid potential licensing issues

* Removed an unnecessary duplicate class

and moved code into an existing file to resolved CS0436 warning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question: Why is IconSource not supported in this library?
2 participants