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

Drawing (vector icons) #1117

Merged
merged 8 commits into from Aug 20, 2017

Conversation

Projects
None yet
2 participants
@aelij
Member

aelij commented Aug 20, 2017

Drawing is a convenient way to create vector icons, used by WPF. The Visual Studio Image Library provides many icons as Drawings, and there are other tools that can produce them. They are much more lightweight compared to Shape controls since they are not part of the visual tree.

Here's a sample Avalonia rendering:
image

The bottom are Ellipse controls I thought to use for Stretch reference, but it looks like there's a bug there. It's weird since I'm using the exact same transform from Shape.

Sample WPF rendering:

image

There's also an issue of required clipping in UniformToFill. Any idea how to do that?

Note that I created a control called DrawingPresenter since Avalonia doesn't have an ImageSource base class that supports both vector and bitmaps. Perhaps it's worth considering something like this so that vector images could be rendered anywhere bitmaps can (e.g. ImageBrush, Image).

@grokys

grokys approved these changes Aug 20, 2017

This looks great! I've got a few comments that are mainly style related and don't reflect on the functionality so I'm approving anyway, if you have time to address them great, if not it's no problem; I can do it if you like. Regarding the issues in the PR description - I think we should merge this and address them separately.

set => SetValue(DrawingProperty, value);
}
public static readonly StyledProperty<Stretch> StretchProperty =

This comment has been minimized.

@grokys

grokys Aug 20, 2017

Member

Nit: we usually define the properties at the top of the file rather than intermixing them with the property getter/setters.

set => SetValue(StretchProperty, value);
}
static DrawingPresenter()

This comment has been minimized.

@grokys

grokys Aug 20, 2017

Member

Nit: constructors should go above properties.

[Content]
public AvaloniaList<Drawing> Children { get; } = new AvaloniaList<Drawing>();
public static readonly StyledProperty<double> OpacityProperty =

This comment has been minimized.

@grokys

grokys Aug 20, 2017

Member

Nit: again, ordering. I (personally) tend to use http://stylecop.soyuz5.com/SA1201.html as the basis for ordering. This isn't a hard-and-fast rule, but just for reference.

This comment has been minimized.

@aelij

aelij Aug 20, 2017

Member

Perhaps you can add the StyleCop Roslyn analyzers to the project

This comment has been minimized.

@grokys

grokys Aug 20, 2017

Member

Yeah, we used to have them, but there were complaints that it was too strict and caused too much distraction... Personally I'd like to add them back but that would hit resistance. Not sure what the best way to handle style-related things is, but for now I just try to point them out in PRs, which isn't ideal as they're not really documented or enforced.

static PolylineGeometry()
{
PointsProperty.Changed.Subscribe(onNext: v =>

This comment has been minimized.

@grokys

grokys Aug 20, 2017

Member

I think this could be better expressed as:

PointsProperty.Changed.AddClassHandler<PolylineGeometry>((s, e) => 
    s.OnPointsChanged(e.OldValue as Points, e.NewValue as Points));
{
(v.Sender as PolylineGeometry)?.OnPointsChanged(v.OldValue as Points, v.NewValue as Points);
});
IsFilledProperty.Changed.AddClassHandler<PolylineGeometry>(x => a => x.NotifyChanged());

This comment has been minimized.

@grokys

grokys Aug 20, 2017

Member

I don't even understand how this works!? I'd write it as:

IsFilledProperty.Changed.AddClassHandler<PolylineGeometry>((s, _) => s.NotifyChanged());

This comment has been minimized.

@aelij

aelij Aug 20, 2017

Member

It works because it uses the overload that accepts a Func<TTarget, Action<AvaloniaPropertyChangedEventArgs>>. It's the same as writing x => x.SomeMethod, but instead, SomeMethod is a lambda. Anyway, I didn't know the other overload existed :)

protected set => base.PlatformImpl = value;
}
private Points _points;

This comment has been minimized.

@grokys

grokys Aug 20, 2017

Member

Fields go at the top.

{
if (Drawing != null)
{
using (context.PushPreTransform(_transform))

This comment has been minimized.

@grokys

grokys Aug 20, 2017

Member

I think to address the issue of required clipping in UniformToFill you could just push a clip here?

aelij added some commits Aug 20, 2017

@aelij aelij merged commit 4885ef8 into AvaloniaUI:master Aug 20, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aelij aelij deleted the aelij:drawing branch Aug 20, 2017

@grokys grokys added this to the Beta 1 milestone Jan 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment