search

telerik / UI-For-UWP

This repo contains the source code for Telerik UI for Universal Windows Platform (UWP), which includes 20+ UI controls for developers building UWP applications.
http://www.telerik.com/uwp/
Other
1.16k stars 234 forks source link

Fix copy/paste bug in EditingService.cs #513

Closed Zofware closed 1 year ago

Zofware commented 2 years ago

Fix a copy/paste bug in EditingService.cs that caused the data grid view state to go haywire when using inline editing mode on rows with MVVM properties (i.e. properties that send PropertyChanged notifications).

Also, ensure the row is refreshed/redrawn when inline edits are cancelled. Previously, canceled edits would still appear on the row until the view was refreshed/redrawn by other means such as resizing the column.

Zofware commented 2 years ago

@vvgeorgiev,

Thanks for posting your feedback. I sent a reply a few hours ago that was incorrect so I deleted it. It had a link to a sample app that was supposed to demonstrate the issue I'm trying to fix, but the sample itself was buggy. Once I straighten out the sample, I'll repost and we can continue the discussion.

Thanks, Adam

Zofware commented 2 years ago

@vvgeorgiev,

Here's a short YouTube video showing why I think at least some additional refresh logic is needed after fixing the two one-line copy/paste bugs in the first commit of this PR. The problem is when edits are canceled, the original values are not shown on the affected row until something else causes a redraw (like another edit operation on the same row).

It's possible that moving the RegisterUpdate call, as in the 2nd commit of this PR, is all that's needed to get the row to redraw correctly after canceling an edit but I don't think so. I'll have to check again since it's been a few months since I made these changes.

Regardless, some additional refresh logic is required after canceling an edit. Please suggest a more appropriate way of doing this if you know it. My familiarity with this code is pretty limited.

Thanks, Adam

vvgeorgiev commented 2 years ago

Hi Adam, Thanks for the additional details.

Your initial commit is no doubt correct. Moving the RegisterUpdate wouldn't matter much runtime, as it's executed asynchronously. Still, it's correct. Now, I seem to struggle to reproduce the problem as seen in your video. Sharing that app, you used for the video might be a good idea. Thanks. Ves

Zofware commented 2 years ago

@vvgeorgiev, my apologies for not posting the link to my sample repo. It's here: https://github.com/Zofware/EditingServiceBug

By default, the sample builds against the default version of Telerik UI on nuget. You'll have to apply my first commit to the Telerik code, locally build it, and then reference it in the sample app to reproduce the bug.

vvgeorgiev commented 2 years ago

Hi Adam, Thanks for the app. I am afraid it does not reproduce the issue on my end. I am not sure if the Windows version matters, or there's anything else. My suggestion here would be to go ahead and merge the core update i.e. the first commit, along with this change, while further investigation is needed on the side effects you have shown. Thanks, Ves

Zofware commented 2 years ago

FWIW, I'm running Windows 10 22H2 (19045.2130).

Zofware commented 2 years ago

I'll roll back the last two commits, leaving only the first two. Thanks.

Zofware commented 2 years ago

Last two commits reverted.

vvgeorgiev commented 2 years ago

I am sorry, I did not make it clear. I suggest that we go ahead with the first commit and only a single change from the second one -- the one that moves the call to UpdateService.RegisterUpdate from line 198 to line 205.

Zofware commented 2 years ago

Sorry for the misunderstanding. Fixed.