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.

Sign up for GitHub

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

Jump to bottom

Wikigames: set initial currentState from Prefs #5354

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Wikigames: set initial currentState from Prefs #5354

wants to merge 16 commits into from

Conversation

cooltey
Copy link
Collaborator

@cooltey cooltey commented Feb 27, 2025 •
edited
Loading

What does this do?

This PR sets the current game state from the Prefs when the ViewModel is created.

Also moved the impression event to the end of onGameEnded() in the case of gameState being null.

https://phabricator.wikimedia.org/T387418

Williamrai
Williamrai reviewed Feb 27, 2025
View reviewed changes
Copy link
Collaborator

@Williamrai Williamrai left a comment

Choose a reason for hiding this comment

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

The changes looks good. I think with this change we can also remove this additional logic, since we are persisting the state.

else if (_gameState.value == null || ::currentState.isInitialized.not()) {
            "unknown"
        }

@Williamrai
Copy link
Collaborator

The changes looks good. I think with this change we can also remove this additional logic, since we are persisting the state.

else if (_gameState.value == null || ::currentState.isInitialized.not()) {
            "unknown"
        }

This has been resolved.

@cooltey cooltey requested a review from voyagerfan as a code owner February 28, 2025 20:44
@cooltey cooltey changed the title Use savedStateHandle for the events and currentState to prevent crashes Wikigames: Use savedStateHandle for the events and currentState to prevent crashes Feb 28, 2025
@cooltey cooltey added the Ready to merge PR passed design signoff and ready to be merged. label Mar 3, 2025
dbrant
dbrant requested changes Mar 4, 2025
View reviewed changes
app/src/main/java/org/wikipedia/games/onthisday/OnThisDayGameViewModel.kt Outdated
@@ -236,6 +247,8 @@ class OnThisDayGameViewModel(savedStateHandle: SavedStateHandle) : ViewModel() {
val langToState = totalState.langToState.toMutableMap()
langToState[wikiSite.languageCode] = currentState
Prefs.otdGameState = JsonUtil.encodeToString(TotalGameState(langToState)).orEmpty()
savedStateHandle[KEY_CURRENT_STATE] = JsonUtil.encodeToString(currentState)
Copy link
Member

Choose a reason for hiding this comment

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

The entire game state gets saved to Prefs (on the previous line). Do we really need to duplicate it into savedStateHandle?

@cooltey cooltey changed the title Wikigames: Use savedStateHandle for the events and currentState to prevent crashes Wikigames: set initial currentState from Prefs Mar 5, 2025
@cooltey cooltey requested review from Williamrai and dbrant March 5, 2025 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@voyagerfan voyagerfan Awaiting requested review from voyagerfan voyagerfan is a code owner

@Williamrai Williamrai Awaiting requested review from Williamrai Williamrai is a code owner

@dbrant dbrant Awaiting requested review from dbrant dbrant is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Ready to merge PR passed design signoff and ready to be merged.
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cooltey @Williamrai @dbrant