Refactor GameSelectionScreen.created() #849
Conversation
src/migrations/FolderMigration.ts
Outdated
| const fs = FsProvider.instance; | ||
| const rorPath = path.join(PathResolver.ROOT, "RiskOfRain2"); | ||
| await CacheUtil.clean(); |
There was a problem hiding this comment.
I was wondering why is cache clean run as a part of the migration? Not that it matters that much, I guess.
There was a problem hiding this comment.
Originally it copy/pasted the directory, however there were reports that it was failing silently in some cases causing loss of profiles.
It got reworked to rename instead and that was just leftover.
Doesn't hurt to leave in, however no preference if you want to remove it or not
There was a problem hiding this comment.
I guess I'll leave it in just in case, to avoid any regressions.
There was a problem hiding this comment.
So I ended up removing it anyway, because I noticed that it caused unhandled exceptions. Reading the profile folder fails if the migrations are not run yet, because it makes wrong assumptions about the directory structure. I'm quite sure that this is not caused by my changes, but was a pre-existing condition.
|
|
||
| await fs.rename(path.join(PathResolver.ROOT, "mods"), rorPath); | ||
| console.log("Directory migration done"); |
There was a problem hiding this comment.
What's the policy with console.logging in this project? My initial reaction was to clean them out. Then I thought they might help users to debug stuff. But then again, they don't have access to the console in production, do they?
There was a problem hiding this comment.
Logging is accessible during production. Ctrl + Shift + I for r2modman, and accessible via logs in TMM.
Generally the rule I've mostly followed is that if a user can't see it, we should still be able to track progress via logs.
src/pages/GameSelectionScreen.vue
Outdated
| }) | ||
|
|
||
| try { | ||
| await FolderMigration.runMigration(); |
There was a problem hiding this comment.
I considered moving the migration to e.g. App.vue so it wouldn't get checked every time this view mounts (which can happen multiple times now that the app doesn't restart upon game change). But this.runningMigration is now used to prevent game selection when the migration is running, and handling that might become complicated if the migration is run in the App. Any thoughts where this should be called (or if it's still needed at all)?
There was a problem hiding this comment.
Could belong to Vuex? If not yet ran, run and set state.
There was a problem hiding this comment.
I'll look into it 👍🏻
There was a problem hiding this comment.
This seems to work nicely in the main Vuex store.
src/r2mm/manager/ManagerSettings.ts
Outdated
| /** | ||
| * Return default game selection needed to skip the game selection screen. | ||
| */ | ||
| public static async getDefaults(settings?: ManagerSettings) { |
There was a problem hiding this comment.
Is it ok to import GameManager in ManagerSettings, or should the files be kept loosely coupled? I personally think the helper methods fits nicely here, but it could be moved to another file that imports both GameManager and ManagerSettings, I guess?
There was a problem hiding this comment.
I'd say no mostly because then we have a two-way binding on the files. It'd be nice if we could keep settings pretty independent of everything.
What does this do when there's no defaultGame?
There was a problem hiding this comment.
I'll see if there's a suitable helper file to move this into, and create one if there isn't.
If there's no default game, the returned defaultGame and defaultPlatform will be undefined (the latter can be undefined independently too). If either is undefined, the calling Vue component will not set this.selectedGame/this.selectedPlatform nor call this.proceed (i.e. works like it did before, I believe).
There was a problem hiding this comment.
Created ManagerUtils
|
Left some questions as comments. Unfortunately they're not visible here, but they can be seen in the "Files Changed" tab. |
Attempt to improve code quality, although most of the changes are subjective. Move the migration check into Vuex store so it doesn't get called multiple times now that the game selection screen can be accessed without restarting the app. The cache cleaning was removed, since it failed if the old directory structure was still used: it tried to read the profiles from a non- existing dir. Refs TS-1138
anttimaki
force-pushed
the
TS-1138-GameSelectionScreen-refactor
branch
from
2daa4af
to
e99a272
Compare
There was a problem hiding this comment.
Also something that came to my mind, did you test if setting a default game and then going back to the game selection screen works as expected? As far as I could tell the changes in this PR keep the overall logic the same, but I'd still check that just to be sure.
Yes and it does. |
src/utils/ManagerUtils.ts
Outdated
| const globals = settings.getContext().global; | ||
| const defaultGame = GameManager.findByFolderName(globals.defaultGame); | ||
| const defaultPlat = defaultGame?.storePlatformMetadata.find(x => x.storePlatform === globals.defaultStore); | ||
| return {defaultGame, defaultPlatform: defaultPlat?.storePlatform}; |
There was a problem hiding this comment.
Could you remove the optional chaining operator for now?
There was a problem hiding this comment.
Replaced two occurrences of optional chaining with good old ternary operator.
Extract any logic out of the component, only setting the attributes by calling helper methods. Clean up unnecessary code and attempt to simplify the implementation. Refs TS-1138
anttimaki
force-pushed
the
TS-1138-GameSelectionScreen-refactor
branch
from
e99a272
to
afb3957
Compare
No description provided.