Android: Add Log Configuration to UI

[Ebola16]

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

[mahdihijazi]

Please check my comment on #7121
I think it will impact this one.

[Ebola16]

Addressed @mahdihijazi 's concerns and updated the PR to include #7318. Lint has been giving me a lot of problems though. Any idea how to fix the lint issues? I ran ctrl+alt+L in android studio.

Is one of these settings incorrect?

[Ebola16]

Manually fixed the lint errors 😒
Ready for review

[leoetlino]

Not a big fan of the way the log types are completely duplicated… Now anyone who wants to add, remove or edit a log type has to update both the C++ code and the Android code, which is unnecessarily time-consuming and error-prone.

The log file configuration keys also shouldn't be duplicated.

Instead, you should interact with the LogManager to query and set log-related settings, log type names and descriptions, etc. I'd recommend looking at DolphinQt's LogConfigWidget.

[sepalani]

It's working as expected even with debug logs (I had to build my own APK with the CMake flag -DCMAKE_BUILD_TYPE=Debug, though). You should probably hide the "debug" verbosity based on the Common::Log::MAX_LOGLEVEL like Dolphin on PC.

[Ebola16]

@sepalani Let me know if you want me to squash the commits after review.

[sepalani]

It's a bit sad that you have to duplicate these arrays to handle this case. Though, I don't think there is much you can do without changing the way these SettingsItem classes work and how tied they are with Android resources which are read-only.

Otherwise, this change looks fine to me. I also don't mind having these as two commits as they make sense separated. If you feel they don't, feel free to squash them together.

[Ebola16]

I agree that the duplication is annoying but there are other cases in Dolphin that handle this issue in the same way I did. I'd rather change them all in a separate PR in the future.

[Ebola16]

Can we can get this merged too? It'll help with debugging issues.

[JosJuice]

I reviewed the C++ side of the PR.

[Ebola16]

@JosJuice I think I took care of the remaining unresolved comments but they should probably be checked again. I also took care of simple clang-tidy suggestions on the C++ side and squashed the related commits.

[JosJuice]

I would prefer if the clang-tidy fixes were applied in a different commit (or even in a different PR), since they don't have much to do with the other changes.

[Ebola16]

Comments addressed and Clang-Tidy changes moved to separate PR. I also added SettingsFragmentPresenter.LOG_TYPE_NAMES to hopefully handle LinkedHashMap duplication.

[JosJuice]

This should hopefully be the last review round.

[JosJuice]

Is this guaranteed to run after NativeLibrary.Initialize? I don't believe it will work correctly otherwise.

[Ebola16]

From my testing I haven't seen any problems here. Should I add a method that sets this LinkedHashMap and call it with AfterDirectoryInitializationRunner.run immediately after NativeLibrary.Initialize for an abundance of caution?

[Ebola16]

Please see the latest commit. Let's (hopefully) prevent a lot of problems with calling NativeLibrary by completing initialization before launching Settings and Emulation activities.

The last commit should also address this review comment.

[JosJuice]

I'm still not sure if there is any guarantee that things will execute in the right order. The information I can find is that static variables will be initialized when the class is loaded, but I can't find anything about when specifically classes actually are loaded.

If you are going to gate launching the settings activity behind directory initialization, please keep in mind that there already is a check for that here, and I would prefer only having one check for it for the settings (but perhaps your new location for it is better?):

dolphin/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/features/settings/ui/SettingsActivityPresenter.java

Line 85 in 63c53eb

private void prepareDolphinDirectoriesIfNeeded()

[Ebola16]

I put a breakpoint on NativeLibrary.GetLogTypeNames and indeed it was called when the class was loaded. The class is only (eventually) called through SettingsActivity.launch. I may be missing something but it seems this shouldn't cause any problems. I'll look into the second part of your response soon.

[Ebola16]

Reverted the last commit. We eventually call DirectoryInitialization.areDolphinDirectoriesReady() for both Settings and Emulation fragments. Also, SettingsActivity.launch adds a static context so it isn't worth changing the check to there.

Hasn't commented on the PR in months, and the problem is fixed as far as I can tell

[JosJuice]

I still don't really like relying on undocumented class loading behavior, but I suppose that the behavior isn't going to change without a point release of Android and accompanying compatibility change notes, so if the behavior is going to change in the future, we can deal with it when it happens. LGTM other than one comment.