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

Improve duplicate handling #1668

Closed

Conversation

w-syss
Copy link
Contributor

@w-syss w-syss commented Feb 22, 2020

I was very confused when using the IgnoreDuplicate Property of the SnackBar as it did the exact opposite of what i expected it to do. I think the wording "IgnoreDuplicate" implies that duplicates are in fact ignored and not that the IgnoreDuplicate property is ignored.

These changes aim to clarify what these properties actually do, along with a bit of simplification in the comparison logic.

I am aware that breaking the public API might not be the best idea, however I think this clarification is a good QoL change.

Renamed IgnoreDuplicate to ShowAlways.
Added MessageExpired method.
Added appropriate Equals method.
Added autogenerated GetHashCode method.

ShowAlways more closely represented what that property actually does. IgnoreDuplicate is not very telling in the context of a SnachkBarMessageQueueItem.
MessageExpired serves as a helper to determine if a message already exceeded it's duration.
Equals and HashCode simplify comparisons of SnackBarMessageQueueItems, especially useful in the determining if a message is a duplicate.
Changed IgnoreDuplicate to DiscardDuplicates
This reflects the changes to the Snackbar. Negating the selection is not necessary anymore.
Changed IgnoreDuplicate to DiscardDuplicates.
Extracted message checking.

These changes improve readability of the SnackbarMessage filtering.
@jespersh
Copy link
Contributor

I suppose I can agree with that. Nice work.

Could I get you tempted to write a few tests to see that duplicates are properly discarded depending on different content and actioncommands?
It would be very appreciated :-)

Copy link
Member

@Keboo Keboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work. Thank you for the contribution. Because this is a breaking API change, I am going to flag it for the 4.0.0 release.

Also +1 for the unit tests. This has been an area of confusion in the API that could certainly use some.

MaterialDesignThemes.Wpf/SnackbarMessageQueueItem.cs Outdated Show resolved Hide resolved
MaterialDesignThemes.Wpf/SnackbarMessageQueueItem.cs Outdated Show resolved Hide resolved
MaterialDesignThemes.Wpf/SnackbarMessageQueueItem.cs Outdated Show resolved Hide resolved
MaterialDesignThemes.Wpf/SnackbarMessageQueueItem.cs Outdated Show resolved Hide resolved
@Keboo Keboo added breaking change Items here have breaking API changes. enhancement labels Feb 22, 2020
@Keboo Keboo added this to the 4.0.0 milestone Feb 22, 2020
@w-syss
Copy link
Contributor Author

w-syss commented Feb 22, 2020

Thank you for you advice. I will add unit tests.

Removed null check for value type.
Improved null handling in Equals.
Simplyfied GetHashCode.
@w-syss
Copy link
Contributor Author

w-syss commented Feb 23, 2020

I added System.ValueTuple 4.3.1 as a dependency. For some reason for versions >= 4.4.0 there is a dependency issue with System.Core.

This helps testing the SnackbarMessageQueue.
These are some basic unit tests for the SnackbarMessageQueue. This commit only tests the GetSnackbarMessage method if it handles duplicates and null values correctly.
@w-syss w-syss requested a review from Keboo February 29, 2020 14:03
Copy link
Member

@Keboo Keboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I pushed up some additional changes handling the NuGet dependency with paket.

@Keboo
Copy link
Member

Keboo commented Oct 10, 2020

@w-syss I apologize it has taken so long to get to the 4.0.0 release. Do you mind revisiting this PR and seeing about updating it so it can be merged?

@w-syss
Copy link
Contributor Author

w-syss commented Oct 11, 2020

Sure, just give me a couple of days.

@Keboo Keboo mentioned this pull request Feb 5, 2021
@Keboo
Copy link
Member

Keboo commented Feb 5, 2021

This is resolved by #2240

@Keboo Keboo closed this Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Items here have breaking API changes. enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants