Duplicate active game and settings instance in Vuex #1230
Conversation
There was a problem hiding this comment.
Looks good to me, but again going to question the need for having all of this functionality in the same VueX modules. In my opinion there are distinctly different functionalities being mixed together right now that have no reason to be as highly coupled as they are.
Anyway, nothing I'd consider a blocker for merging
| static get defaultGame(): Game { | ||
| return this._gameList.find(value => value.internalFolderName === "RiskOfRain2")!; | ||
| } |
There was a problem hiding this comment.
Why not just hardcode the pointer to this game instance? Why do we need to enumerate any lists? We know the exact value this should point to at build time, there's no need to leave room for uncertainty
There was a problem hiding this comment.
I'm not entirely sure why this has been implemented like this earlier. Took a quick peek now at the git history but that didn't provide much information. I can change the implementation if we think that's safe to do, but that'll have to wait until after my vacation.
There was a problem hiding this comment.
I'm fairly sure it was done this way because of uncertainty towards moving to a cloud based game list. If the game were to be removed then we'd return undefined rather than erroring on the .internalFolderName because _gameList.RiskOfRain2 wouldn't exist in the result
| const getDefaultGame = () => { | ||
| const defaultGame = GameManager.unsetGame(); | ||
|
|
||
| // Don't trust the non-null asserted typing of .unsetGame(). | ||
| if (defaultGame === undefined) { | ||
| throw new Error("GameManager.unsetGame() returned undefined"); | ||
| // Don't trust the non-null asserted typing of GameManager.defaultGame. | ||
| if (GameManager.defaultGame === undefined) { | ||
| throw new Error("GameManager.defaultGame returned undefined"); | ||
| } | ||
|
|
||
| return defaultGame; | ||
| return GameManager.defaultGame; | ||
| }; |
There was a problem hiding this comment.
And as there's no need for uncertainty, this entire snippet can be removed
anttimaki
force-pushed
the
move-local-mod-list-store
branch
from
8fa0b21 to
25d0c82
Compare
The old name was a bit confusing since it could be read as a method that resets the active game stored in GameManager's internal state.
Set the game when user proceeds (or is automatically redirected) from game selection screen. Update the settings whenever game is changed properly via action. To ensure the settings are properly loaded, they should be accessed via getter rather than directly from the state. This is not ideal but we're constrained by the fact that actions are the only part of Vuex that supports async operations and provides access to both local and root state/getters/mutations/actions. The change means that the GameManager is no longer the single source of truth in the matter. This is a trade-off with eventually being able to do more business logic in Vuex store instead of the UI components.
SearchAndSort now accesses only Vuex, which handles updating the local state and the persistent settings storage.
- If profile/loadOrderingSettings is called in SearchAndSort, its sibling component LocalModList has already tried to access related settings - Move resetting mod list when changing between profiles from Profiles component to Manager, since that seems to work too and feels like a more correct place for it - Move modFilters/reset call from Manager's created to beforeCreate. The location doesn't really matter since the components that use the related settings aren't mounted initially, but for consistency's sake it's called in the same place as the rest
anttimaki
force-pushed
the
vuex-active-game
branch
from
4e1521c to
b17d878
Compare
To clarify, are you referring to having the active game and settings in the root store? Or the new dispatchers in profile module? In either case, what would you suggest as a sensible structure? |
| static get defaultGame(): Game { | ||
| return this._gameList.find(value => value.internalFolderName === "RiskOfRain2")!; | ||
| } |
There was a problem hiding this comment.
I'm fairly sure it was done this way because of uncertainty towards moving to a cloud based game list. If the game were to be removed then we'd return undefined rather than erroring on the .internalFolderName because _gameList.RiskOfRain2 wouldn't exist in the result
| // Reset the mod list to prevent the previous profile's list | ||
| // flashing on the screen while a new profile's list is loaded. |
There was a problem hiding this comment.
Does this actually solve it? The beforeCreate is async and I'm not sure if Vue even awaits for this to complete before rendering
There was a problem hiding this comment.
I don't know about the race condition either, but I think it's safe to say this should at least improve the situation if nothing else
There was a problem hiding this comment.
I think ebkr might be correct and keeping the mod list resetting in Profiles.vue might be safer to avoid a race condition. I made the move here mostly because having Profiles.vue manage state on behalf of the Manager.vue is not ideal, but obviously less-than-ideal code that works is better. I'll look into this a bit more.
There was a problem hiding this comment.
I'm under the assumption that everything that relies on this would get re-rendered if it changes regardless if there's a race condition or not, so it wouldn't be a problem?
There was a problem hiding this comment.
I tested this by making the Vuex dispatch function sleep a bit if an empty array of mods was passed to it. As a result, when called in beforeCreate, it rendered the proper mod list first, and then once the sleep finished, rendered the view with empty mod list. So a totally artificial test and quite unlikely that earlier call with empty array would complete later than a later call with non-empty array. But theoretically possible since the first call isn't awaited, methinks?
No description provided.