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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 3 commits into from
+93 −70

Conversation

kirylkaveryn
Copy link
Contributor

@kirylkaveryn kirylkaveryn commented Jun 11, 2024 •
edited

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

@kirylkaveryn
[core] split onClose and onSwitchFullScreen
06c92cf 
Signed-off-by: Kiryl Kaveryn <kirylkaveryn@gmail.com>
@kirylkaveryn
[ios] split logic for the closing the PP and show/hide side buttons
a30001f 
Signed-off-by: Kiryl Kaveryn <kirylkaveryn@gmail.com>
@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 •
edited

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?

biodranik
biodranik approved these changes Jun 15, 2024
View reviewed changes
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
biodranik
biodranik approved these changes Jun 17, 2024
View reviewed changes
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
biodranik
biodranik reviewed Jun 18, 2024
View reviewed changes
android/app/src/main/java/app/organicmaps/MwmActivity.java Outdated
@@ -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

@strump
Introduced method PlacePageActivationListener.onSwitchFullScreenMode()
dd05917 
Added onSwitchFullScreenMode listener call from JNI

Signed-off-by: S. Kozyr <s.trump@gmail.com>
@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@biodranik biodranik biodranik approved these changes

@strump strump Awaiting requested review from strump

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ios] The app enters the FullScreen mode when handles the deeplink
3 participants
@kirylkaveryn @biodranik @strump