-
Notifications
You must be signed in to change notification settings - Fork 83
feat: UI/UX Overhaul #395
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
base: master
Are you sure you want to change the base?
feat: UI/UX Overhaul #395
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the toString() isn't needed?
| val graphicsDriver: String = Container.DEFAULT_GRAPHICS_DRIVER, | ||
| val graphicsDriverVersion: String = "", | ||
| val audioDriver: String = Container.DEFAULT_AUDIO_DRIVER, | ||
| ) { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will also need to add GOG here too
| * This is the main entry point for displaying library items. It delegates to: | ||
| * - [ListViewCard] for list view (PaneType.LIST) | ||
| * - [GridViewCard] for grid views (PaneType.GRID_HERO, PaneType.GRID_CAPSULE) |
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.
I agree with this. LibraryItem should mostly be quite dumb.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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