Conversation
…ation, UI improvements (based on screensize) and implement SavedStateHandle
|
Important Review skippedToo many files! 128 files out of 278 files are above the max files limit of 150. You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi, a lot of work has clearly gone into this, but we'd need to discuss before doing any sort of UI/UX overhaul. Please message me on Discord and I can add you to the #development channel. In addition to that, there's already a chat going on about this. |
| val service = instance | ||
| if (service == null) { | ||
| Timber.e("Could not start QR logon: Service not initialized") | ||
| val event = SteamEvent.QrAuthEnded(success = false, message = "Service not initialized") | ||
| PluviaApp.events.emit(event) | ||
| return@withContext | ||
| } | ||
|
|
||
| service.steamClient?.let { steamClient -> | ||
| isWaitingForQRAuth = true | ||
|
|
||
| val authDetails = AuthSessionDetails().apply { | ||
| deviceFriendlyName = SteamUtils.getMachineName(instance!!) | ||
| deviceFriendlyName = SteamUtils.getMachineName(service) |
There was a problem hiding this comment.
So I'd suggest that if the service isn't running, we should start the service with a getter.
| hasError = true | ||
| } | ||
| } catch (e: Exception) { | ||
| hasError = true |
There was a problem hiding this comment.
Would recommend we also log out an error too in the catch if possible.
| PluviaTheme { | ||
| ConnectionStatusBanner( | ||
| connectionState = ConnectionState.CONNECTING, | ||
| connectionMessage = "Reconnecting to Steam...", |
There was a problem hiding this comment.
Mind to pop these into templated strings!
| A(InputIcons.Xbox.buttonColorA), | ||
| B(InputIcons.Xbox.buttonColorB), | ||
| X(InputIcons.Xbox.buttonColorX), | ||
| Y(InputIcons.Xbox.buttonColorY), | ||
| LB(InputIcons.Xbox.lb), | ||
| RB(InputIcons.Xbox.rb), | ||
| LT(InputIcons.Xbox.lt), | ||
| RT(InputIcons.Xbox.rt), | ||
| START(InputIcons.Xbox.start), | ||
| SELECT(InputIcons.Xbox.select), | ||
| DPAD(InputIcons.Xbox.dpad), | ||
| DPAD_UP(InputIcons.Xbox.dpadUp), | ||
| DPAD_DOWN(InputIcons.Xbox.dpadDown), | ||
| DPAD_LEFT(InputIcons.Xbox.dpadLeft), | ||
| DPAD_RIGHT(InputIcons.Xbox.dpadRight), |
There was a problem hiding this comment.
I think we're missing the Xbox Button (The one between the Select and Start) which may be helpful, such as bringing up the XServerScreen menu
Thoughts?
There was a problem hiding this comment.
I initially added this when I was testing with an xbox gamepad, then switched to testing on an Odin 3 and realized that we don’t have this button on most, if not all, android handhelds.
We could either show it conditionally or just not use it for consistency reasons. Thoughts?
There was a problem hiding this comment.
So I think as an iteration (not necessarily in this PR), users should be able to bind that button on their handheld so that they can use it.
Easier than them having to use the touch-screen for a more mature experience
There was a problem hiding this comment.
I assume the toString() isn't needed?
There was a problem hiding this comment.
From my understanding the custom toString() was redundant. Kotlin data class automatically generates a toString() method that outputs all properties in the exact same format. I assumed it was added by mistake, if it was intentional we should add it back. I didn't find any difference in behavior during testing. Could have been done for debugging purposes maybe?
| val graphicsDriver: String = Container.DEFAULT_GRAPHICS_DRIVER, | ||
| val graphicsDriverVersion: String = "", | ||
| val audioDriver: String = Container.DEFAULT_AUDIO_DRIVER, | ||
| ) { |
There was a problem hiding this comment.
So we've removed the state for the drivers and settings etc. I assume that's being moved?
There was a problem hiding this comment.
Yes, see XServerViewModel. From my understanding this is a better way of abstracting this data, feel free to correct me
| STEAM( | ||
| labelResId = R.string.tab_steam, | ||
| showCustom = false, | ||
| showSteam = true, | ||
| showGoG = false, | ||
| showEpic = false, | ||
| installedOnly = false, | ||
| ), |
There was a problem hiding this comment.
Now that GOG is in the code, we can add that in now
| val includeSteam = if (currentTab == app.gamenative.ui.enums.LibraryTab.ALL) { | ||
| _state.value.showSteamInLibrary | ||
| } else { | ||
| currentTab.showSteam | ||
| } | ||
| val includeOpen = if (currentTab == app.gamenative.ui.enums.LibraryTab.ALL) { | ||
| _state.value.showCustomGamesInLibrary | ||
| } else { | ||
| currentTab.showCustom | ||
| } |
There was a problem hiding this comment.
Will also need to add GOG here too
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryAppItem.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Will need to take into account GOG here, you can see the current code on how we get the URL (The whole URL is in the iconHash for GOG).
| * Apply immersive mode for a full-screen experience. | ||
| * Must be called in multiple lifecycle methods to ensure bars stay hidden. | ||
| */ | ||
| private fun applyImmersiveMode() { |
There was a problem hiding this comment.
We'll have to get some testing on a few different devices to make sure this functionality works here 👍
There was a problem hiding this comment.
This mostly feels like a hack, but the only way I could get it to consistently stay full screen. I have a hard time believing there isn’t a better way.
| } | ||
|
|
||
| // Show dialog when adding custom game folder | ||
| private val SHOW_ADD_CUSTOM_GAME_DIALOG = booleanPreferencesKey("show_add_custom_game_dialog") |
There was a problem hiding this comment.
Is this now handled elsewhere?
|
Overall looks really good. Mind that GOG integration is in the main branch so this'll need to take that into consideration. Happy to work with you on getting that working, just hit me up. Once you've dealt with conflicts, I'd love to pull this down and do some proper testing. |
| * | ||
| * TODO: reconsider this approach when merging GOG and Epic | ||
| */ |
There was a problem hiding this comment.
You've got a bit in here to reconsider how this would work with GOG and Epic.
Would you like to chat through this?
|
@utkarshdalal In order to allow coderabbitai through. We could have a separate PR to just merge in all the icons etc and just not use them yet. Then we can let it add its review to the follow-up. |
|
Some feedback as per my testing: Main Area: Settings: Controls: Overall this looks gorgeous, much better than we currently have and definitely in the right direction! Very happy to help contribute to this branch/PR to help it along, or provide any other feedback, but that's my thoughts so far! |
There was a problem hiding this comment.
I see the colours have changed... We should not do this, the icon, logo, website etc colours are all based off these. If this is needed, we should absolutely retain the primary and accent colours (0xFFA21CAF and 0xFF06B6D4)
There was a problem hiding this comment.
I see 0xFFA21CAF is retained, but the accent cyan isn't
|
Sounds good to me re CodeRabbit @phobos665 - we also have a lot of merge conflicts now |
|
I'm resolving conflicts |
|
Conflicts resolved, looks like we lost the ability to launch custom games |
|
Looks like we have to sign out from steam after this is merged otherwise it won't load the profile correctly? |
|
If there are no local games installed, we get stuck on the local tab. |
|
When a game page is opened in landscape, can't scroll down to see the details using only the gamepad |
WORK IN PROGRESS.
A handful of
TODO'shave been added that need to be discussed first before proceeding with the merge