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: Convert Settings to Kotlin #11662

Merged
merged 52 commits into from Mar 19, 2023
Merged

Conversation

t895
Copy link
Contributor

@t895 t895 commented Mar 15, 2023

Nothing functionally changed here, just swapping to Kotlin.

Apologies in advance for such a large PR. This isn't really something you want to do in smaller chunks because it leads to a lot of annoying clean up later.

@JosJuice
Copy link
Member

The enum class listings are really long after translation to Kotlin. Do they have to be formatted like that?

@t895
Copy link
Contributor Author

t895 commented Mar 15, 2023

If you try to put them in one line and then use the built in formatting, then only the ones that reach the character limit will be formatted like that. That felt inconsistent so I had them all follow the same pattern.

But no they don't have to.

@JosJuice
Copy link
Member

Something we probably should do sooner or later is replace stuff like Settings.FILE_DOLPHIN and Settings.SECTION_INI_CORE with direct string literals, so the declarations take up less space and are easier to read. (This more or less matches what we do in C++ too.) I'm fine with doing that either before or after this PR, but either way, I think I would prefer to put stuff on one line where it is possible.

Also, it seems like SettingsFragmentPresenter has the same problem of setting declarations being overly long. I guess there isn't as much we can do here since almost all of them are longer than one line?

@t895
Copy link
Contributor Author

t895 commented Mar 15, 2023

Something we probably should do sooner or later is replace stuff like Settings.FILE_DOLPHIN and Settings.SECTION_INI_CORE with direct string literals, so the declarations take up less space and are easier to read. (This more or less matches what we do in C++ too.) I'm fine with doing that either before or after this PR, but either way, I think I would prefer to put stuff on one line where it is possible.

I'm not sure what you mean. Like replacing instances of Settings.SECTION_INI_CORE with the string it represents? ("Core" in this case)

Also, it seems like SettingsFragmentPresenter has the same problem of setting declarations being overly long. I guess there isn't as much we can do here since almost all of them are longer than one line?

I'll at least check, but in those cases, yes.

@JosJuice
Copy link
Member

I'm not sure what you mean. Like replacing instances of Settings.SECTION_INI_CORE with the string it represents? ("Core" in this case)

Yes, exactly. Sorry if it was unclear.

@t895
Copy link
Contributor Author

t895 commented Mar 15, 2023

You're fine, I'm just a little dull.

I guess we could do that? It just feels less intuitive to me. I'll push that off to another PR, though.

@t895
Copy link
Contributor Author

t895 commented Mar 15, 2023

Reformatted the settings types and the presenter. As expected I couldn't do much about the presenter but a lot were collapsed in BooleanSetting

Copy link
Member

@JosJuice JosJuice left a comment

Choose a reason for hiding this comment

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

I'm really not a fan of how the already long SettingsFragmentPresenter is now even longer. But if we're converting everything to Kotlin, I guess that's how it has to be.

I'll merge this after a day or so, in case someone else wants to offer any comments.

@mbc07
Copy link
Contributor

mbc07 commented Mar 17, 2023

It would be nice to get this merged, as it's a blocker (kinda) for the work I'm doing in PR #11600...

@JosJuice JosJuice merged commit 002a96a into dolphin-emu:master Mar 19, 2023
14 checks passed
@t895 t895 deleted the kotlin-settings branch March 21, 2023 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants