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

Implement FullScreenEffect for Android #1130

Closed
wants to merge 51 commits into from
Closed

Conversation

Cfun1
Copy link
Contributor

@Cfun1 Cfun1 commented Mar 26, 2021

Description of Change

Implement FullScreenEffect for shared and Android platform.

Bugs Fixed

PR Status

Code is still under work some parts could be removed/modified.
More importantly it needs to be made platform specific.

API Changes

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard

@Cfun1
Copy link
Contributor Author

Cfun1 commented Mar 26, 2021

It's still a draft and in a relative early stage, but if you guys have any remarks/review that needs to be pointed out now, please don't hesitate.

@maxkoshevoi
Copy link
Contributor

maxkoshevoi commented Mar 26, 2021

I don't think it should be an effect since it makes sense to be able to trigger it from code, which you cannot do with effect (except maybe setting it to None and to something else later).

Take a look at StatusBar class in #812 for example of platform specific functionality.

I'm not sure that I implemented it correctly, but it should be something like this.

CC: @pictos

@Cfun1
Copy link
Contributor Author

Cfun1 commented Apr 11, 2021

Hey @pictos I pushed an attempt to make it platform specific, a part from folders locations/namespaces I still feel like it's not the best approach especially that OnModeChanged() will be duplicate on each platform implementation, your guidance is needed an appreciated :)

@pictos
Copy link
Collaborator

pictos commented Apr 13, 2021

Hey @pictos I pushed an attempt to make it platform specific, a part from folders locations/namespaces I still feel like it's not the best approach especially that OnModeChanged() will be duplicate on each platform implementation, your guidance is needed an appreciated :)

@Cfun1 I'll take a look asap

Copy link
Collaborator

@pictos pictos left a comment

Choose a reason for hiding this comment

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

I took a look on my end, the implementation looks good to me, I would suggest moving files to another place.

Xamarin.CommunityToolkit.Effects.AndroidSpecific.FullScreenEffect should be moved to
PlatformConfiguration/AndroidSpecific/FullScreenEffect, for the namespace follow the same pattern used by iOS and UWP.

Also, for reference how to use effect and the On<Platform> API

Will this feature be supported on other platforms? IF yes ignore the next lines

Also, I saw that you used the effect API, but that isn't ideal, because we can't use it with this API
On<Android>().FullScren(), like we have here

If you need help changing the API, please let me know

@Cfun1
Copy link
Contributor Author

Cfun1 commented Apr 16, 2021

@pictos thanks for your assistance, yes this feature will be supported for the other platforms but with specific full screen modes to each platform.
Although I am still not 100% sure if we should keep the Effect API in this case or try to completely replace it with the platform specific api, I think both approaches will work but not sure which one is the most appropriate one for this case.

@pictos
Copy link
Collaborator

pictos commented Apr 16, 2021

@pictos thanks for your assistance, yes this feature will be supported for the other platforms but with specific full-screen modes to each platform.
Although I am still not 100% sure if we should keep the Effect API in this case or try to completely replace it with the platform-specific API, I think both approaches will work but not sure which one is the most appropriate one for this case.

Can we match all platform configurations into a single Enum? Using the Android and UWP values? The iOS one will be Enabled if any value different from Disabled is set

@Cfun1
Copy link
Contributor Author

Cfun1 commented Apr 17, 2021

@pictos maybe I didn't understand you correctly, but I don't see how we can match them all in one single enum. What if the dev want to set the Immersive mode for Android and PreferredLaunchViewSize for UWP?

This is how I see the picture: In order to have a consistent behavior across platforms the dev must set a full screen mode for each platform, ie if the dev set only for Android android:FullScreenEffect.Mode="ImmersiveDroid" and don't set any mode for the other platforms it's their responsibility.

Maybe the way I set the table enumerating the modes in the linked issue could be confusing, in fact they are not grouped by the same behavior ie Android.LeanBack doesn't necessary mean it has the same behavior as UWP.Maximized

@pictos
Copy link
Collaborator

pictos commented May 8, 2021

@pictos maybe I didn't understand you correctly, but I don't see how we can match them all in one single enum. What if the dev want to set the Immersive mode for Android and PreferredLaunchViewSize for UWP?

This is how I see the picture: In order to have a consistent behavior across platforms the dev must set a full screen mode for each platform, ie if the dev set only for Android android:FullScreenEffect.Mode="ImmersiveDroid" and don't set any mode for the other platforms it's their responsibility.

Maybe the way I set the table enumerating the modes in the linked issue could be confusing, in fact they are not grouped by the same behavior ie Android.LeanBack doesn't necessary mean it has the same behavior as UWP.Maximized

@Cfun1 I see now, said that I think you can move with your current implementation for enums

Copy link
Collaborator

@pictos pictos left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, sorry for taking so long to review it (:

{
base.OnElementPropertyChanged(args);

if (!args.PropertyName.Equals(ModeProperty.PropertyName) || isDetaching)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even after Detached is executed, does this method fire?

Copy link
Contributor Author

@Cfun1 Cfun1 Jun 16, 2021

Choose a reason for hiding this comment

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

Yes I hit that case that why I added that field, but it only happened when I was in the middle of debugging and set the effect to "Disabled" on a page and try navigating to it initially or after having navigated to a page having IsPersistent on. After restarting the debug session this funny behavior is replaced by the fact that OnModeChanged is not called for that page since the default value is "Disabled".

So I suggest we add another enum value named "Default" just to ensure OnModeChanged of pages with effect value set to "Disabled" get triggered, it will be useful when the app is in full screen mode either by a persistent effect on a previous page or by something else, we want that to get triggered so that the effect disable the full screen mode on that page and restore it when disappears. 45a30ac

}
}

// Still under work to handle different APIs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need more work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depends on what is the minimum Android API level that we should support ?

@Cfun1 Cfun1 requested a review from pictos June 16, 2021 12:54
@Cfun1
Copy link
Contributor Author

Cfun1 commented Jul 2, 2021

@pictos any update on this?

@pictos
Copy link
Collaborator

pictos commented Jul 3, 2021

@Cfun1 sorry didn't have time to ship it in 1.2, but this will in 1.3 for sure :D , as soon we have a new version of XF I'll work on this

@pictos
Copy link
Collaborator

pictos commented Jul 17, 2021

@Cfun1 I took a look into this one and I believe that makes sense to implement this feature per platform using the platform-specific API. Do you agree?

@brminnick
Copy link
Contributor

brminnick commented Nov 11, 2021

sorry didn't have time to ship it in 1.2, but this will in 1.3 for sure :D , as soon we have a new version of XF I'll work on this

@pictos It looks like we missed this one in the v1.3 Release.

Do you still want to merge it, or should we defer it to .NET MAUI Community Toolkit?

@brminnick
Copy link
Contributor

Thanks! However, we are no longer adding new features to Xamarin Community Toolkit, focusing on the .NET MAUI Community Toolkit.

Please open a New Feature Discussion to implement this feature in the .NET MAUI Community Toolkit.

I've posted more information about the Future Of Xamarin Community Toolkit here: https://devblogs.microsoft.com/xamarin/the-future-of-xamarin-community-toolkit/?WT.mc_id=mobile-0000-bramin

@brminnick brminnick closed this Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants