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

UI: Expose games build ID for cheat management #4340

Merged
merged 25 commits into from May 7, 2023

Conversation

gnisman
Copy link
Contributor

@gnisman gnisman commented Jan 24, 2023

Currently Ryujinx is not exposing Build ID to the user in the UI which make it hard to manage cheats

Screenshot 2023-02-03 at 23 32 46

Screenshot 2023-02-03 at 23 33 34

Closes #3397

@gdkchan gdkchan added enhancement New feature or request gui Related to Ryujinx.Ui labels Jan 24, 2023
@MetrosexualGarbodor
Copy link
Collaborator

I can't test right now, does this change whenever a game update is added or removed?

@gnisman
Copy link
Contributor Author

gnisman commented Jan 25, 2023

I can't test right now, does this change whenever a game update is added or removed?

Yes

@gnisman gnisman changed the title Ava UI: Expose games build ID for cheat management Ava / GDK UI: Expose games build ID for cheat management Jan 26, 2023
@gnisman gnisman changed the title Ava / GDK UI: Expose games build ID for cheat management UI: Expose games build ID for cheat management Jan 26, 2023
@gdkchan
Copy link
Member

gdkchan commented Jan 26, 2023

The way how the build ID is currently formatted (inside brackets after the game name) is the as the title ID, which can lead someone to assume this is the title ID rather than build ID. Maybe it would be better to have something explicitly say that this is the build ID.

@gnisman
Copy link
Contributor Author

gnisman commented Jan 26, 2023

The way how the build ID is currently formatted (inside brackets after the game name) is the as the title ID, which can lead someone to assume this is the title ID rather than build ID. Maybe it would be better to have something explicitly say that this is the build ID.

Wanted to change it to something like "Cheats Available for {0} [Build id: {1}]"
But CheatWindowHeading exists in 14 different languages in AVA, do we really want to change it?
In my opinion title id and build id looks completely different to mistake between them
but on the other hand users got used to see title id in this cheats window.
What do you suggest to do?

@OldManKain
Copy link

Don't mind my crappy editiing skills but would it be possible to add the BID's something like this?
image

@gnisman
Copy link
Contributor Author

gnisman commented Jan 29, 2023

Don't mind my crappy editiing skills but would it be possible to add the BID's something like this? image

It is possible but i dont think there is much use for the Tittle Id here, just adds bloat

@gnisman
Copy link
Contributor Author

gnisman commented Jan 29, 2023

@gdkchan , @OldManKain added BID as a prefix (updated the original screenshots) what do you think?

@jojodunet
Copy link

Personally, I think it’s better to see the both, TID and BID, on the same window.
Something like shown by @OldManKain

@AcK77
Copy link
Member

AcK77 commented Feb 3, 2023

IMO the BuildId should be extracted (and cached in the game json maybe?) when games are loaded in the gamelist, the method should be in Ryujinx.Ui.Common instead of ApplicationLoader and the BuildId should be shown in a readonly textbox to be able to copy/paste it.

@gnisman
Copy link
Contributor Author

gnisman commented Feb 3, 2023

IMO the BuildId should be extracted (and cached in the game json maybe?) when games are loaded in the gamelist, the method should be in Ryujinx.Ui.Common instead of ApplicationLoader and the BuildId should be shown in a readonly textbox to be able to copy/paste it.

Moved the method to Ryujinx.Ui.Common and Build id is shown as readonly Text Box now

about extracting build id when games are loaded in the game list, not sure it will be a good to extract build id of all the games ahead of time but rather only when needed in cheat window.
maybe we should address it as part of #3285

@gnisman
Copy link
Contributor Author

gnisman commented Feb 9, 2023

@AcK77 , @OldManKain do you think that the bid as a text box looks visually ok? (screenshots updated at the first comment)

@OldManKain
Copy link

@AcK77 , @OldManKain do you think that the bid as a text box looks visually ok? (screenshots updated at the first comment)

I can live with that, now it's up to the higher ups to make their decision.

@gnisman gnisman requested a review from TSRBerry April 21, 2023 10:53
@TSRBerry TSRBerry requested a review from a team April 21, 2023 18:50
@gnisman gnisman requested a review from TSRBerry April 21, 2023 20:21
Copy link
Member

@TSRBerry TSRBerry left a comment

Choose a reason for hiding this comment

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

Lgtm, thank you for being so patient with me!

@TSRBerry TSRBerry requested a review from a team April 21, 2023 22:46
@gnisman
Copy link
Contributor Author

gnisman commented Apr 23, 2023

Lgtm, thank you for being so patient with me!

Thanks for the review and the suggestions, appreciate it

@TSRBerry
Copy link
Member

Your latest commit is not correct unfortunately. Could you please drop this one and rebase your PR instead?

@gnisman
Copy link
Contributor Author

gnisman commented May 1, 2023

Your latest commit is not correct unfortunately. Could you please drop this one and rebase your PR instead?

Hey,
can you explain what is wrong with the merge? or you just prefer rebases instead of merges?

@TSRBerry
Copy link
Member

TSRBerry commented May 1, 2023

Oh sorry, it seems like it was correct after all and I just didn't read it properly. I thought you added your changes outside of the src directory.

src/Ryujinx.Ava/UI/Views/Main/MainMenuBarView.axaml.cs Outdated Show resolved Hide resolved
src/Ryujinx.Ava/UI/Windows/CheatWindow.axaml Outdated Show resolved Hide resolved
src/Ryujinx.Ava/UI/Windows/CheatWindow.axaml.cs Outdated Show resolved Hide resolved
src/Ryujinx.Ava/UI/Windows/CheatWindow.axaml.cs Outdated Show resolved Hide resolved
src/Ryujinx.Ui.Common/App/ApplicationData.cs Outdated Show resolved Hide resolved
src/Ryujinx/Ui/MainWindow.cs Outdated Show resolved Hide resolved
src/Ryujinx/Ui/Widgets/GameTableContextMenu.cs Outdated Show resolved Hide resolved
src/Ryujinx/Ui/Windows/CheatWindow.cs Outdated Show resolved Hide resolved
src/Ryujinx/Ui/Windows/CheatWindow.cs Outdated Show resolved Hide resolved
@gnisman gnisman requested a review from AcK77 May 5, 2023 21:22
Copy link
Member

@AcK77 AcK77 left a comment

Choose a reason for hiding this comment

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

Some last things and it should be good ;)! Thanks!

src/Ryujinx.Ava/UI/Windows/CheatWindow.axaml Outdated Show resolved Hide resolved
src/Ryujinx.Ava/UI/Windows/CheatWindow.axaml.cs Outdated Show resolved Hide resolved
src/Ryujinx.Ava/UI/Windows/CheatWindow.axaml.cs Outdated Show resolved Hide resolved
@gnisman
Copy link
Contributor Author

gnisman commented May 6, 2023

Some last things and it should be good ;)! Thanks!

Done, Thanks for the feedback

@gnisman gnisman requested a review from AcK77 May 6, 2023 07:40
@AcK77 AcK77 merged commit dde208b into Ryujinx:master May 7, 2023
6 checks passed
@gnisman gnisman deleted the expose-games-build-id branch May 7, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request gui Related to Ryujinx.Ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Expose games' build ID on the UI or have an option to copy them for cheat management
8 participants