Skip to content
This repository was archived by the owner on May 5, 2021. It is now read-only.

Conversation

VladislavAntonyuk
Copy link

No description provided.

@msftclas
Copy link

msftclas commented Jun 1, 2018

CLA assistant check
All CLA requirements met.

@VladislavAntonyuk
Copy link
Author

@mattleibow Please review

@mattleibow
Copy link
Contributor

Is there a reason you are using a pre-release of forms? I would like to avoid this if possible.

@mattleibow
Copy link
Contributor

build


namespace SignaturePad.Forms
{
[RenderWith (typeof (SignaturePadCanvasRenderer))]
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this break the renderer?

Copy link
Author

Choose a reason for hiding this comment

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

The page is not rendered at all, and throw the exeption unable to cast to Xamarin.Forms.IRegistable

Copy link
Contributor

Choose a reason for hiding this comment

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

But this problem should go away now since we have a WPF renderer?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I have returned it

@VladislavAntonyuk
Copy link
Author

I'll recheck with latest stable Xamarin forms

Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

This looks good so far, but a few changes to make the code a bit cleaner.

  • is it possible to use the floating point types? eg PointF, SizeF and RectangleF. This will reduce the need for the #if NET471
  • add some magic extension methods to hide the logic and keep the main code clean.
  • a new define WINDOWS_DESKTOP or even `WINDOWS_WPF (just in case we go Windows.Forms)
  • use a stable version of Forms if we can.

<PackageReference Include="MSBuild.Sdk.Extras" Version="1.4.0" PrivateAssets="All" />
<PackageReference Include="Xamarin.Forms" Version="2.5.0.280555" />
<PackageReference Include="MSBuild.Sdk.Extras" Version="1.5.4" PrivateAssets="All" />
<PackageReference Include="Xamarin.Forms" Version="3.1.0.469394-pre1" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use a stable version of forms here?

Copy link
Author

Choose a reason for hiding this comment

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

yes, I am in progress

using Android.Widget;
using Xamarin.Forms.Platform.Android;
using NativeColor = Android.Graphics.Color;
#elif NET471
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it may be better to declare a WINDOWS_DESKTOP define and use that - if we ever change the .net version, then we will have to update code all over the place.

Copy link
Author

Choose a reason for hiding this comment

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

NETFRAMEWORK variable goes by default. I use it

#elif NET471
using Xamarin.Forms.Platform.WPF;
using NativeSignaturePadCanvasView = Xamarin.Controls.SignaturePadCanvasView;
using NativePoint = System.Drawing.Point;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why we can't use PointF?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I just noticed that you are using System.Drawing.Xxx is it possible to use the WPF types instead? System.Windows.Point, System.Windows.Size and System.Windows.Rect.

I had a look at the code, and they are using the WPF Point, Size and Rect - which are double based.

Copy link
Author

Choose a reason for hiding this comment

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

it must be styluspoint as default for wpf, my mistake

if (ctrl != null)
{
#if NET471
ctrl.LoadPoints (e.Points.Select (p => new NativePoint ((int)p.X, (int)p.Y)).ToArray ());
Copy link
Contributor

Choose a reason for hiding this comment

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

If it is possible to use PointF and SizeF, then nothing has to change here.

Copy link
Author

Choose a reason for hiding this comment

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

stylus point works ok with float so it is redundant and removed


namespace SignaturePad.Forms
{
[RenderWith (typeof (SignaturePadCanvasRenderer))]
Copy link
Contributor

Choose a reason for hiding this comment

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

But this problem should go away now since we have a WPF renderer?

}

paths.Add (new InkStroke (newpath, strokePoints, color, width));
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use extension methods here as well? See the other platforms: https://github.com/xamarin/SignaturePad/blob/master/src/SignaturePad.Shared/Extensions.cs

All smoke and mirrors, but I would like to keep these files clean of #if so it is easier to modify and understand.

Copy link
Author

Choose a reason for hiding this comment

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


#if NET471
var size = this.RenderSize;
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

{
signatureBounds = new NativeRect (0, 0, (float)viewSize.Width, (float)viewSize.Height);
}
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for this copy?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, I see. floats, ints and types... hopefully we can use the XxxF

Copy link
Author

Choose a reason for hiding this comment

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

I've just updated review, and remove all redundant # where it possible

Copy link
Author

Choose a reason for hiding this comment

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

viewSize Width is double so we can't avoid casting

private static readonly NativeColor SignaturePadLightColor = Windows.UI.Colors.White;
#elif NET471
private static readonly NativeColor SignaturePadDarkColor = new NativeColor () { A = System.Drawing.Color.Black.A, B = System.Drawing.Color.Black.B, G = System.Drawing.Color.Black.G, R = System.Drawing.Color.Black.R };
private static readonly NativeColor SignaturePadLightColor = new NativeColor () { A = System.Drawing.Color.White.A, B = System.Drawing.Color.White.B, G = System.Drawing.Color.White.G, R = System.Drawing.Color.White.R };
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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


namespace Xamarin.Controls
{
partial class InkPresenter : System.Windows.Ink.StrokeCollection
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rather than a new type, is it possible to just add a using?
using InkPresenter = System.Windows.Ink.StrokeCollection

Copy link
Author

Choose a reason for hiding this comment

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

removed as redundant

@mattleibow
Copy link
Contributor

@VladislavAntonyuk looks good so far, looking forward to merging this for the world.

@mattleibow
Copy link
Contributor

Build

@VladislavAntonyuk
Copy link
Author

Could not load file or assembly 'netstandard, Version=2.0.0.0 Please add it on your CI

@VladislavAntonyuk
Copy link
Author

@mattleibow do you have any objections not to merge it?

@mattleibow
Copy link
Contributor

No, but I have been travelling and not had a chance to do a more detailed review and have a look at it running. I will be back in the office next week, so I hope to merge then.

@VladislavAntonyuk
Copy link
Author

VladislavAntonyuk commented Mar 28, 2020

@adampenn I have push new changes. Make sure you have install gtk, uwp 18362, android sdk 29. I suggest you to use build.ps1 or build.sh to generate nuget package

@VladislavAntonyuk
Copy link
Author

I setup CI, you can download artifacts there (see readme in my fork).

<file src="output/uwp/Themes/Generic.xbf" target="lib/uap10.0/SignaturePad/Themes/Generic.xbf" />
<file src="output/uwp/SignaturePad.xr.xml" target="lib/uap10.0/SignaturePad/SignaturePad.xr.xml" />
<file src="output/uwp/SignaturePad/Themes/Generic.xaml" target="lib/uap10.0/SignaturePad/Themes/Generic.xaml" />
<file src="output/uwp/SignaturePad/SignaturePad.xr.xml" target="lib/uap10.0/SignaturePad/SignaturePad.xr.xml" />

Choose a reason for hiding this comment

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

These two files dont seem to be ending up in the output directory

Copy link
Author

Choose a reason for hiding this comment

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

build output for uwp
image

Choose a reason for hiding this comment

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

I apologize, I was not clear, I see them there but I do not see them in the nuget package. I see it in the signaturePad nuget package but not the signaturepad.forms package
image

image

Makes me think its just not getting copied to one of the packages

Copy link
Author

Choose a reason for hiding this comment

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

I would appreciate, if you create a PR, so I will check I setup permissions right.

VladislavAntonyuk and others added 2 commits May 31, 2020 16:16
Bumps Win2D.uwp from 1.24.0 to 1.25.0.

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
dependabot-preview bot and others added 7 commits November 21, 2020 01:02
…#6)

Bumps [Microsoft.NETCore.UniversalWindowsPlatform](https://github.com/Microsoft/dotnet) from 6.2.10 to 6.2.11.
- [Release notes](https://github.com/Microsoft/dotnet/releases)
- [Commits](https://github.com/Microsoft/dotnet/commits)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
…#11)

Bumps [Microsoft.NETCore.UniversalWindowsPlatform](https://github.com/Microsoft/dotnet) from 6.2.11 to 6.2.12.
- [Release notes](https://github.com/Microsoft/dotnet/releases)
- [Commits](https://github.com/Microsoft/dotnet/commits)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Bumps Win2D.uwp from 1.25.0 to 1.26.0.

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Bumps Win2D.uwp from 1.25.0 to 1.26.0.

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Vladislav Antonyuk <33021114+VladislavAntonyuk@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants