Created RoundImageEx, Moved ImageEx Core Logic to Base Class #1100

WilliamABradley · 2017-04-15T14:07:33Z

I was looking for a way to clip an ImageEx Control, then I stumbled on to #277 where @JohnnyWestlake suggested restructuring the ImageEx to use ImageBrushes, but didn't want to because it would remove features.

He wanted to have the Original ImageEx Logic and I wanted the ability to make a Circular Image.
I thought, Why don't we have both?
Da, Da Da, Da Da, Da.

I shifted all of the Core Logic to the ImageExBase Base Class, ImageEx retains complete original functionality, and I created the new RoundImageEx to use Ellipses and ImageBrushes.

RoundImageEx loses GetAsCastingSource() & NineGrid, however, it gains the ability to add a Stroke w/ StrokeThickness & the ability to disable Stroke on Placeholder Images.

I also updated the ImageEx Sample Page, although it doesn't demonstrate using Stroke, or other RoundImageEx properties.

…ipped Image. -Moved shared ImageEx Logic to ImageEx base. -RoundImageEx has Properties to add Stroke, StrokeThickness and the ability to disable Stroke on Placeholder Images. -Added RoundImageEx to ImageEx Sample Page.

-Removed Unused Usings.

dnfclas · 2017-04-15T14:07:39Z

@WilliamABradley,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

dnfclas · 2017-04-15T14:13:40Z

@WilliamABradley, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

-Moved Stroke Visual State Strings to RoundImageEx. -Attached PlaceholderSourceChanged (Woops) and removed SetSource as it was copied from ImageEx.Source. -ShowStroke Visual State doesn't seem to work, testing the Stroke animation with EnableDependentAnimation works on the CommonStates, but not in StrokeStates, so I am temporarily setting StrokeThickness binding (ShowPlaceholderStroke does nothting).

WilliamABradley · 2017-04-17T04:30:57Z

It all works except for the View State to show StrokeThickness on the Placeholder Image. Currently I have just set the StrokeThickness to the Placeholder Image, so the ShowPlaceholderStroke bool does nothing.

…t properties now work. -Modified coding formatting to adhere to Style requests. -Fixed missing Bracket.

hermitdave · 2017-04-21T08:43:32Z

@WilliamABradley I think something has wrong very wrong. Another PR is being referenced. That way you need to do is that the 2nd PR must start on dev again

WilliamABradley · 2017-04-21T13:55:26Z

@hermitdave How do you mean? This one was on the dev branch, but I wanted to create a second PR, but it wouldn't let me take a branch off of Microsoft/Dev, so I branched off of Origin/Dev and reverted the changes. How do I fix this mess?

~~Do you mean #1109? as This PR doesn't have any references to this PR, but that PR has references to this PR. Help :( I'm a GitHub noob. If I branch off of dev again it will have all of this code?~~ (Fixed)

hermitdave · 2017-04-21T14:12:41Z

maybe I didn't pay enough attention..

it however looks to me that there are conflicts for
Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/ImageEx.Source.cs
Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/ImageEx.cs

WilliamABradley · 2017-04-21T14:17:47Z

@hermitdave I shifted the Core Logic out of ImageEx into a base class so that a RoundImageEx class can use the shared Logic. (The ImageBrush and Image implementations are different, so an XAML restyle wouldn't work here). ImageEx gets to retain all of it's functionality, and RoundImageEx gets to use ImageCache and most of the features of ImageEx, except it loses Nine Grid and GetAsCasting source, but you can use it to create circular images, which ImageEx can't do.

WilliamABradley · 2017-04-21T15:19:17Z

@hermitdave I Merged back onto Microsoft/Dev if that helps?
Perhaps a better name for this control is ImageBrushEx?

hermitdave · 2017-04-21T21:36:23Z

@WilliamABradley you need to run Please run '.\build.ps1 UpdateHeaders' from powershell to fix issue with file headers. I have reviewed much of the code yesterday. Will go through again over the weekend

WilliamABradley · 2017-04-22T03:29:52Z

@hermitdave I'm getting this issue. Is it compatible with VS 2017? Or do I need something else? Do I need to specify a path parameter?

Odonno · 2017-04-26T11:44:54Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/ImageExBase.Placeholder.cs

+
+            if (e.OldValue == null || e.NewValue == null || !e.OldValue.Equals(e.NewValue))
+            {
+                if (control is RoundImageEx round)


From what I know, C# 7 is not currently available for Continuous Integration. Please change to use C# 6 features such as as.

:( I looked it up and I couldn't find anything about it? It is located in quite a bit of my code, in this and in #1116, so it would be a pain to replace.

@WilliamABradley If you want us to merge your branch, I recommend you to retarget your code to C# 6 only. To clarify, I am not against C# 7 but the fact is that the solution will not compile because CI does not support C# 7 for the moment.

think you might be better off with something like this

RoundImageEx round = control as RoundImageEx if (round != null) { } else { }

@Odonno Fixed it, I'll do the same for my other PR.

…nt, as it caused a RuntimeBinder Exception in .Net Native.

WilliamABradley · 2017-04-29T04:19:31Z

@Odonno @hermitdave I have replaced the C# 7 features with C# 6 compatible features. I have also fixed an issue that caused a crash in .Net Native. I believe that this PR is ready for final review, as it is also now passing in AppVeyor :)

WilliamABradley · 2017-04-29T09:14:29Z

@Odonno What happens now?

hermitdave · 2017-04-29T09:20:22Z

A PR requires 2 reviews, I'll do mine maybe tonight. If all is well, i will merge it in

WilliamABradley · 2017-04-29T09:28:52Z

@hermitdave Awesome! When I get around to it, I also have a few more things I could contribute, such as:

A Helper for ContentDialogs to ensure only one is open at any time, a way to interrupt a dialog with another, if it is more important, and a way to interrupt a dialog and afterwards return to the previous dialog.
Another Helper for Producing a Cortana VCD.xml via Code (Currently CortanaExtensions on GitHub), based on the idea of NotificationExtensions, it allows you to easily populate the VCD at runtime, as well as populate it with translated strings.
A Control similar to AdaptiveGridView, except that it maintains aspect ratio, and uses ViewBoxes to scale the Items.

hermitdave · 2017-04-29T10:07:20Z

@WilliamABradley create issues and uservoice entries for those.

As far as adaptive grid like control, maybe providing that as an option within AdaptiveGridView might be desirable.

ScottIsAFool · 2017-04-29T10:08:57Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/ImageEx.Members.cs

 namespace Microsoft.Toolkit.Uwp.UI.Controls
 {
+    using Windows.Media.Casting;
+    using Windows.UI.Xaml;
+    using Windows.UI.Xaml.Controls;


usings should be outside of the namespace

ScottIsAFool · 2017-04-29T10:09:50Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/ImageExBase.Members.cs

+namespace Microsoft.Toolkit.Uwp.UI.Controls
+{
+    using System;
+    using Windows.UI.Xaml;


usings outside of namespace

ScottIsAFool · 2017-04-29T10:10:19Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/ImageExBase.Placeholder.cs

 namespace Microsoft.Toolkit.Uwp.UI.Controls
 {
+    using Windows.UI.Xaml;


usings outside namespace

ScottIsAFool · 2017-04-29T10:11:46Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/ImageExBase.Placeholder.cs

+
+            if (e.OldValue == null || e.NewValue == null || !e.OldValue.Equals(e.NewValue))
+            {
+                RoundImageEx round = control as RoundImageEx;


Discussion: There should be a virtual OnPlaceholderSourceChanged method that RoundImageEx can override and all this code should go in there, rather than be in the base class.

ScottIsAFool · 2017-04-29T10:12:04Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/ImageExBase.Source.cs

 namespace Microsoft.Toolkit.Uwp.UI.Controls
 {
+    using System;
+    using System.Collections.Generic;
+    using System.Threading;


usings outside namespace

ScottIsAFool · 2017-04-29T10:14:14Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/ImageExFailedEventArgs.cs

 namespace Microsoft.Toolkit.Uwp.UI.Controls
 {
+    using System;


usings outside namespace

ScottIsAFool · 2017-04-29T10:14:25Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/ImageExOpenedEventArgs.cs

 namespace Microsoft.Toolkit.Uwp.UI.Controls
 {
+    using System;


usings outside namespace

ScottIsAFool · 2017-04-29T10:14:33Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/RoundImageEx.Members.cs

+namespace Microsoft.Toolkit.Uwp.UI.Controls
+{
+    using Windows.UI.Xaml;
+    using Windows.UI.Xaml.Media;


usings outside namespace

ScottIsAFool · 2017-04-29T10:15:06Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/RoundImageEx.cs

+namespace Microsoft.Toolkit.Uwp.UI.Controls
+{
+    using Windows.UI.Xaml;
+    using Windows.UI.Xaml.Media;


usings outside namespace

ScottIsAFool · 2017-04-29T10:15:40Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/RoundImageEx.xaml

@@ -0,0 +1,125 @@
+<ResourceDictionary xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"


Ensure xaml styler has been run on this.

-Made OnPlaceholdersource changed, marked virtual to be overridden. -ShowPlaceholderStroke still disabled as ViewState doesn't work.

WilliamABradley · 2017-04-29T10:38:24Z

@ScottIsAFool Those changes have been made, however, ShowPlaceholderStroke currently does nothing, because I couldn't get it to work. The StrokeThickness on PlaceholderEllipse should default to 0, With the Visual State ShowStroke setting it to the desired Stroke value. Any ideas as to why it doesn't work?

ScottIsAFool · 2017-04-29T13:54:05Z

@WilliamABradley you can't have a binding in the To value when used inside a control template. This would have to be set in code, sadly.

WilliamABradley · 2017-04-29T13:56:18Z

@ScottIsAFool How would that be done? Would you attach an x:Name to the Animation so you update the value when the StrokeThickness changes, or some other way?

ScottIsAFool · 2017-04-29T14:01:26Z

@WilliamABradley to be honest, looking at it, I'm not sure any of that is needed. Your ellipse's StrokeThickness is already being set and that should suffice. I don't think the ShowPlaceholderStroke property is even needed. I'm going to add a couple more comments to my review.

ScottIsAFool · 2017-04-29T14:01:38Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/RoundImageEx.xaml

+                                 HorizontalAlignment="{TemplateBinding HorizontalAlignment}"
+                                 VerticalAlignment="{TemplateBinding VerticalAlignment}"
+                                 Opacity="1.0"
+                                 Stroke="{Binding RelativeSource={RelativeSource TemplatedParent}, Path=Stroke}"


This should just be a TemplateBinding

This is due to a bug in ImageBrushes http://stackoverflow.com/questions/22134281/templatebinding-fails-when-the-target-is-an-imagebrush-imagesource

ScottIsAFool · 2017-04-29T14:01:49Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/RoundImageEx.xaml

+                                 VerticalAlignment="{TemplateBinding VerticalAlignment}"
+                                 Opacity="1.0"
+                                 Stroke="{Binding RelativeSource={RelativeSource TemplatedParent}, Path=Stroke}"
+                                 StrokeThickness="{Binding RelativeSource={RelativeSource TemplatedParent}, Path=StrokeThickness}">


This should also just be a TemplateBinding

This is due to a bug in ImageBrushes http://stackoverflow.com/questions/22134281/templatebinding-fails-when-the-target-is-an-imagebrush-imagesource

I'm not sure this is applicable. From what that SO says, this is because the OP needed to basically convert to another value. As long as Stroke and StrokeThickness on RoundImageEx are using the same property types as Ellipse, then no conversion should be needed and TemplateBinding can be used.

Woops, It was only for the ImageBrushes anyway, not the Properties on the Ellipse.

ScottIsAFool · 2017-04-29T14:02:22Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/RoundImageEx.xaml

+                                </VisualState>
+                                <VisualState x:Name="Unloaded" />
+                            </VisualStateGroup>
+                            <VisualStateGroup x:Name="StrokeStates">


I'm not sure this VisualState is even needed.

ScottIsAFool · 2017-04-29T14:03:14Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/RoundImageEx.Members.cs

+            DependencyProperty.Register(nameof(StrokeProperty), typeof(Brush), typeof(RoundImageEx), new PropertyMetadata(default(Brush)));
+
+        // Using a DependencyProperty as the backing store for ShowPlaceholderStroke.  This enables animation, styling, binding, etc...
+        public static readonly DependencyProperty ShowPlaceholderStrokeProperty =


Is this property even needed? If the dev doesn't want the placeholder to have a stroke, then the StrokeThickness should just be set to 0.

Makes sense

ScottIsAFool · 2017-04-29T14:03:49Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/RoundImageEx.Members.cs

+    {
+        // Using a DependencyProperty as the backing store for StrokeThickness.  This enables animation, styling, binding, etc...
+        public static readonly DependencyProperty StrokeThicknessProperty =
+            DependencyProperty.Register(nameof(StrokeThickness), typeof(double), typeof(RoundImageEx), new PropertyMetadata(default(double)));


This property should really be using Thickness, not double as its type.

The StrokeThickness on an Ellipse is a double Property, this is just a pass-through. It wouldn't make sense for a circle to have differing thickness on different sides. It has no sides,

Interesting, ignore this. Apparently for Ellipse StrokeThickness uses double, not Thickness, like everywhere else does 8-)

-Fixed StrokeProperty. -Replaced Bindings with TemplateBindings for Stroke and Thickness for Ellipses.

nmetulev

Looks great.

Only request I have is about documentation. The sample page should be more clear that these are two different controls. At minimum, the code should have code for both controls.

Also, could you update the documentation for ImageEx to include this control?

hermitdave · 2017-05-22T11:20:57Z

Microsoft.Toolkit.Uwp.UI.Controls/ImageEx/RoundImageEx.xaml

+            <Setter.Value>
+                <ControlTemplate TargetType="controls:RoundImageEx">
+                    <Grid>
+                        <Ellipse x:Name="PlaceholderEllipse"


I know its a bit late in the day. Any reason we are using Ellipse instead of Border ? Border would allow you to have image with rounded corner rather than elliptical images which is what default will be unless the user were to go override the XAML.

Personally Image with CornerRadius better than PeoplePicture (the one coming out in FCU)

I guess that could work too, because you could set the corner radius to say 999, in order to create a round image anyway right?

Its not difficult to create rounded image by tweaking corner radius. I am not sure if there's a default value that forces circular ellipse but my point is Border with RoundedCorner can be turned into fully circular image if desired.
Ellipse can't do that. Any thoughts on why we should go for one or the other ?

The Control name wouldn't make sense anymore. I could name it ImageBrushEx, although it would still be encapsulated so it wouldn't really be an ImageBrush you could attach to things,

good point about naming. @nmetulev @ScottIsAFool @Odonno any thoughts on this ?

Another thing is, without setting the corner radius, it behaves practically the same as ImageEx, but without NineGrid and GetAsCastingSource

…rnerRadius as a larger number, e.g. 999. -Setting Stroke is replaced with Setting BorderBrush and BorderThickness. -Added RoundImageEx to ImageEx.md, as well as to the .bind file.

WilliamABradley · 2017-05-22T12:05:37Z

@nmetulev I have updated the documentation.

nmetulev · 2017-05-22T17:48:35Z

Thanks :)

WilliamABradley added 3 commits April 16, 2017 01:36

-Added Licenses.

f3669aa

-Removed Unused Usings.

-Fixed Typo.

d0d4d5d

dnfclas added the cla-required label Apr 15, 2017

dnfclas added cla-signed and removed cla-required labels Apr 15, 2017

WilliamABradley mentioned this pull request Apr 16, 2017

Update ImageEx for non-rectangular uses #1013

Closed

WilliamABradley added 2 commits April 17, 2017 16:20

Removed K

504b8b5

-Removed the need for MaskWidth and MaskHeight, using Width and Heigh…

4e7ccc3

…t properties now work. -Modified coding formatting to adhere to Style requests. -Fixed missing Bracket.

nmetulev requested a review from hermitdave April 17, 2017 17:37

nmetulev added this to the v1.5 milestone Apr 17, 2017

nmetulev added the reviewers wanted label Apr 17, 2017

WilliamABradley mentioned this pull request Apr 19, 2017

TextToolbar for RichEditBox #1109

Closed

10 tasks

-Merged with Microsoft/Dev

df945d1

-Fixed Licenses and Namespaces.

2e13838

Odonno suggested changes Apr 26, 2017

View reviewed changes

WilliamABradley added 2 commits April 27, 2017 01:05

-Removed Pattern Matching.

70d5de3

-Replaced Dynamic Event Handler Assignment with Type Verfied Assignme…

ed07ec7

…nt, as it caused a RuntimeBinder Exception in .Net Native.

-Simplified AttachSource.

ee2d42b

Odonno approved these changes Apr 29, 2017

View reviewed changes

ScottIsAFool reviewed Apr 29, 2017

View reviewed changes

-Moved Usings Location outside of namespace.

86d6788

-Made OnPlaceholdersource changed, marked virtual to be overridden. -ShowPlaceholderStroke still disabled as ViewState doesn't work.

ScottIsAFool reviewed Apr 29, 2017

View reviewed changes

WilliamABradley added 2 commits April 30, 2017 02:14

-Removed ShowPlaceholderStroke, Removed StrokeStates,

6ac21f6

-Properly Removed ShowPlaceholder Stroke.

014a3f4

-Fixed StrokeProperty. -Replaced Bindings with TemplateBindings for Stroke and Thickness for Ellipses.

nmetulev requested changes May 22, 2017

View reviewed changes

hermitdave reviewed May 22, 2017

View reviewed changes

-Replaced Ellipses with Borders, to achieve full round effect, set Co…

7fb9286

…rnerRadius as a larger number, e.g. 999. -Setting Stroke is replaced with Setting BorderBrush and BorderThickness. -Added RoundImageEx to ImageEx.md, as well as to the .bind file.

hermitdave approved these changes May 22, 2017

View reviewed changes

nmetulev approved these changes May 22, 2017

View reviewed changes

nmetulev merged commit efe7217 into CommunityToolkit:dev May 22, 2017

JohnnyWestlake mentioned this pull request May 24, 2017

[Feature Request] ImageBrushEx in 15063 #1179

Closed

		@@ -0,0 +1,125 @@
		<ResourceDictionary xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"

Created RoundImageEx, Moved ImageEx Core Logic to Base Class #1100

Created RoundImageEx, Moved ImageEx Core Logic to Base Class #1100

Conversation

WilliamABradley commented Apr 15, 2017 • edited Loading

dnfclas commented Apr 15, 2017

dnfclas commented Apr 15, 2017

WilliamABradley commented Apr 17, 2017

hermitdave commented Apr 21, 2017

WilliamABradley commented Apr 21, 2017 • edited Loading

hermitdave commented Apr 21, 2017

WilliamABradley commented Apr 21, 2017

WilliamABradley commented Apr 21, 2017

hermitdave commented Apr 21, 2017

WilliamABradley commented Apr 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamABradley Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

WilliamABradley commented Apr 29, 2017

WilliamABradley commented Apr 29, 2017

hermitdave commented Apr 29, 2017

WilliamABradley commented Apr 29, 2017 • edited Loading

hermitdave commented Apr 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamABradley commented Apr 29, 2017

ScottIsAFool commented Apr 29, 2017

WilliamABradley commented Apr 29, 2017

ScottIsAFool commented Apr 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmetulev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamABradley commented May 22, 2017

nmetulev commented May 22, 2017

WilliamABradley commented Apr 15, 2017 •

edited

Loading

WilliamABradley commented Apr 21, 2017 •

edited

Loading

WilliamABradley commented Apr 22, 2017 •

edited

Loading

WilliamABradley Apr 26, 2017 •

edited

Loading

WilliamABradley commented Apr 29, 2017 •

edited

Loading