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: Use Back to open the emulation menu on all devices #9004
Conversation
3ac9b3d
to
644de53
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I can easily test this menu while emulating! Some of these critiques aren’t directly related to this PR but they should probably be taken care of before merging this:
- Bringing up the menu then immediately pressing a button on the Wii overlay results in a crash:
2020-08-06 16:05:17.899 1479-1607/? E/WindowManager: RemoteException occurs on reporting focusChanged, w=Window{16a1341 u0 org.dolphinemu.dolphinemu.debug/org.dolphinemu.dolphinemu.activities.EmulationActivity EXITING}
android.os.DeadObjectException
at android.os.BinderProxy.transactNative(Native Method)
at android.os.BinderProxy.transact(Binder.java:1145)
at android.view.IWindow$Stub$Proxy.windowFocusChanged(IWindow.java:500)
at com.android.server.wm.WindowState.reportFocusChangedSerialized(WindowState.java:3951)
at com.android.server.wm.WindowManagerService$H.handleMessage(WindowManagerService.java:5494)
at android.os.Handler.dispatchMessage(Handler.java:106)
at android.os.Looper.loop(Looper.java:214)
at android.os.HandlerThread.run(HandlerThread.java:65)
at com.android.server.ServiceThread.run(ServiceThread.java:44) - It would probably be better to keep the ability to exit emulation by pressing back twice. On phones with large screens (like Samsung Note8), pressing back then pressing exit on the opposite side of the screen is cumbersome. Testing mods or Dolphin itself can require many emulation restarts.
- Pressing “back” then swiping up causes the menu to briefly pop-up and disappear from the screen.
- Rename "Exit" to "Exit Emulation", since it is unclear if we're exiting emulation or the menu.
- I think the space between the game title and first menu option is a little large. Since there are now many menu options it would be better to reduce that space if possible.
- Japanese characters stretch onto second line of game title (tested with R4YJ2M):

I can't reproduce this. (I also can't make sense of the stack trace...)
The problem with this is that pressing Back currently is the only way to close the menu if you don't have a touchscreen. What do you think about letting a long press of the Back button exit emulation instead?
I'm not sure if I understand this issue correctly. If I swipe up in an area outside the menu, the menu disappears like it normally does when I touch an area that's outside the menu. If I swipe up in an area inside the menu, I scroll the ScrollView.
Done.
Done.
It looks to me like the title in your screenshot is just barely longer than what fits on one line. Isn't it normal to put the last part of the title on a second line in that case? |
|
The last two added commits LGTM.
Sorry, these are the correct steps to reproduce the crash:
That's fine and we should probably have a "Long press back to exit" toast appear when initially pressing back so users are aware of this feature.
I should clarify, this all needs to be done in one fluid motion:
You'll see the menu very briefly pop-up, which doesn't look great. But now that I think about it I'm not sure if we can reasonably do anything about this...
Yes but I think there is room to fit that title on one line without changing the size of the menu. Couldn't we extend |
9eae2b9
to
d5a4638
Compare
Well, I've reproduced some sort of crash: This crash should be fixed with the latest push.
EDIT: Actually, never mind, the long press detection doesn't work at all. I'll look into this more tomorrow.
Reproduced. I think these steps are "unusual" enough that it isn't really a big deal.
We could decrease the margin on the right, but I think it looks odd to have a smaller margin on the right than on the left. I don't believe it would be a good idea to decrease the margin on the left, because Google suggests horizontal margins of 48dp (5% of the screen) at the left and right edges of the screen on Android TV to compensate for overscan, which is actually more than the current 32dp. |
d5a4638
to
be46b61
Compare
|
Fixed the long press detection issue. |
|
New problem. After long pressing to end emulation, a game cannot be launched afterwards, probably due to the check from #8860. This does not occur with the "Exit Emulation" menu option.
The crash was fixed.
Could we add "Long press back to exit" to
Agreed, feel free to ignore that point.
I'm fine with not changing the left margins but I still think we should use the space we have for the title to maximize the number of settings on the screen. Could we change |
be46b61
to
6cb114f
Compare
So guess what? I actually noticed and fixed this yesterday, but forgot to commit it before pushing. I guess that's what happens when you try to code right before going to bed :) It should work with the latest push.
Hmm... I don't know... It's partially that I don't want to make any particular toast too long (you only have a limited number of seconds to read them, and not everyone is fluent in English) and partially that I don't want users to think that that's the only way to exit and that they need to memorize that a long press is how you exit.
218dp is 250dp minus 32dp, so... You're essentially suggesting to make the right margin 0dp? That would look pretty bad. I tried changing the right margin to 24dp, which I believe would provide just enough room to fit your title on one line and which provides enough room to fit "The Legend of Zelda: The Wind Waker" on two lines, and it didn't look too ugly, but it did still look a little odd. Keep in mind that no matter where we draw the line for how wide the title should be, there'll always be some title that needs just a tiny bit more space to take up one line less. We have to draw the line somewhere, and I believe the most sensible line to draw is making the right margin equal to the left margin. |
I'll add it to the toast.
That effectively changes the right margin. The screenshot you just posted has less than 32dp of space between the end of the text and the end of the menu, if you include the empty space of the i. The reason why this looks fine in this specific case is because fullwidth i has a lot of empty space in it, but this reason does not apply to game titles that end in essentially any other character. I don't want to make other titles look worse just to improve the rendering of titles that consist of exactly 9 CJK characters ending in i. |
6cb114f
to
f360b6b
Compare
|
I have increased the menu width by 10dp, which should hopefully make it able to fit 9 CJK characters on one line. |
|
I like all of the changes now. I need to leave but I'll give this a final pass later today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Save/Load state slots do nothing when tapped.
- Previously, swiping from the very top or bottom of the screen would display bars hidden by immersive mode in addition to Dolphin's action bar. I'd like the menu from this PR to also display when unhiding the bars, similar to how the action bar was unhidden, which will make the menu easier to access.
- It would probably be better if the menu options closed the menu upon completion. I'm assuming consecutive menu actions aren't common.
EmulationActivity'sMENU_ACTION_PAUSE_EMULATIONandMENU_ACTION_UNPAUSE_EMULATIONcases are only called by the old menu. These cases are handled inMenuFragment'sonClickoverride now. We don't need the old menu code anymore, including some of the methods called in those cases. It's also annoying that I can't make a review comment far away from code you've edited.
f360b6b
to
8870d24
Compare
Fixed. Because such a tap is outside of the main menu, Dolphin interpreted it as a command to close the main menu, and it seems like that led to the submenu being closed in the process.
I believe that bringing up a UI element that blocks such a large portion of the screen is not a good thing to trigger with an action that the user easily can perform by accident, not to mention that I don't think there's any precedent for apps doing something like this, so it wouldn't be consistent with anything the user is used to. There is also the problem of whether the menu should be dismissed automatically after doing this. Dismissing it automatically would probably make it hard to use (because you'll probably want to read the different options there are before pressing anything), but not dismissing it automatically is inconsistent with everything else that appears when performing the same action. I don't think these problems are worth it for getting rid of one extra tap (and only on touchscreen devices which are not using gestural navigation, at that).
Actually, the menu closing after selecting something was one of my biggest annoyance points with the old menu. I think the menu is quick and easy enough to close that this isn't a big problem.
I have removed the redundant code, but in a slightly different way that let me remove the |
Nothing happens when save slots are tapped in portrait mode still. Landscape mode appears to work now.
Fair points, drop this suggestion.
I think we'd only want to keep the menu open if a user is likely to select multiple menu options. Are there common reasons to do this? If not I don't see a reason to keep the menu open.
That part LGTM. |
8870d24
to
d7988fb
Compare
Fixed.
Reproduced, but I have no idea why this PR would cause it to happen or what to do about it. It also seems like it happens for just one frame.
There is such an OSD message, but it ends up being entirely obscured by the menu. Not sure what the best solution is here...
Fixed. I was trying to compare coordinates that were relative to
I can't think of a specific scenario where it would be common to want to change two settings in a row. But sometimes I need to look around in the menus (well, only really the overlay controls menu...) to remember what options are available, and I believe it's useful for that. And every user has to go through the options at least once if they want to know what options there are. |
Ok since it's a minor graphical bug that doesn't need to block this PR.
Oh, that problem was actually the save slot not responding to taps, which has been fixed.
If there isn't a common scenario for changing multiple menu settings I still think we should close the menu upon making a selection. This would greatly improve the problem of the menu covering OSD messages. Even though not closing the menu may be slightly more convenient for users to learn what our settings do, I don't think that justifies the additional taps that will always be required to close the menu. |
Regarding savestate loading specifically, I believe there may be cases where someone has multiple savestates and doesn't remember slot contains the one they want. The only way to find out which one is which is to try loading all the savestates until they find the right one, which can be done a lot faster if the savestate submenu isn't dismissed after loading a savestate. I do think the additional taps are justified, since it only requires a single tap on a rather large area of the screen. You can move your right thumb into the position of the A/B/X/Y buttons, tap the screen to dismiss the menu, and then continue playing without having to move your finger to another location. |
|
Alright that case makes sense to me. I need to leave my computer soon so I'll give this PR another check today or tomorrow. And this doesn't need to be done in this PR but what do you think about aligning OSD messages to the right side of the screen on Android? I don't know how feasible that would be. |
|
I'm not sure if that would be easy or not. But I should be able to make a PR that adjusts the x coordinates of OSD messages while the menu is open, which is probably the better solution anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now although issues should probably filed for:
- When saving a state, a blank OSD message box usually appears and disappears before showing the successful save OSD message. It appears when the "Saving State..." OSD message is expected but the message box is much larger than that. Easier to reproduce by saving the state multiple times. Code in
OSD::DrawMessages's for loop is initially triggered once as expected, but eventually becomes triggered multiple times per each OSD message. - Adjust the x coordinates of OSD messages while the menu is open.
|
I would disagree with the long-press-Back thing since on Android 10 this input essentially does not exist in gesture mode - swiping from the side and holding doesn't do anything, it just waits till you take your finger off the screen and activates a normal Back press |
|
I don't see any harm in keeping the option of long pressing even if only some users can use it. Though I guess it is somewhat weird for a toast to mention a feature that you might not be able to use... |
|
I've added 32dp of extra space when the navigation bar is at the bottom of the screen. |
I like this change. I also noticed the four save/load menu options went missing. See the last screenshot. |
|
They work for me. You most likely just haven't enabled them in the settings. |
Nevermind, that fixed things. I must have deleted my Dolphin.ini for testing at some point and forgotten about it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now although issues should probably filed for:
- When saving a state, a blank OSD message box usually appears and disappears before showing the successful save OSD message. It appears when the "Saving State..." OSD message is expected but the message box is much larger than that. Easier to reproduce by saving the state multiple times. Code in
OSD::DrawMessages's for loop is initially triggered once as expected, but eventually becomes triggered multiple times per each OSD message. - Adjust the x coordinates of OSD messages while the menu is open.
Now emulation is easy to exit both with and without gesture navigation thanks to the lower position of "Exit Emulation", which hopefully will be useful for @MANGOM1LK.
With a navigation bar or physical buttons, emulation can be ended by long pressing back or the "Exit Emulation" menu button. Without a navigation bar, a back gesture can be performed and then "Exit Emulation" can be tapped, which is comparable to the old double tap back method.
|
@JosJuice I think I figured out how to make OSD messages adapt to emulation menu visibility and survive orientation changes. Let me know if I should make this a separate PR: Ebola16@c9dd638 Also, I updated the other non-blocking issue. I may get around to testing this further but feel free to solve it before me:
|
|
I have a different solution in mind that I believe will work better. I'll create a PR for it once this PR is merged (due to merge conflicts). |
|
Can't argue with something that works better, that's fine. |
b3a7f8e
to
715e96d
Compare
https://bugs.dolphin-emu.org/issues/12029 We currently have one way of opening the menu on touch screen devices (swiping down from the top of the screen to bring up the action bar and selecting the menu in the action bar), and another way of opening the menu on Android TV (pressing Back). However, some devices that claim to support touch (or don't support leanback? Dolphin currently conflates the two) don't actually let you swipe down from the top of the screen in the way that Dolphin expects, notably Chromebooks. There are also some phones where you can swipe down from the top of the screen but this for some reason doesn't lead to the action bar becoming visible, though we are getting less reports about this nowadays than in the past. This change makes us use the Back method on all devices, since it should work on all devices with no significant drawbacks. Unfortunately, we not only have two different ways of triggering the menu but actually two entirely different menus, with the non-touch menu not implementing options that only are revelant when using a touch screen. A later commit will add the missing features to the menu that we now use on all devices.
Android TV devices aren't the only devices without touchscreens. Regarding MotionAlertDialog, I could've replaced the leanback check with a touchscreen check instead of just removing it, but I thought there was no reason to prevent people with touchscreens from doing a long back press if they want to.
Removing the menu for a split second before showing the transition back to the main activity looks janky.
25% of the screen isn't necessarily wide enough on phones, especially not in portrait mode.
To make it easier to access on touchscreens.
715e96d
to
cecec75
Compare
|
What's the status of this pull request? |
|
I believe both I and @Ebola16 consider this ready to merge. |
|
After messing around for a bit, I'm very happy with how it performs and I like it a lot more than the old behavior. |

https://bugs.dolphin-emu.org/issues/12029
We currently have one way of opening the menu on touch screen devices (swiping down from the top of the screen to bring up the action bar and selecting the menu in the action bar), and another way of opening the menu on Android TV (pressing Back). However, some devices that claim to support touch (or don't support leanback? Dolphin currently conflates the two) don't actually let you swipe down from the top of the screen in the way that Dolphin expects, notably Chromebooks. There are also some phones where you can swipe down from the top of the screen but this for some reason doesn't lead to the action bar becoming visible, though we are getting less reports about this nowadays than in the past.
This change makes us use the Back method on all devices, since it should work on all devices with no significant drawbacks.