Skip to content
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

[Experiment] Initial CompositionCollectionView implementation and basic samples #277

Open
wants to merge 54 commits into
base: main
Choose a base branch
from

Conversation

arcadiogarcia
Copy link
Member

@arcadiogarcia arcadiogarcia commented Sep 19, 2022

This PR contains the initial implementation of the CompositionCollectionView control proposed in #135. More specifically, it implements:

  • CompositionCollectionView XAML control
  • Layout and LayoutBehavior as the extensible base class for layouts and their behaviors
  • InteractionTrackerBehavior, the first (of hopefully many) general purpose behavior, to easily plug InteractionTrackers into layouts
    • This also includes InteractionTrackerElementBehavior for per-element interaction trackers, and InteractionTrackerGesture for defining reusable InteractionTrackerGestures
    • A few basic samples
CommunityToolkit.Labs_.CompositionCollectionView.Sample.2022-09-18.22-29-20.mp4

I'd like to check this in as an initial version of the control so it can be pulled into apps easily and tried by other people. While the public interfaces might undergo changes and I plan to add more behaviors and samples, functionality wise the control is feature complete. I still need to add tests and document each method too.

Other caveats:

  • The current implementation only supports UWP. The long term plan is to eventually also support Reunion but I haven't gotten around to testing that. Support for other platforms that do not support Composition are out of the scope of this control though.
  • This control relies heavily on the ExpressionNodes from the Community Toolkit, to which I had to add a couple extra features. Since that part of the library is not accepting new features right now, this experiment also contains a fork of that part of the library with my changes, the long term plan would be to eventually merge this fork. (See Add Evaluate() method to expression nodes WindowsCommunityToolkit#4559)

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Looks like there's a back-up csproj file committed as well.

Haven't had a chance to try it out, but wanted to provide a few high-level comments first. Thanks for the PR!

@Arlodotexe do we have an easy way to exclude something entirely from Uno at the moment?

}

#if !WINAPPSDK
public abstract partial class Layout<TId, TItem> : ILayout, IDisposable
Copy link
Member

Choose a reason for hiding this comment

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

Layout conflicts with the base ItemsRepeater Layout class and is too generic here (which is why you probably ran into immediate issues in WindowsAppSDK land). This should be more like CompositionCollectionLayout or something. Maybe we need a more concise name for the layout itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, also I'm flexible about naming in general, if someone suggests something that rolls off the tongue better than CompositionCollectionView I'm happy to rename it

labs/CompositionCollectionView/src/Layout.cs Outdated Show resolved Hide resolved
@Arlodotexe
Copy link
Member

@Arlodotexe do we have an easy way to exclude something entirely from Uno at the moment?

We don't (yet) have a way to disable a component under a specific platform.

The expectation so far is that Labs experiments may not be production ready, but once it is, components should work on as many platforms as reasonably possible.

So far, we haven't come across any experiment that will never run under a specific platform, but we can (and should) in the future if it becomes a hard blocker.

@arcadiogarcia
Copy link
Member Author

arcadiogarcia commented Sep 28, 2022

So far, we haven't come across any experiment that will never run under a specific platform, but we can (and should) in the future if it becomes a hard blocker.

I'm afraid this might be the first experiment that falls in that bucket, because it depends heavily on the composition animation API and as far as I can tell Uno doesn't have any plans to add support for those anytime soon. (They mentioned it as a required feature in the Animation for Android epic (unoplatform/uno#5586, which hasn't seen much activity lately and isn't added to any milestone, and it isn't even mentioned at all in the Animation for WASM epic unoplatform/uno#3919)

@michael-hawker
Copy link
Member

We definitely need to figure out some story around documenting and calling out supported platforms within Labs, even if we expect stuff to get Uno support later, it may not start out that way. @Arlodotexe something for us to start discussing as we plan for 8.0.

@arcadiogarcia want to run XAML Styler with the settings in the repository (there's also a script in the repo root) so that the rest of the build will run?

@Arlodotexe
Copy link
Member

@arcadiogarcia Do you know why the source project has a shared project in it? This isn't part of our template 🤔

image

@michael-hawker
Copy link
Member

Note, this is also another project which only works on UWP/WinAppSDK, so we'll need whatever fix we do for the #353 to not generate the wasm single project head here as well.

@arcadiogarcia
Copy link
Member Author

@arcadiogarcia Do you know why the source project has a shared project in it? This isn't part of our template 🤔

image

I don't remember tbh. Probably I was trying something and forgot to remove it, as far as I can tell is not important, I deleted it.

@michael-hawker
Copy link
Member

@arcadiogarcia sorry things keep changing underneath you, there's a lot of moving parts still and we're trying to push forward to be able to split our infrastructure out to reuse. Hopefully things will settle down.

Saw there's changes to the global usings file, so there's a conflict now. Most of that seems fine, though we have an open item to refactor that process CommunityToolkit/Tooling-Windows-Submodule#15.

Your changes seem fine, though I am a bit weary of including Windows.UI globally for both UWP and WASDK all-up. Maybe you could have that one just in the files you need it for this PR?

@arcadiogarcia
Copy link
Member Author

@arcadiogarcia sorry things keep changing underneath you, there's a lot of moving parts still and we're trying to push forward to be able to split our infrastructure out to reuse. Hopefully things will settle down.

Saw there's changes to the global usings file, so there's a conflict now. Most of that seems fine, though we have an open item to refactor that process CommunityToolkit/Tooling-Windows-Submodule#15.

Your changes seem fine, though I am a bit weary of including Windows.UI globally for both UWP and WASDK all-up. Maybe you could have that one just in the files you need it for this PR?

Sure no worries! I removed the global Windows.Ui using and ran the xaml styler on my files, let me know if there's anything else I should fix. I see a few other failures on the checks but as far as I can tell they seem unrelated to this PR.

@arcadiogarcia
Copy link
Member Author

Btw should I expect to see the nuget package in the PR feed already? (https://pkgs.dev.azure.com/dotnet/CommunityToolkit/_packaging/CommunityToolkit-PullRequests/nuget/v3/index.json) I see that the new-experiment build succeeds but I don't see the nuget in the feed, do I have to fix something else for it to get published there?

@Arlodotexe
Copy link
Member

I see a few other failures on the checks but as far as I can tell they seem unrelated to this PR.

That's odd. This isn't present on main, and the branch is already up to date with main. Maybe a merge issue? I'll fix it manually for now.

@michael-hawker
Copy link
Member

Looks like the error messages on the build are all this:

Error: /home/runner/work/Labs-Windows/Labs-Windows/components/MarqueeText/src/MarqueeText.Properties.cs(53,12): error CS0246: The type or namespace name 'RepeatBehavior' could not be found (are you missing a using directive or an assembly reference?) [/home/runner/work/Labs-Windows/Labs-Windows/components/MarqueeText/src/CommunityToolkit.Labs.WinUI.MarqueeText.csproj]
Error: /home/runner/work/Labs-Windows/Labs-Windows/components/MarqueeText/src/MarqueeText.cs(54,13): error CS0246: The type or namespace name 'Storyboard' could not be found (are you missing a using directive or an assembly reference?) [/home/runner/work/Labs-Windows/Labs-Windows/components/MarqueeText/src/CommunityToolkit.Labs.WinUI.MarqueeText.csproj]

Which is weird as that's a different experiment...

@@ -0,0 +1,20 @@
<!--
Copy link
Member

Choose a reason for hiding this comment

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

@Arlodotexe these are remnants of the old system...

Also we're missing the multi-target props, so think that's why we're seeing the Uno warnings/errors, eh?

  D:\a\Labs-Windows\Labs-Windows\components\CompositionCollectionView\src\Behaviors\InteractionTrackerGesture\GesturePreviewControl.cs(8,23): error CS0260: Missing partial modifier on declaration of type 'GesturePreviewControl'; another partial declaration of this type exists [D:\a\Labs-Windows\Labs-Windows\components\CompositionCollectionView\src\CommunityToolkit.Labs.WinUI.CompositionCollectionView.csproj]
  D:\a\Labs-Windows\Labs-Windows\components\CompositionCollectionView\src\CompositionCollectionView.cs(11,21): error CS0260: Missing partial modifier on declaration of type 'CompositionCollectionView'; another partial declaration of this type exists [D:\a\Labs-Windows\Labs-Windows\components\CompositionCollectionView\src\CommunityToolkit.Labs.WinUI.CompositionCollectionView.csproj]

#endif

global using MUXC = Microsoft.UI.Xaml.Controls;
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Member

Choose a reason for hiding this comment

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

Changes here are going to be a problem/conflict with the work we're doing for #405 and #398. Should just tie-in to clean up needed for #321?

@@ -9,7 +9,6 @@
<Copyright>(c) .NET Foundation and Contributors. All rights reserved.</Copyright>
<PackageProjectUrl>https://github.com/CommunityToolkit/Labs-Windows</PackageProjectUrl>
<PackageReleaseNotes>https://github.com/CommunityToolkit/Labs-Windows/releases</PackageReleaseNotes>
<PackageIcon>Icon.png</PackageIcon>
Copy link
Member

Choose a reason for hiding this comment

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

There's some other non-needed changes here and in the common\test projects

@@ -0,0 +1,137 @@
#nullable disable
Copy link
Member

Choose a reason for hiding this comment

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

License should still be top thing, does this tie into CS8602? Ideally we should just be good had writing nullable safe code, but probably easier to just do at project level in csproj and track via a todo later.

Copy link
Member

Choose a reason for hiding this comment

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

Nullability is worth doing a pass on before merging in here. This PR will already be nontrivial to bring up to date with our latest tooling.

using System.Numerics;
using Windows.UI;

namespace Microsoft.Toolkit.Uwp.UI.Animations.ExpressionsFork
Copy link
Member

Choose a reason for hiding this comment

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

Do these things already exist in the toolkit? Can we not just reference them from the existing package?

Copy link
Member Author

Choose a reason for hiding this comment

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

This experiment needs the changes from my branch at CommunityToolkit/WindowsCommunityToolkit#4559, which we never merged to master, that's why I'm including my own fork that is evolving in parallel. I agree that adding a copy of those classes is not ideal, but there doesn't seem to be any good way to add functionality to those core toolkit classes from an experiment (I can't add the Evaluate() method as an extension method because it relies on private properties of the ExpressionNodes).

@@ -0,0 +1,17 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Member

Choose a reason for hiding this comment

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

We should just use PolySharp reference for these. Were you using records somewhere?

Copy link
Member

Choose a reason for hiding this comment

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

PolySharp is included by default on the Uwp head. For component libraries, they need to be included on the project itself, see examples here.

#endif
}

#if !WINAPPSDK
Copy link
Member

Choose a reason for hiding this comment

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

Bit confused by these conditionals in the samples, there's no comment, why is this excluded on the WindowsAppSDK?

@@ -0,0 +1,97 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Member

Choose a reason for hiding this comment

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

This file should be CanvasSample.xaml.cs no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants