Maui icon indicating copy to clipboard operation
Maui copied to clipboard

Add TouchBehavior

Open Axemasta opened this issue 1 year ago • 24 comments
trafficstars

This is a rebase of the touch-effect branch, back ontop of main.

The rebase wasn't pretty and there were lots of conflicts, I sided with main in every conflicted file but there were still some issues that had to be resolved by hand. Where any file was broken by the rebase, I have copied the code from main in to replace the contents.

Axemasta avatar Feb 01 '24 21:02 Axemasta

@Axemasta thanks for updating this PR. What action do you need now? Do you need a maintainer to review the current changes? Looking on my phone I see changes to popup related bits which might not be intended?

bijington avatar Feb 06 '24 21:02 bijington

The outstanding work as I am aware is:

  • Fix input transparency on iOS (Code is complete and I have a branch with the fix on my fork)
  • Fix unit tests (a couple are failing)
  • Update sample app & make the touch behavior sample inline with other MCT sample pages

The next steps are testing and ensuring it works and is ready to merge!

I'm not sure what to do about this branch since its a rebase branch, do we abandon the old touch effect branch and merge this one (when ready)? If we decide to use the old branch, I can't rebase since i dont have push permissions and i'm unsure of how I can rebase it without doing a force push.

Axemasta avatar Feb 08 '24 20:02 Axemasta

What's the least amount of effort for you? I can rebase and force push if you need?

bijington avatar Feb 08 '24 21:02 bijington

You don't need to fret about rebasing, @Axemasta, we have branch protection rules in place that will take care of that automatically for you.

To keep this PR scoped to the new TouchBevahior control, please remove the all of edits to files that are unrelated to the new TouchBehavior:

  • [ ] samples/CommunityToolkit.Maui.Sample/App.xaml.cs
  • [ ] src/CommunityToolkit.Maui.Core/Views/Alert/AlertView.macios.cs
  • [ ] src/CommunityToolkit.Maui.Core/Views/DrawingView/PlatformView/MauiDrawingView.android.cs
  • [ ] src/CommunityToolkit.Maui.Core/Views/DrawingView/PlatformView/MauiDrawingView.windows.cs
  • [ ] src/CommunityToolkit.Maui.Core/Views/DrawingView/PlatformView/MauiDrawingView.windows.cs
  • [ ] src/CommunityToolkit.Maui.MediaElement/CommunityToolkit.Maui.MediaElement.csproj
  • [ ] src/CommunityToolkit.Maui/Behaviors/PlatformBehaviors/Touch/SafeFireAndForgotExtensions.cs
  • [ ] src/CommunityToolkit.Maui/CommunityToolkit.Maui.csproj

TheCodeTraveler avatar Feb 15 '24 21:02 TheCodeTraveler

@brminnick I have addressed all but two of these files in my latest commit, the files not address are:

  • samples/CommunityToolkit.Maui.Sample/App.xaml.cs, this will be reverted when i add the demo page to the sample
  • src/CommunityToolkit.Maui/Behaviors/PlatformBehaviors/Touch/SafeFireAndForgotExtensions.cs, this is part of the touch behavior feature

Axemasta avatar Feb 27 '24 17:02 Axemasta

I mentioned this in the discord yesterday, after updating the sample app to use a viewmodel I discovered the TouchBehavior will not update its binding context & the commands (or any bindings) will not work. This is due to the behavior being a PlatformBehavior which does not set a binding context.

I discovered this from the following sources:

For the time being I have opted to set the binding context of the platform behavior so that the sample functions in the intended way. My question here is will this cause issues for consumers of the behavior? Is there any special handling we will need to do to ensure there aren't issues or would we be better reverting to the old api design, which uses attached properties to achieve the behavior?

@pictos what do you think about this issue?

Axemasta avatar Feb 28 '24 12:02 Axemasta

@Axemasta Yeah, as you can see on #795 it's this way by design (as I mentioned in the discussion).

For the time being I have opted to set the binding context of the platform behavior so that the sample functions in the intended way

The user can set the BindingContext itself, and use the normal Binding expression. And I believe that we should align our implementation with the general recommendation, that's to set BindingContext on behaviors.

pictos avatar Feb 28 '24 14:02 pictos

And I believe that we should align our implementation with the general recommendation, that's to set BindingContext on behaviors.

Does this mean the toolkit should set the BindingContext in the behaviors? Or we apply it to our samples?

bijington avatar Feb 28 '24 14:02 bijington

And I believe that we should align our implementation with the general recommendation, that's to set BindingContext on behaviors.

Does this mean the toolkit should set the BindingContext in the behaviors? Or we apply it to our samples?

The toolkit shouldn't apply the BindingContext to its behaviors. The reason Behaviors don't set the BindingContext is because they are meant to be reusable (for example you set a behavior on a style). With that, the user should decide if he wants to set the BindingContext or use the RelativeSource to set the bindings.

pictos avatar Feb 28 '24 15:02 pictos

Thanks for the detail. Adding a behavior to a Style when you have properties like Commands that will need to be bound or assigned sounds like a dangerous thing to be able to do.

I'm not criticizing anything here, just stating it feels a bit dangerous to allow it

bijington avatar Feb 28 '24 17:02 bijington

I'm not criticizing anything here, just stating it feels a bit dangerous to allow it

I agree, previously you couldn't do this because it was using an attached effect. The docs are going to have to be quite clear here that here is a specific way you setup binding & you definitely shouldn't apply to a style.

People can still abuse it at their own risk, but the documented way should work fine!

Axemasta avatar Feb 28 '24 17:02 Axemasta

The docs are going to have to be quite clear

The .NET Maui docs on Behaviors are very explicit that behaviors do not inherit the BindingContext of their parent: https://learn.microsoft.com/en-us/dotnet/maui/fundamentals/behaviors?view=net-maui-8.0#create-a-net-maui-behavior

image

TheCodeTraveler avatar Feb 28 '24 17:02 TheCodeTraveler

Oh good spot! We should link to that on all behavior pages

bijington avatar Feb 28 '24 17:02 bijington

@Axemasta do you think it's fairly safe for me to get started on the docs PR for this?

bijington avatar Feb 28 '24 17:02 bijington

@Axemasta do you think it's fairly safe for me to get started on the docs PR for this?

Yes I think so, the hover feature requires implementing and I'm starting to look at that. Api wise everything is there and aside from hover it behaves how it did in forms

Axemasta avatar Feb 28 '24 17:02 Axemasta

Current status of the pr, I have fixed hover which is now working on both platforms. Tests immediately all went green as a bonus (minus 1 test that had the wrong default plugged into it).

All that remains dev wise is to fix the issue i'm seeing on android devices where a child view becomes invisible if the parent has the effect applied. I've narrowed it down to UseNativeAnimation being the culprit, when we don't apply a ripple view the components render fine and are visible, its only when applying the ripple that the child view gets made invisible. I need to debug further to find the cause.

Axemasta avatar Mar 04 '24 21:03 Axemasta

@Axemasta thanks for bringing this to the toolkit. The old XF Toucheffect (NativeAnimation) never works for me when the effect was attached on a layout in a list/cv or scrollview. The effect was executed even the user just want to scroll the items. On iOS i used XamEffects: https://github.com/mrxten/XamEffects/. But this not works in android. I created my own version for android: https://github.com/mrxten/XamEffects/issues/52#issuecomment-895888358 Perhaps it helps you.

AlleSchonWeg avatar Mar 05 '24 08:03 AlleSchonWeg

Perhaps it helps you.

Thanks its good to know it wasn't working properly in the XCT. I checked my port and the iOS native animations work but the android ones don't show so I've clearly ported the bug 😂. Currently the ripple animation is actually playing on Android, the issue is that the ripple view swallows any child views therefore causing the ui not to render correctly.

I've actually found the root cause and I'm working on shipping a fix to position the ripple view behind any child views so the order of views it correct.

The XamEffect library looks really cool, I've used the syncfusion ripple in the past and love how slick it makes your controls look. Its a small addition that makes a big difference to the look & feel of the app 😌

Axemasta avatar Mar 05 '24 09:03 Axemasta

Ok i've been able to fix the issue i was seeing on android with nested view, I've simply removed the code that brings the ripple view to the front, it was causing the problems.

I can verify expected appearances work: nested android fix

And more complicated nested layouts also work properly: mullti nest demonstration

This also works for a single view that has no nested views :)

I believe this PR is feature complete now and is ready for a proper review!

Axemasta avatar Mar 05 '24 09:03 Axemasta

I hate to suggest big changes but is it a silly idea to suggest a rename to InteractionBehavior given this does a lot more than just touch based interactions?

bijington avatar Mar 05 '24 20:03 bijington

Wow! There is a LOT to this TouchBehavior API! I know this came from XCT and @Axemasta you have put in a lot of effort to bring it to the toolkit! I am just trying to comprehend how I can get some docs written. I think I will take the samples page and build on from there.

These question(s) are purely to open a conversation:

  • do we know why we have properties like NormalBackgroundImageSource that only apply when the behavior is attached to an Image? I've been trying to think on how it might be possible (and failing) to only expose these properties when the behavior is attached to an Image element.

bijington avatar Mar 05 '24 20:03 bijington

I hate to suggest big changes but is it a silly idea to suggest a rename to InteractionBehavior given this does a lot more than just touch based interactions?

I personally think the name TouchBehavior is suitable since all the features are concerned with responding to different touches on the given view 😄

  • do we know why we have properties like NormalBackgroundImageSource that only apply when the behavior is attached to an Image? I've been trying to think on how it might be possible (and failing) to only expose these properties when the behavior is attached to an Image element.

AFAIK this is from the old design when you could change the image source based on touch, it needed to know what the "normal" was. We could see how much of the Normal api can be removed now & if everything still functions the same, I know in my port I had issues with views not returning to normal unless that was explicitly set.

Axemasta avatar Mar 06 '24 18:03 Axemasta

I personally think the name TouchBehavior is suitable since all the features are concerned with responding to different touches on the given view 😄

It was the Hover properties that made me question the name 🙂

AFAIK this is from the old design when you could change the image source based on touch, it needed to know what the "normal" was. We could see how much of the Normal api can be removed now & if everything still functions the same, I know in my port I had issues with views not returning to normal unless that was explicitly set.

Yeah I get that. I'd love to only expose the properties when they can have an effect. We might be limited by what we can achieve there

bijington avatar Mar 06 '24 19:03 bijington

@Axemasta looks like the unit test are breaking the build, can you take a look on those?

pictos avatar Mar 09 '24 20:03 pictos

Looks like there is a build error for the sample app because now we have a duplicate Splash.svg file. Was that added intentionally? Should we remove the one that is added in this PR?

jfversluis avatar Mar 15 '24 19:03 jfversluis

The docs PR is at: https://github.com/MicrosoftDocs/CommunityToolkit/pull/392

bijington avatar Mar 25 '24 21:03 bijington

fyi I've just tried the 99.0.0-preview1678 nightly build (the first where TouchBehavior seems to appear in), and it seems to make my windows app crash on launch with an unspecified error and on android nothing really happens when adding a touchbehaviour to a control.

MitchBomcanhao avatar Mar 28 '24 09:03 MitchBomcanhao

@MitchBomcanhao please open an issue with a sample project that reproduces the error

pictos avatar Mar 28 '24 13:03 pictos

@pictos to get the windows app failing on launch, Just get a blank new app, add that nightly build to the project, and launch the app. it'll fail with the error shown below. It'll take me longer to figure out how to make a sample project added to github (which is a requirement to open an issue) than it will for you to try this.

Unspecified error

   at WinRT.ExceptionHelpers.<ThrowExceptionForHR>g__Throw|39_0(Int32 hr)
   at WinRT.ExceptionHelpers.ThrowExceptionForHR(Int32 hr)
   at ABI.Microsoft.Windows.AppNotifications.IAppNotificationManagerMethods.Register(IObjectReference _obj)
   at Microsoft.Windows.AppNotifications.AppNotificationManager.Register()
   at CommunityToolkit.Maui.AppBuilderExtensions.<>c.<UseMauiCommunityToolkit>b__0_3(Application _, LaunchActivatedEventArgs _)
   at Microsoft.Maui.MauiWinUIApplication.<>c__DisplayClass1_0.<OnLaunched>b__3(OnLaunched del)
   at Microsoft.Maui.LifecycleEvents.LifecycleEventServiceExtensions.InvokeLifecycleEvents[TDelegate](IServiceProvider services, Action`1 action)
   at Microsoft.Maui.MauiWinUIApplication.OnLaunched(LaunchActivatedEventArgs args)
   at Microsoft.UI.Xaml.Application.Microsoft.UI.Xaml.IApplicationOverrides.OnLaunched(LaunchActivatedEventArgs args)
   at ABI.Microsoft.UI.Xaml.IApplicationOverrides.Do_Abi_OnLaunched_0(IntPtr thisPtr, IntPtr args)

MitchBomcanhao avatar Mar 28 '24 14:03 MitchBomcanhao

I confirmed that TouchBehavior works as expected in CommunityToolkit.Maui.Sample on both Android + Windows

Android Windows
Screenshot_1711644268

@MitchBomcanhao Could you open an Issue and provide a reproduction sample today? We are planning to release TouchBehavior in CommunityToolkit.Maui v8.0.0 tomorrow (Friday 28 March).

TheCodeTraveler avatar Mar 28 '24 16:03 TheCodeTraveler