Add a MosaicControl to repeat an image #702

samoteph · 2016-12-08T23:24:25Z

As asked in the issue #658, I propose MosaicControl.
It works with XAML or Composition (Composition is the default).
You can animate the image or synchronize it with a ScrollViewer to create a parallax effect.

dnfclas · 2016-12-08T23:24:28Z

Hi @samoteph, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

dnfclas · 2016-12-08T23:42:07Z

@samoteph, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

deltakosh · 2016-12-09T16:30:36Z

Microsoft.Toolkit.Uwp.UI.Controls/project.json

@@ -1,6 +1,7 @@
 {
  "dependencies": {
    "Microsoft.NETCore.UniversalWindowsPlatform": "5.1.0",
+    "Robmikh.CompositionSurfaceFactory": "0.7.0-alpha",


do you mind updating the nuspec in /build folder?

Sorry but I don't understand what must i do.
Could you give me some hint ?

Sorry I missed this question. Can you update this file: https://github.com/Microsoft/UWPCommunityToolkit/blob/dev/build/Microsoft.Toolkit.Uwp.UI.Controls.nuspec

deltakosh · 2016-12-09T16:32:56Z

Microsoft.Toolkit.Uwp.UI.Controls/Microsoft.Toolkit.Uwp.UI.Controls.csproj

@@ -193,6 +198,7 @@
  <ItemGroup>
    <EmbeddedResource Include="Properties\Microsoft.Windows.Toolkit.UI.Controls.rd.xml" />
  </ItemGroup>
+  <ItemGroup />


not required :)

deltakosh · 2016-12-09T16:36:00Z

Microsoft.Toolkit.Uwp.UI.Controls/MosaicControl/MosaicControl.cs

+        /// <summary>
+        /// MosaicControl is created with XAML
+        /// </summary>
+        PureXaml,


Why exposing this? I understand the need to use Xaml when composition is not available (and I love this fallback) but why as an user could I change it?

Good point ! I'll change the exposition of the enum to private.

deltakosh · 2016-12-09T16:36:26Z

Microsoft.Toolkit.Uwp.UI.Controls/MosaicControl/MosaicControl.cs

+        }
+
+        // Using a DependencyProperty as the backing store for ScrollViewer.  This enables animation, styling, binding, etc...
+        public static readonly DependencyProperty ScrollViewerContainerProperty =


We try to keep all DP first in the file (sorry for being picky)

deltakosh · 2016-12-09T16:37:30Z

Microsoft.Toolkit.Uwp.UI.Controls/MosaicControl/MosaicControl.cs

+            this.RefreshMove(this.OffsetX + this.animationX, this.OffsetY + this.animationY);
+        }
+
+        object lockerOffset = new object();


must define an access modifier

deltakosh · 2016-12-09T16:37:55Z

Microsoft.Toolkit.Uwp.UI.Controls/MosaicControl/MosaicControl.cs

+                }
+            }
+
+            // retire les sprites visual en trop


Comments must be in english (But I agree with you, it is a shame :))

Never again French words in this page !

deltakosh · 2016-12-09T16:39:33Z

Microsoft.Toolkit.Uwp.UI.Controls/MosaicControl/MosaicControl.cs

+        /// When the control is in Design mode, Composition is not allowed.
+        /// </summary>
+        /// <returns>the UIStrategy choosed</returns>
+        private UIStrategy GetUIStrategy()


Why not a getter?

I remove the DP UIStrategy and replace by a simple property with a getter.

Perfect. A cool .md in the doc folder and we are good to go

deltakosh · 2016-12-09T16:41:07Z

Impressive control!! Just some tiny comments and a documentation and we are good to go :)

…e + Documentation

JohnnyWestlake · 2016-12-12T11:20:58Z

Microsoft.Toolkit.Uwp.UI.Controls/MosaicControl/MosaicControl.cs

+            if (rootElement != null)
+            {
+                this.rootElement = rootElement;
+                rootElement.SizeChanged += RootElement_SizeChanged;


OnApplyTemplate() can get called multiple times, so always safer to unhook before hooking any events in here,

Thanks for the tips !

JohnnyWestlake · 2016-12-12T11:26:49Z

Microsoft.Toolkit.Uwp.UI.Controls/MosaicControl/MosaicControl.xaml

+                    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
+                    xmlns:controls="using:Microsoft.Toolkit.Uwp.UI.Controls">
+    <Style TargetType="controls:MosaicControl">
+        <Setter Property="Template">


For the benefit of Xbox One controller navigation (and keyboard navigation), can we add <Setter Property="IsTabStop" Value="False" /> to this? Whilst it's content is fine to be focusable I don't think there's any reason the container should be by default.

JohnnyWestlake · 2016-12-12T11:29:18Z

Microsoft.Toolkit.Uwp.UI.Controls/MosaicControl/MosaicControl.cs

+    {
+        // Using a DependencyProperty as the backing store for ScrollViewer.  This enables animation, styling, binding, etc...
+        public static readonly DependencyProperty ScrollViewerContainerProperty =
+            DependencyProperty.Register("ScrollViewerContainer", typeof(FrameworkElement), typeof(MosaicControl), new PropertyMetadata(null, OnScrollViewerContainerChange));


Inline with the style of some of the other controls in the project, you may want to consider DependencyProperty.Register(nameof(ScrollViewerContainer).... rather than DependencyProperty.Register("ScrollViewerContainer"...

(same applies for the rest of the DP's)

…alse

skendrot

Also: Remove uses of this.
It would be good to have an image that looks good as it's tiled.

skendrot · 2016-12-12T15:28:53Z

Microsoft.Toolkit.Uwp.UI.Controls/MosaicControl/MosaicControl.xaml

+                    <Grid>
+                        <Border                        
+                        x:Name="RootElement"                        
+                        Background="{TemplateBinding Background}"


Template bind the border properties as well

skendrot · 2016-12-12T15:59:28Z

Microsoft.Toolkit.Uwp.UI.Controls/MosaicControl/MosaicControl.cs

+        /// <summary>
+        /// Gets or sets an X offset of the image
+        /// </summary>
+        public double OffsetX


Why add new offset properties instead of just using margin?

OffsetX and OffsetY are not destined to manage the layout but the starting position of the repeated image. It's like properties AlignmentX and AlignmentY of the ImageBrush class in WPF.

skendrot · 2016-12-12T16:01:24Z

Microsoft.Toolkit.Uwp.UI.Controls/MosaicControl/MosaicControl.cs

+        public static readonly DependencyProperty AnimationDurationProperty =
+            DependencyProperty.Register(nameof(AnimationDuration), typeof(double), typeof(MosaicControl), new PropertyMetadata(30.0, OnAnimationDuration));
+
+        private FrameworkElement rootElement = null;


name private member variables with _pascalCase

skendrot · 2016-12-12T16:05:07Z

Microsoft.Toolkit.Uwp.UI.Controls/MosaicControl/MosaicControl.cs

+            /// <summary>
+            /// MosaicControl is created with XAML
+            /// </summary>
+            PureXaml,


Why support xaml at all for something like this? Rendering and memory would be too much

The rendering with XAML has been created at first for the XAML Viewer (when it'll be functionnal ^^) and after for version that doesn't support Composition. As it use a unique ImageBrush it is not too expensive for the memory consumption and the performances are not bad.

deltakosh

Good to go for me..

deltakosh · 2016-12-13T18:08:55Z

@skendrot, @JohnnyWestlake ok for the merge?

skendrot · 2016-12-13T18:31:40Z

Not too excited about the new dependency on Robmikh.CompositionSurfaceFactory

deltakosh · 2016-12-13T18:34:41Z

Robert is a developer on the Windows Composition team so I'm fine with this dependency.

samoteph · 2016-12-13T22:19:47Z

Maybe Robert would be OK to integrate his CompositionSurfaceFactory in the toolkit :)

deltakosh · 2016-12-13T22:21:27Z

Ping @robmikh :)

deltakosh · 2016-12-14T18:36:12Z

perhaps stupid question but can we use this instead?
https://github.com/Microsoft/UWPCommunityToolkit/pull/719/files#diff-566172c706c8b43eb5a488b17ca21bddR37

samoteph · 2016-12-15T09:22:21Z

This is not a stupid question at all. In case of a Device Lost, the image must be reloaded. CompositionSurfaceFactory does all the work for us. SurfaceLoader doesn't (I guess).

XamlBrewer · 2016-12-15T09:36:48Z

There's also a huge performance gain: CompositionSurfaceFactory loads images a lot faster than SurfaceLoader.

robmikh · 2016-12-15T10:13:35Z

Hi sorry, I missed this in the torrent of notifications! :)

I have no qualms about adding CompositionSurfaceFactory, I'm just not sure how it would work. It's currently a C++/CX component, so I'm guessing it would become its own package in a sub-namespace? Just know that I don't remember if ID3D11Device4 is an Anniversary Update interface or not, and it's what I use to get proper device lost events.

I've also recently added GetSharedSurfaceFactoryForCompositor so that it's easier for multiple components to all use the same SurfaceFactory object. That way there will only be one D3D device created by the app.

deltakosh · 2016-12-15T15:25:52Z

No worry :)
@shenchauhan Do you think you can use @robmikh code to support DeviceLost? (I try to define if we can avoid this external dependency and only use your SurfaceLoader)
If we cannot (And base on @XamlBrewer mention there is also a perf issue), we should remove the SurfaceLoader and only use CompositionSurfaceFactory

samoteph · 2016-12-21T15:48:08Z

Up :)

deltakosh · 2016-12-21T17:15:09Z

I think @shenchauhan is working on removing surfaceloader

deltakosh · 2016-12-21T17:15:27Z

it 's a go for me. just a second reviewer

deltakosh · 2017-01-03T19:11:02Z

Do you mind removing conflicts? (pretty please :))

…as Robmikh has suggested

samoteph · 2017-01-03T20:25:36Z

Done ?

deltakosh · 2017-01-03T20:27:10Z

ok trying to summon another reviewer:) I'll merge else

samoteph · 2017-01-03T20:31:47Z

Oky :)

shenchauhan

A couple of comments around styling and also possibly consider using parallax that we will be shipping in the toolkit.

shenchauhan · 2017-01-03T19:26:36Z

Microsoft.Toolkit.Uwp.UI.Controls/MosaicControl/MosaicControl.cs

+    /// </summary>
+    public sealed class MosaicControl : ContentControl
+    {
+        private FrameworkElement rootElement = null;


Please use the .NET Core coding stlyes mentioned here: https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md

so it should be _rootElement

shenchauhan · 2017-01-03T20:41:11Z

Microsoft.Toolkit.Uwp.UI.Controls/MosaicControl/MosaicControl.cs

+        /// <param name="e">arguments</param>
+        private async void RootElement_SizeChanged(object sender, SizeChangedEventArgs e)
+        {
+            Debug.WriteLine("sizeChanged=" + e.NewSize.Width);


Remove the Debug.WriteLine

shenchauhan · 2017-01-03T20:55:00Z

Microsoft.Toolkit.Uwp.UI.Controls/MosaicControl/MosaicControl.cs

+                    {
+                        var speedRatio = this.ParallaxSpeedRatio;
+
+                        scrollX = -((scrollviewer.HorizontalOffset * scrollviewer.ActualWidth) / scrollviewer.ViewportWidth) * speedRatio;


Would it better to use the parallax behavior that's going into the Toolkit? (I think there is a PR for it already) This way there is a consistent way for people to adjust the parallax effect across the toolkit. Appreciate that the PR hasn't gone through yet, but thought I would ask the question here in case you weren't aware.

the Mosaic control integrate an infinite scrolling effect. this is made by using a Composition expression. this expression manage the offset modulo (included negative value) of the scrolling. This expression is used for manual move (OffsetX, OffsetY) + parallax effect and this is the core of the control. The parallax Service is a great helper to create quickly a parallax effect but in this case the scenario is too complicated I think for be used internally.

deltakosh · 2017-01-04T04:19:28Z

Do you mind removing conflicts? (pretty please :))
I promise to merge after that :)

….git

shenchauhan

Looks good! 👍 Nice control.

samoteph · 2017-01-12T08:53:13Z

Thanks guys !

Integration of MosaicControl

2140ad4

dnfclas added the cla-required label Dec 8, 2016

dnfclas added cla-signed and removed cla-required labels Dec 8, 2016

deltakosh reviewed Dec 9, 2016

View reviewed changes

deltakosh added reviewers wanted need documentation 📃 labels Dec 9, 2016

Minor change to be complient with the UWPCommunityToolkit coding styl…

6a78f4a

…e + Documentation

JohnnyWestlake reviewed Dec 12, 2016

View reviewed changes

samuel.blanchard added 3 commits December 12, 2016 13:22

nameof on DP

46b03e0

OnApplyTemplate() can be called multiple times + XBOX One IsTabStop=F…

951ebe1

…alse

Modification of the documentation (link to the gif)

923c592

skendrot reviewed Dec 12, 2016

View reviewed changes

remove "this" and add _ for privates variables

87f3cce

deltakosh removed the need documentation 📃 label Dec 12, 2016

deltakosh approved these changes Dec 12, 2016

View reviewed changes

Replace CreateFromCompositor by GetSharedSurfaceFactoryForCompositor …

1870b03

…as Robmikh has suggested

shenchauhan requested changes Jan 3, 2017

View reviewed changes

Debug.WriteLine

f22c637

samoteph and others added 3 commits January 5, 2017 10:53

Merge branch 'dev' into dev

4f7bd20

Add the nuget Robmikh.CompositionSurfaceFactory as dependency

4ea78e8

Merge branch 'dev' of https://github.com/samoteph/UWPCommunityToolkit…

628fee6

….git

shenchauhan approved these changes Jan 5, 2017

View reviewed changes

shenchauhan merged commit c1c72e3 into CommunityToolkit:dev Jan 5, 2017

Add a MosaicControl to repeat an image #702

Add a MosaicControl to repeat an image #702

Conversation

samoteph commented Dec 8, 2016

dnfclas commented Dec 8, 2016

dnfclas commented Dec 8, 2016

Choose a reason for hiding this comment

samoteph Dec 11, 2016 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samoteph Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deltakosh Dec 9, 2016 • 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

Choose a reason for hiding this comment

deltakosh commented Dec 9, 2016

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

skendrot 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

deltakosh left a comment

Choose a reason for hiding this comment

deltakosh commented Dec 13, 2016

skendrot commented Dec 13, 2016

deltakosh commented Dec 13, 2016

samoteph commented Dec 13, 2016

deltakosh commented Dec 13, 2016

deltakosh commented Dec 14, 2016

samoteph commented Dec 15, 2016

XamlBrewer commented Dec 15, 2016 • edited Loading

robmikh commented Dec 15, 2016 • edited Loading

deltakosh commented Dec 15, 2016 • edited Loading

samoteph commented Dec 21, 2016

deltakosh commented Dec 21, 2016

deltakosh commented Dec 21, 2016

deltakosh commented Jan 3, 2017

samoteph commented Jan 3, 2017

deltakosh commented Jan 3, 2017

samoteph commented Jan 3, 2017

shenchauhan 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

deltakosh commented Jan 4, 2017

shenchauhan left a comment

Choose a reason for hiding this comment

samoteph commented Jan 12, 2017

samoteph Dec 11, 2016 •

edited

Loading

samoteph Dec 9, 2016 •

edited

Loading

deltakosh Dec 9, 2016 •

edited

Loading

XamlBrewer commented Dec 15, 2016 •

edited

Loading

robmikh commented Dec 15, 2016 •

edited

Loading

deltakosh commented Dec 15, 2016 •

edited

Loading