[android] Map fragments improvements #4808

rtsisyk · 2023-03-18T10:23:38Z

arnaudvergnet commented

2023-03-18 10:23:38 +00:00

(Migrated from github.com)

Further improve place page and map buttons fragment abstraction by trying to follow Android's best practices.

With these changes, the main activity does not call fragment functions directly, but instead updates their state using view models. Fragments send data back to the activity using event callbacks.

Saving the UI state between configuration changes is a lot easier as view models handle this natively, it is easier to sync multiple UI components to a single state and we better understand when the data is modified.

The menu bottom sheets would also benefit from this change, but I will keep that for another PR.

This is not ready yet, I still need to find and fix possible regressions, but this will make the code easier to maintain and will also help with implementing organicmaps/organicmaps#4613.

@Jean-BaptisteC you don't need to test yet, I already know of some bugs, but I would greatly appreciate your help when I feel the PR is close enough to being finished :). The PR is only published to let people know I'm working on this.

Fixes #4729
Fixes #4903
Fixes #4680

Further improve place page and map buttons fragment abstraction by trying to follow [Android's best practices](https://developer.android.com/topic/architecture/ui-layer?continue=https%3A%2F%2Fdeveloper.android.com%2Fcourses%2Fpathways%2Fandroid-architecture%23article-https%3A%2F%2Fdeveloper.android.com%2Ftopic%2Farchitecture%2Fui-layer#udf). With these changes, the main activity does not call fragment functions directly, but instead updates their state using view models. Fragments send data back to the activity using event callbacks. Saving the UI state between configuration changes is a lot easier as view models handle this natively, it is easier to sync multiple UI components to a single state and we better understand when the data is modified. The menu bottom sheets would also benefit from this change, but I will keep that for another PR. This is not ready yet, I still need to find and fix possible regressions, but this will make the code easier to maintain and will also help with implementing https://git.omaps.dev/organicmaps/organicmaps/pulls/4613. @Jean-BaptisteC you don't need to test yet, I already know of some bugs, but I would greatly appreciate your help when I feel the PR is close enough to being finished :). The PR is only published to let people know I'm working on this. Fixes #4729 Fixes #4903 Fixes #4680

👍 1

Jean-BaptisteC (Migrated from github.com) approved these changes 2023-03-18 16:49:38 +00:00

Jean-BaptisteC (Migrated from github.com) left a comment

✅ Pixel 6 - Android 13

I have see weird bugs in first launch, but i'm not able to reproduce

Open OM
Select POI -> open place page
Open editor
Leave editor without save -> place page is closed

Others problem (buttons zoom is disabled)

select POI -> expand place page
Rotate device ->informations on place page disappear

✅ Pixel 6 - Android 13 I have see weird bugs in first launch, but i'm not able to reproduce - Open OM - Select POI -> open place page - Open editor - Leave editor without save -> place page is closed Others problem (buttons zoom is disabled) - select POI -> expand place page - Rotate device ->informations on place page disappear <img src="https://user-images.githubusercontent.com/87148630/226121049-c8c4bf6b-d991-4735-ac51-184ee39f0368.png" height=400 />

Jean-BaptisteC (Migrated from github.com) approved these changes 2023-03-19 17:59:48 +00:00

Jean-BaptisteC (Migrated from github.com) left a comment

✅ Pixel 6 - Android 13
Bugs about layer ✅
Bugs about download button on place page ✅

Find another bug :)

Select town or airport outside of downloaded map (with link to website)
Start download map
Open website on browser
Go back to OM -> button about download is not present on place page

✅ Pixel 6 - Android 13 Bugs about layer ✅ Bugs about download button on place page ✅ Find another bug :) - Select town or airport outside of downloaded map (with link to website) - Start download map - Open website on browser - Go back to OM -> button about download is not present on place page

biodranik (Migrated from github.com) reviewed 2023-04-09 21:50:06 +00:00

biodranik (Migrated from github.com) left a comment

@rtsisyk let's proceed with this refactoring to unblock AA and avoid many merge conflicts caused by delayed merging.

android/src/app/organicmaps/widget/placepage/sections/PlacePageOpeningHoursFragment.java

					
				@ -0,0 +188,4 @@

				  public void onStop()

				  {

				    super.onStop();

				    mViewModel.getMapObject().removeObserver(this);

biodranik (Migrated from github.com) commented

2023-04-09 21:48:18 +00:00

    if (mapObject != null)
      refreshOpeningHours(mapObject);

```suggestion if (mapObject != null) refreshOpeningHours(mapObject); ```

android/src/app/organicmaps/widget/placepage/sections/PlacePagePhoneFragment.java

					
				@ -0,0 +50,4 @@

				  public void onStop()

				  {

				    super.onStop();

				    mViewModel.getMapObject().removeObserver(this);

biodranik (Migrated from github.com) commented

2023-04-09 21:49:00 +00:00

    if (mapObject != null)
      mPhoneAdapter.refreshPhones(mapObject.getMetadata(Metadata.MetadataType.FMD_PHONE_NUMBER));

```suggestion if (mapObject != null) mPhoneAdapter.refreshPhones(mapObject.getMetadata(Metadata.MetadataType.FMD_PHONE_NUMBER)); ```

biodranik (Migrated from github.com) reviewed 2023-04-10 12:38:24 +00:00

android/src/app/organicmaps/maplayer/MapButtonsController.java

biodranik (Migrated from github.com) commented

2023-04-10 12:38:24 +00:00

Something is very wrong with formatting here.

biodranik (Migrated from github.com) reviewed 2023-04-10 12:41:04 +00:00

android/src/app/organicmaps/maplayer/MapButtonsController.java

biodranik (Migrated from github.com) commented

2023-04-10 12:41:03 +00:00

What are these different observers, and what is their role/usefulness for the user, especially when some dialogs are displayed on top of the paused activity?
What is the full list of dialogs that may pause the activity? E.g. the "location is disabled" dialog (it means that subscribing to location updates doesn't make sense yet, right?), what else?

1. What are these different observers, and what is their role/usefulness for the user, especially when some dialogs are displayed on top of the paused activity? 2. What is the full list of dialogs that may pause the activity? E.g. the "location is disabled" dialog (it means that subscribing to location updates doesn't make sense yet, right?), what else?

arnaudvergnet (Migrated from github.com) reviewed 2023-04-10 12:58:12 +00:00

android/src/app/organicmaps/maplayer/MapButtonsController.java

arnaudvergnet (Migrated from github.com) commented

2023-04-10 12:58:12 +00:00

The observers related to mMapButtonsViewModel allow the buttons to react to changes in the app state, such as when the position mode or the layers change, etc.

The observer on mPlacePageViewModel.getPlacePageDistanceToTop allows the buttons to follow/avoid the place page when it slides. This observer cannot be removed in onPause otherwise the buttons will not follow the place page if it is sliding while there is a popup over the screen (eg you click share and swipe down the place page at the same time, the place page will slide while the share popup pauses the activity).

To make the code easier to understand, I chose to remove every observer in onStop.

The observers related to `mMapButtonsViewModel` allow the buttons to react to changes in the app state, such as when the position mode or the layers change, etc. The observer on `mPlacePageViewModel.getPlacePageDistanceToTop` allows the buttons to follow/avoid the place page when it slides. This observer cannot be removed in `onPause` otherwise the buttons will not follow the place page if it is sliding while there is a popup over the screen (eg you click share and swipe down the place page at the same time, the place page will slide while the share popup pauses the activity). To make the code easier to understand, I chose to remove every observer in `onStop`.

biodranik (Migrated from github.com) reviewed 2023-04-17 22:31:47 +00:00

android/src/app/organicmaps/maplayer/MapButtonsController.java

biodranik (Migrated from github.com) commented

2023-04-17 22:31:47 +00:00

l. That's a leak. Resume may be called many times.
2. It would be safer to leave the old behavior, and only subscribe/unsubscribe in onStop the one needed observer. It will also save users batteries in many cases.

l. That's a leak. Resume may be called many times. 2. It would be safer to leave the old behavior, and only subscribe/unsubscribe in onStop the one needed observer. It will also save users batteries in many cases.

arnaudvergnet (Migrated from github.com) reviewed 2023-04-18 09:07:20 +00:00

android/src/app/organicmaps/maplayer/MapButtonsController.java

arnaudvergnet (Migrated from github.com) commented

2023-04-18 09:07:19 +00:00

From the documentation on observe: If the given owner, observer tuple is already in the list, the call is ignored. If the observer is already in the list with another owner, LiveData throws an IllegalArgumentException.. So there is no leak.
onStop is safer and will prevent weird UI bugs when displaying system popups. I think we should only use onPause when strictly necessary so we don't introduce strange lifecycle behaviors.

1. From the documentation on `observe`: `If the given owner, observer tuple is already in the list, the call is ignored. If the observer is already in the list with another owner, LiveData throws an IllegalArgumentException.`. So there is no leak. 2. onStop is safer and will prevent weird UI bugs when displaying system popups. I think we should only use `onPause` when strictly necessary so we don't introduce strange lifecycle behaviors.

arnaudvergnet (Migrated from github.com) reviewed 2023-04-18 09:08:35 +00:00

android/src/app/organicmaps/maplayer/MapButtonsController.java

arnaudvergnet (Migrated from github.com) commented

2023-04-18 09:08:35 +00:00

For example onResume may be called right after onPause without triggering onStop. That makes handling lifecycle more complex.

For example `onResume` may be called right after `onPause` without triggering `onStop`. That makes handling lifecycle more complex.

biodranik (Migrated from github.com) reviewed 2023-04-18 21:14:51 +00:00

android/src/app/organicmaps/maplayer/MapButtonsController.java

biodranik (Migrated from github.com) commented

2023-04-18 21:14:50 +00:00

It is an inconsistent pattern to start something in onResume but stop it onStop.

A more general question: the ViewModel approach doesn't make the code easier, it's more connected now than before the refactoring. Are there any better ways? Maybe not following official "pattern recommendations", but something simpler, with fewer dependencies and fewer lines of code?

It is an inconsistent pattern to start something in onResume but stop it onStop. A more general question: the ViewModel approach doesn't make the code easier, it's more connected now than before the refactoring. Are there any better ways? Maybe not following official "pattern recommendations", but something simpler, with fewer dependencies and fewer lines of code?

arnaudvergnet (Migrated from github.com) reviewed 2023-04-19 05:32:18 +00:00

android/src/app/organicmaps/maplayer/MapButtonsController.java

arnaudvergnet (Migrated from github.com) commented

2023-04-19 05:32:18 +00:00

I don't see why it is inconsistent as onResume is called both on activity start and on activity resume, but I don't have that much android experience.

After reading the docs a bit more, I think we may be able to simplify this and only observe/remove in onCreate/onDestroy because the viewmodels are linked to the activity lifecycle and thus will never fire when the app goes into background.

Regarding your question, it does make the code simpler. You now have a clear dependency on only the viewmodels, while before you had a mess of callbacks from different classes interacting with each other. Fragments should not depend on any other class than viewmodels. It makes it a lot easier to handle lifecycle as you don't have to worry about using omSaveState, the viewmodel does it for you.

I don't see why it is inconsistent as onResume is called both on activity start and on activity resume, but I don't have that much android experience. After reading the docs a bit more, I think we may be able to simplify this and only observe/remove in onCreate/onDestroy because the viewmodels are linked to the activity lifecycle and thus will never fire when the app goes into background. Regarding your question, it does make the code simpler. You now have a clear dependency on only the viewmodels, while before you had a mess of callbacks from different classes interacting with each other. Fragments should not depend on any other class than viewmodels. It makes it a lot easier to handle lifecycle as you don't have to worry about using omSaveState, the viewmodel does it for you.

rtsisyk approved these changes 2023-04-23 09:18:27 +00:00

biodranik (Migrated from github.com) reviewed 2023-04-27 08:20:47 +00:00

biodranik (Migrated from github.com) left a comment

It become too big for a detailed review. We can hope for good testing from @Jean-BaptisteC

It would be great to do similar refactoring in steps or to extract independent parts of them into separate PRs/commits.

It become too big for a detailed review. We can hope for good testing from @Jean-BaptisteC It would be great to do similar refactoring in steps or to extract independent parts of them into separate PRs/commits.

vng (Migrated from github.com) reviewed 2023-04-27 16:20:30 +00:00

android/src/app/organicmaps/MwmActivity.java

vng (Migrated from github.com) commented

2023-04-27 16:00:46 +00:00

nit: I prefer avoiding code like this with additional var:
final ? val = mMapButtonsViewModel.getBottomButtonsHeight().getValue();

nit: I prefer avoiding code like this with additional var: ```final ? val = mMapButtonsViewModel.getBottomButtonsHeight().getValue();```

android/src/app/organicmaps/maplayer/MapButtonsController.java

vng (Migrated from github.com) commented

2023-04-27 15:59:58 +00:00

nit: requireActivity() I prefer avoiding code like this with additional var

android/src/app/organicmaps/maplayer/MapButtonsController.java

vng (Migrated from github.com) commented

2023-04-27 16:04:39 +00:00

nit: I'd prefer additional var: final Activity activity = requireActivity();

nit: I'd prefer additional var: ```final Activity activity = requireActivity();```

android/src/app/organicmaps/maplayer/MapButtonsController.java

vng (Migrated from github.com) commented

2023-04-27 16:02:44 +00:00

Is it ok, that we return true (isBehindPlacePage) when there is no placePageWidth ?

android/src/app/organicmaps/maplayer/MapButtonsViewModel.java

					
				@ -0,0 +11,4 @@

				  private final MutableLiveData<MapButtonsController.LayoutMode> mLayoutMode = new MutableLiveData<>(MapButtonsController.LayoutMode.regular);

				  private final MutableLiveData<Integer> mMyPositionMode = new MutableLiveData<>();

				  private final MutableLiveData<Mode> mMapLayerMode = new MutableLiveData<>();

				  private final MutableLiveData<SearchWheel.SearchOption> mSearchOption = new MutableLiveData<>();

vng (Migrated from github.com) commented

2023-04-27 16:06:20 +00:00

Just curious, what is a MutableLiveData and why it is needed here?

android/src/app/organicmaps/widget/placepage/PlacePageController.java

vng (Migrated from github.com) commented

2023-04-27 16:19:43 +00:00

Do we need to call here?
PlacePageUtils.updateMapViewport(mCoordinator, mDistanceToTop, mViewportMinHeight);
mDistanceToTop was changed

Do we need to call here? ```PlacePageUtils.updateMapViewport(mCoordinator, mDistanceToTop, mViewportMinHeight);``` mDistanceToTop was changed

android/src/app/organicmaps/widget/placepage/PlacePageUtils.java

vng (Migrated from github.com) commented

2023-04-27 16:14:25 +00:00

@rtsisyk Will be great if you can test this PR on your Huawei with this problem

@rtsisyk Will be great if you can test this PR on your Huawei with [this](https://git.omaps.dev/organicmaps/organicmaps/pulls/5023) problem

android/src/app/organicmaps/widget/placepage/PlacePageViewModel.java

					
				@ -12,2 +12,4 @@

				  private final MutableLiveData<List<PlacePageButtons.ButtonType>> mCurrentButtons = new MutableLiveData<>();

				  private final MutableLiveData<MapObject> mMapObject = new MutableLiveData<>();

				  private final MutableLiveData<Integer> mPlacePageWidth = new MutableLiveData<>();

				  private final MutableLiveData<Integer> mPlacePageDistanceToTop = new MutableLiveData<>();

vng (Migrated from github.com) commented

2023-04-27 16:16:03 +00:00

Same here, why MutableLiveData ?

arnaudvergnet (Migrated from github.com) reviewed 2023-04-29 13:28:30 +00:00

android/src/app/organicmaps/maplayer/MapButtonsViewModel.java

					
				@ -0,0 +11,4 @@

				  private final MutableLiveData<MapButtonsController.LayoutMode> mLayoutMode = new MutableLiveData<>(MapButtonsController.LayoutMode.regular);

				  private final MutableLiveData<Integer> mMyPositionMode = new MutableLiveData<>();

				  private final MutableLiveData<Mode> mMapLayerMode = new MutableLiveData<>();

				  private final MutableLiveData<SearchWheel.SearchOption> mSearchOption = new MutableLiveData<>();

arnaudvergnet (Migrated from github.com) commented

2023-04-29 13:28:29 +00:00

Here is the documentation: https://developer.android.com/topic/libraries/architecture/livedata#java.

This is a lifecycle aware observable value. When you create the view model, you tie it to an activity. Then the live data inside that view model follows the activity life cycle, so it does not trigger callbacks when the activity is stopped and automatically removes listeners when it is destroyed.

In our case, when we use the viewmodels in fragments we still have to remove the listeners because they have a shorter life cycle than the parent activity (to prevent the listeners from trying to update a destroyed UI).

Tying the view model to the parent activity allows us the reuse the value across different fragments. So the viewmodels store the UI state, and fragments can simply observe the live data to update their UI. No more onSavedInstanceState, no more dependency injection all over the place and we are sure all the fragments use the same state.

This makes debugging a lot easier and makes the codebase more robust. But then again I am no Android expert.

Here is the documentation: https://developer.android.com/topic/libraries/architecture/livedata#java. **This is a lifecycle aware observable value**. When you create the view model, you tie it to an activity. Then the live data inside that view model follows the activity life cycle, so it does not trigger callbacks when the activity is stopped and automatically removes listeners when it is destroyed. In our case, when we use the viewmodels in fragments we still have to remove the listeners because they have a shorter life cycle than the parent activity (to prevent the listeners from trying to update a destroyed UI). Tying the view model to the parent activity allows us the reuse the value across different fragments. So the viewmodels store the UI state, and fragments can simply observe the live data to update their UI. No more `onSavedInstanceState`, no more dependency injection all over the place and we are sure all the fragments use the same state. This makes debugging a lot easier and makes the codebase more robust. But then again I am no Android expert.

arnaudvergnet (Migrated from github.com) reviewed 2023-04-29 13:33:00 +00:00

android/src/app/organicmaps/widget/placepage/PlacePageController.java

arnaudvergnet (Migrated from github.com) commented

2023-04-29 13:32:59 +00:00

To improve performance we only update the viewport when the place page is settled, which happens after the place page finished sliding.

If we call PlacePageUtils.updateMapViewport(mCoordinator, mDistanceToTop, mViewportMinHeight); here then the viewport will update as the place page moves.

To improve performance we only update the viewport when the place page is settled, which happens after the place page finished sliding. If we call `PlacePageUtils.updateMapViewport(mCoordinator, mDistanceToTop, mViewportMinHeight);` here then the viewport will update as the place page moves.

vng (Migrated from github.com) approved these changes 2023-04-29 18:20:59 +00:00

vng (Migrated from github.com) left a comment

Gonna test this PR myself for updateMapViewport.

biodranik (Migrated from github.com) reviewed 2023-04-30 10:18:49 +00:00

biodranik (Migrated from github.com) left a comment

Thanks! Two more issues I found:

In the navigation mode in landscape, when you select some POI, it jumps too much up, and sometimes becomes hidden (on the narrow device). Likely because of the perspective.
When I build a route in landscape, with the finish point at the top, it always becomes hidden by the top bar with routing mode icons. Not sure if it's related to this PR, but it should be fixed anyway. Maybe in a separate PR.

Thanks! Two more issues I found: 1. In the navigation mode in landscape, when you select some POI, it jumps too much up, and sometimes becomes hidden (on the narrow device). Likely because of the perspective. 2. When I build a route in landscape, with the finish point at the top, it always becomes hidden by the top bar with routing mode icons. Not sure if it's related to this PR, but it should be fixed anyway. Maybe in a separate PR.

vng (Migrated from github.com) reviewed 2023-04-30 17:59:16 +00:00

android/src/app/organicmaps/widget/placepage/PlacePageUtils.java

vng (Migrated from github.com) commented

2023-04-30 17:59:16 +00:00

I did testing on my device and it works ok. @rtsisyk Please, check your Huawei.

This repo is archived. You cannot comment on pull requests.

No reviewers

Jean-BaptisteC

rtsisyk

vng