Feature/add drawingcompleted event to drawingview #1473

bijington · 2021-07-05T22:02:11Z

Description of Change

Bugs Fixed

Fixes [Enhancement] Add DrawingCompleted Event to DrawingView #1466

API Changes

Added:

DrawingView.DrawingCompleted event
DrawingCompletedEventArgs with the points that are in the drawing

Behavioral Changes

Refactored the raising of DrawingCompletedCommand to be within the DrawingView which simplifies the raising of the new event also.
Fixed a potentially serious issue with the Points collection being initialised to a share instance.

PR Checklist

Has a linked Issue, and the Issue has been approved
Has tests (if omitted, state reason in description)
Has samples (if omitted, state reason in description)
Rebased on top of main at time of PR
Changes adhere to coding standard

Updated documentation

bijington · 2021-07-05T22:33:26Z

I have clearly screwed something up. I tried rebasing on develop since I had originally branched from main. I suspect if this is still needed I will need to start with a clean branch off develop

pictos · 2021-07-05T22:45:43Z

@bijington we just target main for bug fixes

- Centralize the on complete invocation. - Add event to invoke when complete.

…wingview

VladislavAntonyuk · 2021-07-12T11:01:24Z

also moved it to #1475

VladislavAntonyuk · 2021-07-12T11:02:01Z

src/CommunityToolkit/Xamarin.CommunityToolkit/Views/DrawingView/DrawingView.shared.cs

@@ -94,5 +97,19 @@ static object CoerceValue(BindableObject bindable, object value)
 			Color strokeColor,
 			Color backgroundColor) =>
 			DrawingViewService.GetImageStream(points.ToList(), imageSize, lineWidth, strokeColor, backgroundColor);
+
+		public void OnDrawingCompleted()


I think it should be internal

I did originally make it internal but the DrawingViewRenderer.wpf.cs class is external to this project.

DrawingViewRenderer.wpf.cs is a sample. and not a part of the plugin, so let's make this method internal

Oh I completely missed that! I can revert the renderer code to use the Command implementation then.

This should now be addressed

VladislavAntonyuk · 2021-07-12T11:02:28Z

src/CommunityToolkit/Xamarin.CommunityToolkit/Views/DrawingView/DrawingView.shared.cs

+			if (!Points.Any())
+				return;
+
+			if (DrawingCompletedCommand?.CanExecute(null) ?? false)


CanExecute(Points)

I thought it would get fixed in a separate PR for this: #1474 but I guess it is a limited change. I thought additional unit tests would have been good though which then increases the size of that change.

you are adding a new code, where actually the bug exists. even if it's just a copy from another part. I think it would be nice to fix it as well

OK Happy to fix. I will add some unit tests covering the bug fix also

This should now be addressed

VladislavAntonyuk · 2021-07-12T11:03:19Z

...nityToolkit/Xamarin.CommunityToolkit/Views/DrawingView/Renderer/DrawingViewRenderer.tizen.cs

-				if (Element.DrawingCompletedCommand?.CanExecute(null) ?? false)
-					Element.DrawingCompletedCommand.Execute(Element.Points);
-			}
+			Element.OnDrawingCompleted();

 			if (Element.ClearOnFinish)


maybe it should be removed

What should be removed?

The Clear function does some tidy up logic in the renderer so I think it needs to stay.

for example on Android, you removed this method, while it stays here.
I understand that you wanted to removed duplicated code and move it to the shared OnDrawingCompleted, but because of such additional logic in Clear method, I think it's better to keep it in all renderers.
I implemented the same in DrawingView MultiLine PR.

VladislavAntonyuk · 2021-07-12T11:03:40Z

...munityToolkit/Xamarin.CommunityToolkit/Views/DrawingView/Renderer/DrawingViewRenderer.uwp.cs

-				if (Element.DrawingCompletedCommand?.CanExecute(null) ?? false)
-					Element.DrawingCompletedCommand.Execute(elementPoints);
-			}
+			Element.OnDrawingCompleted();

 			if (Element.ClearOnFinish)


maybe it should be removed

What should be removed?

If Element.ClearOnFinish is executed in DrawingView.shared.cs

The Clear function does some tidy up logic in the renderer so I think it needs to stay.

…ithub.com:bijington/XamarinCommunityToolkit into feature/add-drawingcompleted-event-to-drawingview

…wingview

jfversluis · 2021-07-16T18:30:12Z

Sorry for the conflicts :( let me know if you want to update or abandon this

VladislavAntonyuk · 2021-07-16T18:52:02Z

It is already implemented in the MultiLine PR

bijington · 2021-07-16T20:04:15Z

I don't think this PR adds any real value now so I'm happy to close it

VladislavAntonyuk · 2021-07-16T20:06:14Z

maybe if we have a minor release before 1.3, it can be included

bijington · 2021-07-16T20:28:33Z

It's entirely up to you. If it was released then I guess this would be better targeting main instead?

jfversluis · 2021-07-20T09:59:56Z

Ah right, this is included in the other one. OK, then we can close this one. There probably won't be a service release for 1.2 :)

Thanks for your efforts though @bijington !

bijington changed the base branch from main to develop July 5, 2021 22:31

bijington added 3 commits July 9, 2021 06:07

DrawingCompleted addition:

f5e51e6

- Centralize the on complete invocation. - Add event to invoke when complete.

Removed shared reference for Points

c325dde

Unit tests for DrawingView

a425206

bijington force-pushed the feature/add-drawingcompleted-event-to-drawingview branch from 70daf60 to a425206 Compare July 9, 2021 05:13

Tidy up of drawing view tests

e239425

bijington marked this pull request as ready for review July 9, 2021 05:17

Merge branch 'develop' into feature/add-drawingcompleted-event-to-dra…

e4d225b

…wingview

VladislavAntonyuk reviewed Jul 12, 2021

View reviewed changes

bijington added 4 commits July 14, 2021 16:22

Made OnDrawingCompleted internal

2a3de4a

Pass Points in to CanExecute

2639560

Merge branch 'feature/add-drawingcompleted-event-to-drawingview' of g…

26ccb5e

…ithub.com:bijington/XamarinCommunityToolkit into feature/add-drawingcompleted-event-to-drawingview

Merge branch 'develop' into feature/add-drawingcompleted-event-to-dra…

dbe6676

…wingview

jfversluis closed this Jul 20, 2021

Feature/add drawingcompleted event to drawingview #1473

Feature/add drawingcompleted event to drawingview #1473

Uh oh!

Conversation

bijington commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Bugs Fixed

API Changes

Behavioral Changes

PR Checklist

Uh oh!

bijington commented Jul 5, 2021

Uh oh!

pictos commented Jul 5, 2021

Uh oh!

VladislavAntonyuk commented Jul 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfversluis commented Jul 16, 2021

Uh oh!

VladislavAntonyuk commented Jul 16, 2021

Uh oh!

bijington commented Jul 16, 2021

Uh oh!

VladislavAntonyuk commented Jul 16, 2021

Uh oh!

bijington commented Jul 16, 2021

Uh oh!

jfversluis commented Jul 20, 2021

Uh oh!

Uh oh!

bijington commented Jul 5, 2021 •

edited

Loading