AppCompatActivity and shared element #5789
Conversation
- switch to AppCompatActivity - added “transitionName” property - added prepareFragment optional function for navigate - allow access to _fragment for page
ghost
added
the
|
Please sign CLA at http://www.nativescript.org/cla |
|
CLA signature found, happy contributing! |
|
@farfromrefug Thank you for taking the time to make this PR! We will need some time run our automated tests and review this as the proposed changes are related to some core logic inside the NativeScript framework for Android. |
| public context: android.content.Context; | ||
| public foregroundActivity: android.app.Activity; | ||
| public startActivity: android.app.Activity; | ||
| public foregroundActivity: android.support.v7.app.AppCompatActivity; |
There was a problem hiding this comment.
Could you elaborate a bit why is the change to android.support.v7.app.AppCompat* APIs necessary for supporting shared element transitions?
There was a problem hiding this comment.
Actually i made a all replace. I think you ate right in this case it should still be an activity as you can start another activity which would not be an AppCompatActivity
There was a problem hiding this comment.
I am still trying to get my head around why we need the AppCompat* API switch at all. Can you elaborate?
There was a problem hiding this comment.
@manoldonev You need to compat switch to get access to many methods, like startPostponedEnterTransition on fragments and activities.
You also need supportFinishAfterTransition to make sure transitions are applied when closing an activity.
Also i really want to point out that in P, they deprecate the default FragmentManager so you need to start using the support library anyway.
On a side note it wont make apps bigger as your already include it.
| } | ||
|
|
||
| get transitionName(): string { | ||
| return this.style.transitionName; |
There was a problem hiding this comment.
I don't think transitionName should be registered on Style but on ViewCommon. Check automationText:
https://github.com/NativeScript/NativeScript/blob/master/tns-core-modules/ui/core/view/view-common.ts#L1000
There was a problem hiding this comment.
Great i am all new to nativescript. Will change that.
| _updateTransitions(backstackEntry); | ||
| } | ||
| const manager: android.support.v4.app.FragmentManager = this._getFragmentManager(); | ||
| manager.popBackStack(); |
There was a problem hiding this comment.
Currently we are not using the backstack functionality of FragmentManager but we are managing the fragments on our own. Could you elaborate a bit why are you proposing this change?
There was a problem hiding this comment.
This one is quite important because otherwise shared element animation wont work. Or you need to do it manually (re add the shared element but the other way around). I think you did not use it to get listeners and be notified when transition finished. Which still works in this PR
There was a problem hiding this comment.
I guess we should explore doing this manually as I don't see us replacing our back stack management logic at this point (or adding the FragmentManager's one on top of it).
There was a problem hiding this comment.
@manoldonev i don't see how it breaks your back stack management? You still get the transition end event, and your stack is updated.
|
@farfromrefug btw as far as I can see even with support libraries shared element transitions are supposed to work on Android API Levels 21 or higher, right? Also, this is obviously Android only and ideally we would like to provide cross-platform support for iOS and Android. I did a quick search and there are iOS options (here and here) or cross-platform solutions like this one that we might need to consider as alternatives as well. |
|
@manoldonev not in the support library. Was added in support 22.0.1 i think Edit: the react native version is wrong in what they are saying because they dont seem to be using a navigationcontroller. Their defaut transitiom animation is wrong |
|
test |
|
@farfromrefug btw we are actually working on a sample custom animation showcase that mimics shared element transitions and works on iOS and Android: https://play.nativescript.org/?template=play-ng&id=iDY4kF&v=10 (work in progress). Would this approach work for your scenario as well? |
|
@farfromrefug we tried to run some of our test suites to estimate the potential impact of the proposed changes and whether merging this is feasible but currently the unit test and e2e apps are crashing: You can run the unit test app locally like this (it is located inside the |
|
@manoldonev will look at the test error tomorrow morning! About your example it might work. Right now i see it does not handle different size from one view to the other? Also you don't animate the fragment transition itself right? Would it work with explode or fade transition? It would need to. Thanks |
|
@manoldonev i fixed the unit test app. Will be more careful with this from now on. |
|
@manoldonev found a great library to do that on iOS. It's actually based on android transitions. |
|
test |
|
@farfromrefug about the iOS library -- we do not have a precedent for this and at this point we would prefer not to reference native external lib within NativeScript core. For one thing we would need to get much better understanding of this lib and its potential implications / effects, also adding it and not using it for anything else besides shared element transitions does not seem like the best strategy. As for the Android part -- we had a discussion internally and we agreed that we would need to address Android P support first with highest priority due to its deprecations and breaking changes before even considering how to proceed with this PR (or alternative solution that adds support for cross-platform shared element transitions) as the proposed changes tamper with some core Android internals in NativeScript core (as discussed above one of the things we are concerned about is the backstack management changes and would not like to revert to a situation that #4951 addressed several months ago). Btw, do you think it is feasible to extract the changes you are proposing in a NativeScript plugin? If we can agree on an approach and API to expose maybe that would be the best option for the time being (note that exposing the necessary API might not be that easy as currently this platform-specific implementation is intentionally hidden but at least we can try to discuss it). |
|
@manoldonev i do understand. I don't think it can be made as a plugin because it requires to change the "extends" of the basic Nativescript Activity. At least i think it's not possible. Now in my opinion and seeing the very limited amount of changes in the code, it could be part of the Android P support. Especially when you need to change the transition frame code because of the Android P security. If i am correct the way to get the default transition won't work anymore (same for my code). But whatever you decide, i will understand. Thanks |
This comment was marked as abuse.
This comment was marked as abuse.
|
I agree with @NathanaelA, also consider this a major feat that nativescript needs!! And there is androidX now, that brings among other things, support for the material-components-android(also available under the support.design lib), been using some of these components but it take to much to make it work. This with android P would be great! |
|
@NathanaelA this PR is currently on-hold as we felt it required significant effort on our side to complete at the time. As for AppCompatActivity / deprecated native fragments -- we will revisit those building blocks separately from the shared element transition support in the coming weeks and we'll agree upon a course of action. Will update this thread when we have more info. @danielgek are you referring to AppCompatActivity or the shared element transition support that this PR originally targets? |
This comment was marked as abuse.
This comment was marked as abuse.
|
@manoldonev What's the effort you are talking about? is that testing? Cause on the implementation side it seems like there is nothing else to do |
|
@farfromrefug testing is part of the effort too, yes (tests did not pass with this PR at any point in time) but as discussed above the proposed changes tamper with some core Android internals in NativeScript core and our main concern is the backstack management changes as we would not like to revert to a situation that #4951 addressed several months ago. To summarize -- this feature is on our radar but we will handle breaking changes and deprecations like AppCompatActivity and native fragments first and then we will go back to the task of adding shared element transitions. |
|
@manoldonev was talking about |
|
Until we get the native shared elements animation support added to {N}, |
|
@shiv19 from a design point of view that's not a nice solution, the fact that you see the rest of the list disappearing has made my designer completely not adopting it. The problem is clients keep asking for these kind of transactions because all the major apps are doing it and at the moment this is a big limitation of NS in my opinion, especially if we want apps that looks nice and native. |
|
@demetrio812 Another important support that is required is this: Not sure if it is on their radar. |
|
Hello @shiv19 , thanks for pointing out the other issue, yes I think we really need to be able to create nice looking apps that follow the design guidelines from each OS to be really confident that ANY app can be built with NS, I'm really looking forward to it! |
|
Migration to support library APIs PR #6129 is now merged in the master branch of the NativeScript repo. Can be tested via tns-core-modules@next. |
|
@manoldonev @radeva @shiv19 now that version 6 is out, can you have another look at the shared elements? In general it would be cool to have a general solution that would work on iOS too as it's really needed to step up the UI capabilities of the apps written in Nativescript, especially after how much Flutter is focusing its efforts in UI quality and animations Thanks a lot |
|
I'd like to add that in case you don't want to include them inside the NS Core, the new material components could be an opportunity to address it in a multiplatform way but keeping them in an external library |
8 participants
Follows issue: #5785
manager.popBackStack();I should mention that switching to AppCompat actually changed to default fragment enter animation. With support it is a "push fade" animation (quite fast one). Without it was a fade animation.
For now i have let some commented code so that you can review it.
I am also not very good with tests. Let me know what tests you want me to add.