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

DictionaryAdapter notifies at CancelEdit even when SuppressNotifications is used #299

Closed
Lakritzator opened this issue Sep 13, 2017 · 5 comments
Closed

DictionaryAdapter notifies at CancelEdit even when SuppressNotifications is used #299

Lakritzator opened this issue Sep 13, 2017 · 5 comments
Assignees
@Lakritzator
Milestone
v4.2.0

Comments

@Lakritzator
Copy link
Contributor

When I call BeginEdit / CancelEdit in a SuppressNotificationsBlock, there are PropertyChanged events for properties that are rolled back.

I wrote tests for this, the test Test_SuppressNotification_EditCancel fails in the last assert.
@Lakritzator Lakritzator changed the title DictionaryAdapter notifies when SuppressNotifications DictionaryAdapter notifies at CancelEdit even when SuppressNotifications is used Sep 13, 2017
@jonorossi
Copy link
Member

@Lakritzator any chance you can dig into DictionaryAdapter and work out what is going wrong?

@Lakritzator
Copy link
Contributor Author

Lakritzator commented Sep 13, 2017 •
edited
Loading

@jonorossi yes, I can do that. I do still first need to finish my work on releasing the new version of Greenshot, but I will try to fix this tomorrow. I can't assign this to me, you are welcome to do so.

@ghost ghost assigned Lakritzator Sep 13, 2017
@ghost ghost removed the up-for-grabs label Sep 13, 2017
@Lakritzator
Copy link
Contributor Author

Added PR #301

jonorossi added a commit that referenced this issue Sep 17, 2017
@jonorossi
Merge pull request #301 from Lakritzator/bugfix/SuppressNotifications…
266d7e0 
…BugWhenCancelEdit

Fix for #299, NotifyPropertyChang(ed/ing) events not suppressed on CancelEditing
@jonorossi
Copy link
Member

Thanks @Lakritzator, I don't know why it never checked ShouldNotify, I guess this situation was overlooked. Your fix is merged and will be in the next release.

@Lakritzator
Copy link
Contributor Author

You are welcome, never wonder why... happens to all of us. Thanks for merging.

@jonorossi jonorossi added this to the vNext milestone Sep 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Lakritzator Lakritzator

Labels
None yet
Projects
None yet
Milestone
v4.2.0
Development

No branches or pull requests
2 participants
@jonorossi @Lakritzator