[VIO-103] feat: Implement new FlagshipUI API #1082
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ldoppea
force-pushed
the
feat/rework_set_flagship_ui
branch
2 times, most recently
from
ca14804 to
ecd9dcf
Compare
Ldoppea
changed the title
Ldoppea
force-pushed
the
feat/rework_set_flagship_ui
branch
11 times, most recently
from
81fd6c5 to
6b73311
Compare
Ldoppea
changed the title
Ldoppea
changed the title
acezard
reviewed
Jan 3, 2024
| * This array should be used everytime we call useFlagshipUI to apply a ZIndex to the component | ||
| * All ZIndexes are grouped here to ensure no screen overlap | ||
| */ | ||
| export const ScreenIndexes = { |
There was a problem hiding this comment.
For me all this is declarative config and shouldn't really be in the same place
There was a problem hiding this comment.
Do you mean this should be in another existing file? Or do you want it in a new file?
There was a problem hiding this comment.
yes a new file as I don't see an existing one filling that role. but maybe it can be handled later as it's non critical
acezard
approved these changes
Jan 3, 2024
The FlagshipUI is responsible to apply the correct theme to status and navigation bars base on the displayed screen or cozy-app Previous `setFlagshipUI` API had numerous caveats: - It was difficult to keep track of which component was rendered on top - It was difficult to handle components unmounting as we had to know which theme to apply by guessing which view would be shown after unmounting the components - Previous point implied the creation of many hacks in order to have enough context to make a choice (i.e. hideSplashscreen, leaving LockScreen, handle cozy-home theme, keep track of home's konnector state, leaving OsReceiveScreen etc) This new implementation is an attempt to simplify the API and to reduce the need of hacks and side effects It is based on new out-of-react service (`FlagshipUIService`) that keeps track of all components that use the `useFlagshipUI()` hook When a component registers, unregister or change its color, then the `FlagshipUIService` updates its reference array and recompute the new FlagshipUI to be applied To ease the rendering order calculation, we hardcoded array of ZIndex that every component should use when calling the `useFlagshipUI()` hook This can be done because the application UI is simple enough so we know which screen should be on top of others This seems to be the only reliable way to predict screens order. We tried to guess components positions based on React tree using different hooks (i.e. useMemo, useEffect, useLayoutEffect, useFocus), but none of them would allows us to keep track of screens order We chose to put this mechanism out of React so changing the status and navigation bars would not produce any re-rendering
The comment describes that iOS does not have any navigation bar (BottomBar), which is true But the code did try to apply a `bottomTheme` and would ignore any `topTheme` set without a `bottomTheme` which is the reverse of what we want
This is the first implementation of the new `seFlagshipUI()` hook We need to call `useFlagshipUI()` with 3 parameters: - The component's name -> used only for debuging purpose - The component's ZIndex -> should be based on a ZIndex from `ScreenIndexes` object (this is done this way to ease keep tracking or all screens relating positions) - The default FlagshipUI -> applied when the component is mount. This can be changed later in the component lifecycle using `setFlagshipColors` (not used here) We do this for both the base view (ClouderyOffer) and the overlay that is displayed on top of the WebView until it is fully loaded Then we need to ensure that any component using this hook is not mounted when not displayed because `useFlagshipUI()` is heavily based on the components lifecycles This is why we extracted the LoadingOverlay in its own conditionally mounted component, so it's hook won't be called before needed Also note that the initial values for FlagshipUI are declared outside of the component in order to be stable and not trigger the hook on every rerender
The HomeView component does not directly call the FlagshipUI API. This is done by the cozy-app embedded in the CozyWebView We need those calls to include the HomeView's `componentId` so the `FlagshipUiService`` can keep track of them The embedded cozy-app uses `cozy-intent` to call `localMethods` and we don't have direct control on the calls parameters Also we don't want to update the cozy-apps as this would imply to handle 2 API versions (as old cozy-apps can still exist on the user's device) Instead we intercept those calls in the `cozy-intent` native part and we inject the `componentId` as a parameter of `tryEmit()` Related PR: cozy/cozy-libs#2360
With the new FlagshipUI API, the HomeScreen doesn't need to keep track of the home's UI anymore The FlagshipUIService already keeps track of the application state and receive UI update from the embedded cozy-home
The HomeView keeps track of the cozy-home theme This is done by calling `setTheme` from the cozy-home webapp This code seems to exist only to support the old FlagshipUI API, but it's hard to see the implications of removing everything So instead we want to plug the new API on the `setHomeTheme` hook Then in the future we should investigate if all this file can be simplified or removed Related PR: cozy/cozy-libs#2360
This is the same principle as we did for the HomeView The difference here is that we have the loading animation that should be migrated too With the new API, the Animation view is responsible to handle its own FlagshipUI, so we don't need to keep track of its state inside of the CozyAppScreen This part will be removed in the following commit
CozyAppScreen needs to listen for FlagshipUI changes in order to know which color to display on top and bottom bars (as the cozy-apps are not displayed in an immersive way) We want to use the new API event listener instead of the old one Also not that the new event listener allows to filter on the component's own events by comparing its `componentId`, so we ensure it won't react to changes from the HomeView or any other components
As said in the previous commits: with the new API, the Animation view is responsible to handle its own FlagshipUI, so we don't need to keep track of its state inside of the CozyAppScreen
The SplashScreenService is not a React component, so we use `flagshipUIEventHandler.emit` to handle the new FlagshipUI API As it is not a React component, it doesn't have the same lifecycle and it won't be unmounted Instead of simulating registering/unregistering it from the FlagshipUIService, we make it live "forever" in the FlagshipUIService and we just "hide" it by setting an `undefined` UI when `hideSplashScreen` is called This would avoid having to handle initial SplashScreen that already exists when the app is started
Now that OsReceive uses the new FlagshipUI API, then this hack is not needed anymore Related PR: #1043
This hack was created to set the correct FlagshipUI when leaving the OsReceive screen With the new API the HomeView's FlagshipUI is automatically restored when OsReceiveScreen is unmounted Related commit: b5b6dd1
The LockScreenBars seemed to be here to handle a non-immersive screen and would suggest that Biometry screens are not immersive But after a few tests it seems like every part of this screens and so there is no need to top and bottom bars. Even more, this seems to fix a bug as in a test device this would produce darken status and navigation bars as those would overlap with the OS's Biometric prompt backdrop
`cozy-intent` has been upgraded to `2.19.0` to retrieve `tryEmit` improvements for new FlagshipUI API Related PR: cozy/cozy-libs#2360
Ldoppea
force-pushed
the
feat/rework_set_flagship_ui
branch
from
4bb373b to
7b2a6c9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The FlagshipUI is responsible to apply the correct theme to status and navigation bars base on the displayed screen or cozy-app
Previous
setFlagshipUIAPI had numerous caveats:This new implementation is an attempt to simplify the API and to reduce the need of hacks and side effects
It is based on new out-of-react service (
FlagshipUIService) that keeps track of all components that use theuseFlagshipUI()hookWhen a component registers, unregister or change its color, then the
FlagshipUIServiceupdates its reference array and recompute the new FlagshipUI to be appliedTo ease the rendering order calculation, we hardcoded array of ZIndex that every component should use when calling the
useFlagshipUI()hookThis can be done because the application UI is simple enough so we know which screen should be on top of others
This seems to be the only reliable way to predict screens order. We tried to guess components positions based on React tree using different
hooks (i.e. useMemo, useEffect, useLayoutEffect, useFocus), but none of them would allows us to keep track of screens order
We chose to put this mechanism out of React so changing the status and navigation bars would not produce any re-rendering
Related PR: cozy/cozy-libs#2360
What can be done next
Some improvements can be done for this API
First one is that this API wouldn't be usable on generic components (i.e. modals). To allow this, we can create a React Provider that would allow any component to retrieve its ZIndex from the corresponding Context. This provider would have to be put in a parent component like we do for the
useFlagshipUI()hookSecond one it that current implementation only handle react-native side, but we keep the old architecture on cozy-apps. If this new API is proven to be stable and useful, then we can port it on cozy-intent so cozy-apps would be able to use it
Some parts of the code can still be cleaned. This is the case of
setFlagshipUI.tsandthemeManager.tsthat still have some complex code. It is a bit early to clean them as it is unclear how much we need them with the new API. But we should keep in mind to recheck this part in a few weeksTODO:
componentIdparameter totryEmitcozy-libs#2360 being merged