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: Dynamic SettingsActivity Titles #7274

Merged
merged 1 commit into from
Aug 21, 2019
Merged

Android: Dynamic SettingsActivity Titles #7274

merged 1 commit into from
Aug 21, 2019

Conversation

Ebola16
Copy link
Member

@Ebola16 Ebola16 commented Jul 17, 2018

Implements https://bugs.dolphin-emu.org/issues/11012.

The old code always displayed "Settings". This PR allows different titles, which makes identifying the current menu easier for users. Example:

screenshot_20180716-225508_dolphin emulator

@BhaaLseN
Copy link
Member

"Dynamic" feels a bit like a stretch, since it can actually only be one of the 3 (or 4 if you count "Settings"). What are your thoughts on passing it into launch instead of deriving it from the menu tag?

@Ebola16
Copy link
Member Author

Ebola16 commented Jul 17, 2018

I think "Dynamic" is accurate since the title changes based upon the menu tag.

Would passing to launch have any advantages over deriving from menu tags? There may be cases in the future where we want to change titles outside of launch so I think menu tags are more versatile.

@BhaaLseN
Copy link
Member

Was just a thought, mainly in leu of the open/closed principle where you wouldn't have to change that widget just for the name. But since there is other places where the menu tag is tied in, it doesn't really matter.
Feel free to ignore that comment.

@Ebola16 Ebola16 force-pushed the DSA branch 3 times, most recently from 00e0839 to da33e3e Compare July 31, 2018 20:22
@Ebola16
Copy link
Member Author

Ebola16 commented Jul 31, 2018 •
edited
Loading

@mahdihijazi and @riking
I took your advice into account and updated this PR. Is this better? I also removed an unused import statement.

Also, there is a bug when backing out of Graphics Settings -> Enhancements -> Stereoscopy. It backs out to the Enhancements submenu but the title displays "Graphics Settings" for some reason.

@Ebola16 Ebola16 force-pushed the DSA branch 2 times, most recently from 9d194c4 to aa6f9ad Compare July 31, 2018 21:19
riking
riking approved these changes Aug 1, 2018
View reviewed changes
Copy link
Contributor

@riking riking left a comment

Choose a reason for hiding this comment

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

Looks better :)

riking
riking reviewed Aug 1, 2018
View reviewed changes
Source/Android/app/src/main/res/values/strings.xml
@@ -38,6 +38,15 @@
<string name="wiimote_tilt">Tilt</string>
<string name="wiimote_shake">Shake</string>

<!-- WARNING Do not move these controller entries AT ALL COSTS! They are indexed with ints, and an assumption
is made that they are placed together so that we can access them sequentially in a loop.
Wiimotes start at 4 since they are mapped to padID's 4-7 in the native code.-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Accidentally included?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I remove this for the Wii Remote Extensions? There were no string entries for them.

@Ebola16 Ebola16 force-pushed the DSA branch 2 times, most recently from ce26bbd to 087a97d Compare August 21, 2018 14:17
@Ebola16 Ebola16 force-pushed the DSA branch 4 times, most recently from cf82842 to 4cd95ca Compare August 30, 2018 05:05
@Ebola16
Copy link
Member Author

Ebola16 commented Aug 30, 2018

I think lint is incorrect.

@zackhow
Copy link
Member

zackhow commented Aug 30, 2018 •
edited
Loading

It is correct, just load the new code-style-java.xml file in AS. That way you can just hit cmd+opt+L(ctrl+alt+L on windows, I think) and be g2g.

@Ebola16
Copy link
Member Author

Ebola16 commented Aug 30, 2018 •
edited
Loading

Still getting lint conflicts post ctrl+alt+L. This was rebased to master before pushing.

@zackhow
Copy link
Member

zackhow commented Aug 30, 2018

Looks like you are still using the old style. Open settings -> editor -> code style. Next to scheme there is a drop down, select import and choose the new code-style-java.xml

@Ebola16 Ebola16 force-pushed the DSA branch 2 times, most recently from 3557622 to 87eba7d Compare August 31, 2018 20:56
@Ebola16
Copy link
Member Author

Ebola16 commented Aug 31, 2018

captures

Am I still using the wrong file? Or is the buildbot not using the right XML for lint?

@riking
Copy link
Contributor

riking commented Sep 1, 2018

@Ebola16 If you go into Code Style > Java, if it imported right it should be set to using 2-space indents.

@Ebola16
Copy link
Member Author

Ebola16 commented Sep 1, 2018

capture3
It seems that is set properly.

@riking
Copy link
Contributor

riking commented Sep 1, 2018

Check that the "Dolphin-Java" code style is actually the one being used in your Project/Module Settings. The .iml files aren't committed to version control.

@Ebola16
Copy link
Member Author

Ebola16 commented Sep 1, 2018 •
edited
Loading

I checked settings and project structure but I didn't notice anything abnormal relating to code style. I also completely re-cloned the repo but it didn't help.

@Ebola16 Ebola16 force-pushed the DSA branch 3 times, most recently from 350b49a to 66287d1 Compare September 28, 2018 05:28
@Ebola16 Ebola16 force-pushed the DSA branch 2 times, most recently from 44ac18e to ce2fd2f Compare October 6, 2018 02:27
@zackhow
Copy link
Member

zackhow commented Oct 6, 2018

LGTM

@Ebola16
Copy link
Member Author

Ebola16 commented Oct 6, 2018

I manually fixed the lint errors. However, there is a bug when backing out of Graphics Settings -> Enhancements -> Stereoscopy. It backs out to the Enhancements submenu but the title displays "Graphics Settings". Any ideas on how to fix this?

@zackhow
Copy link
Member

zackhow commented Oct 6, 2018

It's due to getting the menuTag from the initial launch intent, so it will always be the menu tag passed to launch(). Completely missed that, yeah, looking at this a bit more this should be reworked.

@leoetlino
Copy link
Member

What's the status of this PR?

@Ebola16 Ebola16 force-pushed the DSA branch 3 times, most recently from 9f8152d to a5c1b64 Compare March 15, 2019 05:45
@Ebola16
Copy link
Member Author

Ebola16 commented Mar 15, 2019

I redid this PR and the issues seem to be fixed now. Ready for review!

@Ebola16 Ebola16 mentioned this pull request Mar 22, 2019
Merged
leoetlino
leoetlino reviewed May 10, 2019
View reviewed changes
Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

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

This looks fine to me. @mahdihijazi or @zackhow, do you have any comments on this?

@Ebola16
Copy link
Member Author

Ebola16 commented Jul 15, 2019

Rebased for updated clang rules

@Ebola16
Copy link
Member Author

Ebola16 commented Aug 8, 2019

...any other reviews?

@Helios747
Copy link
Contributor

sure

@Helios747 Helios747 merged commit 55d9f89 into dolphin-emu:master Aug 21, 2019
@Ebola16 Ebola16 deleted the DSA branch August 21, 2019 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leoetlino leoetlino leoetlino left review comments

@mahdihijazi mahdihijazi mahdihijazi requested changes

@riking riking riking approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Ebola16 @BhaaLseN @zackhow @riking @leoetlino @Helios747 @mahdihijazi