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

[android] [ios] Split the deactivate PP and switch fullscreen mode logic #8443

Merged

Conversation

kirylkaveryn
Copy link
Contributor

@kirylkaveryn kirylkaveryn commented Jun 11, 2024

Closes #8442

The old logic when the dismissing method should control not only the PP dismissing but also the fullscreen mode toggling contains too many responsibilities, and produces unexpected behavior (if the PP is opened and the user long taps on the map the PP will be dismissed but the side buttons not - ios, or deeplink handling hides the side buttons - ios), and hard to read and debug.

This PR continues #7876 and splits the DeactivateMapSelection(bool notifyUI) into the DeactivateMapSelection() and SwitchFullScreen().
The additional callback m_onSwitchFullScreen = std::move(onSwitchFullScreen); was added.

Fullscreen mode switching is blocked when:

  • navigation mode is active
  • searching is active

Todo:

  • implement a fix in Android. I've failed to handle all these JNI methods... @strump Can you please help me and split the methods in android?

Results:
enter/exit the fullscreen mode independently from the PP
Simulator Screen Recording - iPhone 15 - 2024-06-11 at 16 37 12

entering the fullscreen mode is inactive during the navigation or searching
Simulator Screen Recording - iPhone 15 - 2024-06-11 at 15 17 39

@biodranik
Copy link
Member

Opened PP is always closed on long tap in any mode (search, route planning,...), correct?

@kirylkaveryn
Copy link
Contributor Author

kirylkaveryn commented Jun 11, 2024

Opened PP is always closed on long tap in any mode (search, route planning,...), correct?

Yes.
Maybe this is designed behavior, but IMO it will be better if the user associates the long tap on the map only with the show/hide controls.

@strump
Copy link
Sponsor Contributor

strump commented Jun 12, 2024

@kirylkaveryn made changes to Android. Introduced PlacePageActivationListener.onSwitchFullScreenMode() method.

@strump strump force-pushed the split-deactivare-pp-and-switch-fullscreen-mode-logic branch from 0b5bae3 to 3c678fa Compare June 12, 2024 07:29
@kirylkaveryn
Copy link
Contributor Author

@kirylkaveryn made changes to Android. Introduced PlacePageActivationListener.onSwitchFullScreenMode() method.

Thank you very much!
@biodranik can you please take a look?

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@Jean-BaptisteC can you please help with testing opening/closing PP and switching full-screen mode in different scenarios on Android?

android/app/src/main/cpp/app/organicmaps/Framework.cpp Outdated Show resolved Hide resolved
@kirylkaveryn kirylkaveryn force-pushed the split-deactivare-pp-and-switch-fullscreen-mode-logic branch from 3c678fa to ab4b141 Compare June 17, 2024 06:59
Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

Thanks! It works for Android too, right?

map/framework.cpp Outdated Show resolved Hide resolved
android/app/src/main/java/app/organicmaps/MwmActivity.java Outdated Show resolved Hide resolved
@@ -1239,7 +1239,7 @@ public void onPlacePageDeactivated()
@SuppressWarnings("unused")
public void onSwitchFullScreenMode()
{
if ((mPanelAnimator && mPanelAnimator.isVisible()) ||UiUtils.isVisible(mSearchController.getToolbar()))
if ((mPanelAnimator && mPanelAnimator.isVisible()) || UiUtils.isVisible(mSearchController.getToolbar()))
Copy link
Member

Choose a reason for hiding this comment

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

🤦 Sorry, it's Java, not C++. Let's return the comparison back, polish commits, and merge.

/home/runner/work/organicmaps/organicmaps/android/app/src/main/java/app/organicmaps/MwmActivity.java:1242: error: bad operand types for binary operator '&&'
    if ((mPanelAnimator && mPanelAnimator.isVisible()) || UiUtils.isVisible(mSearchController.getToolbar()))
                        ^
  first type:  PanelAnimator
  second type: boolean

Added onSwitchFullScreenMode listener call from JNI

Signed-off-by: S. Kozyr <[email protected]>
@strump strump force-pushed the split-deactivare-pp-and-switch-fullscreen-mode-logic branch from ed5b932 to dd05917 Compare June 18, 2024 09:58
@strump
Copy link
Sponsor Contributor

strump commented Jun 18, 2024

@biodranik Fixed compilation errors. Squashed Android commits. Should fix actions

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

@strump @kirylkaveryn Д[з]якую!

@biodranik biodranik merged commit 75c22ca into master Jun 18, 2024
15 checks passed
@biodranik biodranik deleted the split-deactivare-pp-and-switch-fullscreen-mode-logic branch June 18, 2024 21:19
@pastk
Copy link
Contributor

pastk commented Jun 23, 2024

Looks like on Android it doesn't work like it should:

  1. Long tap anywhere.
  2. Buttons disappear.
  3. Tap anywhere.
  4. PP opens and buttons appear. Expected: only PP opens.

If PP was open before a long tap then button disappear but PP stays - good.
But a tap on any other spot makes button re-appear again.

@biodranik
Copy link
Member

@pastk thanks for testing! Can you please revert this change and confirm the correct behavior without it?

CC @kirylkaveryn

@pastk
Copy link
Contributor

pastk commented Jun 24, 2024

Before this PR it was almost the same, except that a long tap made PP disappear as well.

@kirylkaveryn
Copy link
Contributor Author

On iOS it works fine.
Seems like android should be fixed.

@pastk
Copy link
Contributor

pastk commented Jun 24, 2024

@strump could you please take a look? 🙏

@pastk
Copy link
Contributor

pastk commented Jun 24, 2024

TBH from a user point of view I don't understand what's the value of making PP behave independently from UI buttons?

Maybe it'll be useful to a few pro users who don't want any buttons (but they will lose ability to search, switch layers, etc).
(basically when PP is open it just hides two corner buttons, but an open PP takes incomparably more space anyway...)

IMHO a major downside is that if a user forgot how to leave the fullscreen mode then it'll be hard to find a way back (I anticipate extra support requests). While with the previous behavior any tap would have brought the UI back - easy.

@biodranik
Copy link
Member

How would you see PP "behave dependent on UI buttons"?

Now we display a one-time toast on how to return to full-screen mode.

The value is:

  • more map can be seen without distraction
  • better screenshots can be made
  • no change in behavior (a tap opens PP)
  • A use-case: use the map to decide where to go/to drive, sometimes tapping on your current position to see elevation and/or speed.

@pastk
Copy link
Contributor

pastk commented Jun 24, 2024

How would you see PP "behave dependent on UI buttons"?

Just like it worked before this PR. What was the issue with that way?

For me PP is a part of UI also (it has actionable buttons), so this distinction may confuse users (why some controls are visible and others are not?).

Now we display a one-time toast on how to return to full-screen mode.

Its easy to miss and/or forget especially if one doesn't use this feature often or long tapped by mistake..

* A use-case: use the map to decide where to go/to drive, sometimes tapping on your current position to see elevation and/or speed.

Yes, but for me seems like a very niche use case considering potential "where did the buttons go? help!" support requests..

@kirylkaveryn
Copy link
Contributor Author

The problem with the previous implementation is that there was no actually fullscreen mode toggling but some hacks in the framework with this notify UI parameter (which is not obvious).

Maybe we should disable the fullscreen mode with the every tap (long or not, selecting a poi etc) on the screen?

@pastk
Copy link
Contributor

pastk commented Jun 24, 2024

The problem with the previous implementation is that there was no actually fullscreen mode toggling but some hacks in the framework with this notify UI parameter (which is not obvious).

Yeap, fine, its good to improve the internals.

I'm focusing on user-visible / behavior consequences.

Maybe we should disable the fullscreen mode with the every tap (long or not, selecting a poi etc) on the screen?

Yeap, essentially it'll bring back the old behavior which I think is the optimal one.

@kirylkaveryn
Copy link
Contributor Author

kirylkaveryn commented Jun 24, 2024

Yeap, essentially it'll bring back the old behavior which I think is the optimal one.

Ios worked in another way and only the long tap brought the controls back.

@biodranik WDYT to show the controls on every action? I can fix it.

@biodranik
Copy link
Member

biodranik commented Jun 24, 2024

Thanks, now I understand the problem. The old behavior on a long tap, with an open PP was:

  1. To hide PP first.
  2. Then switch into a full-screen mode.

The new behavior now (inconsistent and not familiar to our users) is:

  1. Full-screen mode is switched on (buttons are hidden), but PP stays on the screen.
  2. PP can be closed only by closing it explicitly with an x button or a swipe down.

The question is: which of the following options is better?

Option 1: When full-screen mode is toggled with open PP (long tap), everything hides (PP + buttons)

Option 2 (closer to the old behavior):

  1. Long tap with an open PP always closes PP and does not switch the full-screen mode (buttons stay on the screen).
  2. A second long tap (when PP is closed) switches the full-screen mode.

Which option do you think is better? Do you have other ideas?

@kirylkaveryn @pastk @strump

@pastk
Copy link
Contributor

pastk commented Jun 24, 2024

Option 1: When full-screen mode is toggled with open PP (long tap), everything hides (PP + buttons)

I prefer this one.
(actually that's how it worked on Android before this PR)

Option 2 (closer to the old behavior):
1. Long tap with an open PP always closes PP and does not switch the full-screen mode (buttons stay on the screen).
2. A second long tap (when PP is closed) switches the full-screen mode.

I don't think long tap will be popular for closing PP - it feels too tedious to do it this way, its better to learn to use the x button or swipe or back button (and its enough of options to do the same thing already :)

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.

[ios] The app enters the FullScreen mode when handles the deeplink
4 participants