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

Add TouchBehavior #1673

Merged
merged 112 commits into from
Mar 27, 2024
Merged

Conversation

Axemasta
Copy link
Contributor

@Axemasta Axemasta commented Feb 1, 2024

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 Axemasta changed the base branch from touch-effect to main February 1, 2024 21:16
@Axemasta Axemasta force-pushed the rebase-touch-effect branch 2 times, most recently from 390693c to fe78e44 Compare February 2, 2024 09:18
@bijington
Copy link
Contributor

@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?

@Axemasta
Copy link
Contributor Author

Axemasta commented Feb 8, 2024

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.

@bijington
Copy link
Contributor

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

@brminnick brminnick marked this pull request as draft February 15, 2024 21:33
@brminnick brminnick changed the title Rebase touch effect Add TouchBehavior Feb 15, 2024
@brminnick
Copy link
Collaborator

brminnick commented Feb 15, 2024

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

@Axemasta
Copy link
Contributor Author

@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
Copy link
Contributor Author

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?

@pictos
Copy link
Member

pictos commented Feb 28, 2024

@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.

@bijington
Copy link
Contributor

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?

@pictos
Copy link
Member

pictos commented Feb 28, 2024

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.

@bijington
Copy link
Contributor

bijington commented Feb 28, 2024

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

@Axemasta
Copy link
Contributor Author

Axemasta commented Feb 28, 2024

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!

@brminnick
Copy link
Collaborator

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

@bijington
Copy link
Contributor

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

@bijington
Copy link
Contributor

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

@Axemasta
Copy link
Contributor Author

@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

@bijington bijington mentioned this pull request Mar 1, 2024
10 tasks
@Axemasta
Copy link
Contributor Author

Axemasta commented Mar 4, 2024

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.

@AlleSchonWeg
Copy link

@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: mrxten/XamEffects#52 (comment)
Perhaps it helps you.

@Axemasta
Copy link
Contributor Author

Axemasta commented Mar 5, 2024

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
Copy link
Contributor Author

Axemasta commented Mar 5, 2024

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 Axemasta marked this pull request as ready for review March 5, 2024 09:52
@bijington
Copy link
Contributor

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
Copy link
Contributor

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.

@brminnick brminnick dismissed pictos’s stale review March 25, 2024 22:55

Re-review requested

@brminnick brminnick mentioned this pull request Mar 25, 2024
Copy link
Collaborator

@brminnick brminnick left a comment

Choose a reason for hiding this comment

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

Update Unit Tests

brminnick
brminnick previously approved these changes Mar 27, 2024
@brminnick brminnick enabled auto-merge (squash) March 27, 2024 16:01
@brminnick brminnick disabled auto-merge March 27, 2024 23:38
@brminnick brminnick merged commit d32fb23 into CommunityToolkit:main Mar 27, 2024
8 checks passed
@MitchBomcanhao
Copy link

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.

@Axemasta Axemasta deleted the rebase-touch-effect branch March 28, 2024 09:35
@pictos
Copy link
Member

pictos commented Mar 28, 2024

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

@MitchBomcanhao
Copy link

@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)

@brminnick
Copy link
Collaborator

brminnick commented Mar 28, 2024

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).

@MitchBomcanhao
Copy link

MitchBomcanhao commented Mar 28, 2024

@brminnick I will not be able to get that done today as I'm about at the end of the working day... (and it is a long weekend in the UK). I came across this issue on two different projects (one a production app and another a brand new blank app where there's no other code modified other than updating maui, adding the toolkit and initialising it in mauiprogram.cs. perhaps there's some OS level issue where you might be running different windows versions? fyi I'm on windows 10 build 19045.4170 and VS 17.9.4 - that's pretty much all I got for you right now :/

@brminnick
Copy link
Collaborator

brminnick commented Mar 28, 2024

I confirm the nightly NuGet Package of main crashes on startup on Windows. It is a regression from this PR: #1658

I also confirm the nightly NuGet Package of main works as expected on Android.
Android

@ryan-autonomousutilities

Are there any examples of using a CommandParameter for one of these touch events (I'm trying to use the new LongTap)? I can't find one in the samples, and no matter what I try, null gets passed to the command. I just need an example to see the proper syntax.

@brminnick
Copy link
Collaborator

You'll need to manually set TouchBehavior.BindingConext.

Here's more information: https://stackoverflow.com/questions/78245724/both-touchbehavior-commandparameter-and-touchbehavior-longpresscommandparamet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending documentation This feature requires documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants