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

Download refactor separate callback #1545

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 58 additions & 79 deletions src/components/views/DownloadModModal.vue
Original file line number Diff line number Diff line change
Expand Up @@ -91,23 +91,23 @@
<script lang="ts">

import { Component, Vue, Watch } from 'vue-property-decorator';
import ThunderstoreMod from '../../model/ThunderstoreMod';
import StatusEnum from '../../model/enums/StatusEnum';
import R2Error from '../../model/errors/R2Error';
import Game from '../../model/game/Game';
import ManifestV2 from '../../model/ManifestV2';
import Profile from '../../model/Profile';
import ThunderstoreCombo from '../../model/ThunderstoreCombo';
import ThunderstoreMod from '../../model/ThunderstoreMod';
import ThunderstoreVersion from '../../model/ThunderstoreVersion';
import ConflictManagementProvider from '../../providers/generic/installing/ConflictManagementProvider';
import ThunderstoreDownloaderProvider from '../../providers/ror2/downloading/ThunderstoreDownloaderProvider';
import R2Error from '../../model/errors/R2Error';
import StatusEnum from '../../model/enums/StatusEnum';
import ThunderstoreCombo from '../../model/ThunderstoreCombo';
import ProfileInstallerProvider from '../../providers/ror2/installing/ProfileInstallerProvider';
import { MOD_LOADER_VARIANTS } from '../../r2mm/installing/profile_installers/ModLoaderVariantRecord';
import * as PackageDb from '../../r2mm/manager/PackageDexieStore';
import ProfileModList from '../../r2mm/mods/ProfileModList';
import Profile from '../../model/Profile';
import * as ModDownloadUtils from "../../utils/ModDownloadUtils";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: all the other imports here use single quotes.

import { Progress } from '../all';
import Game from '../../model/game/Game';
import ConflictManagementProvider from '../../providers/generic/installing/ConflictManagementProvider';
import { MOD_LOADER_VARIANTS } from '../../r2mm/installing/profile_installers/ModLoaderVariantRecord';
import ModalCard from '../ModalCard.vue';
import * as PackageDb from '../../r2mm/manager/PackageDexieStore';
import { installModsToProfile } from '../../utils/ProfileUtils';

interface DownloadProgress {
assignId: number;
Expand Down Expand Up @@ -149,6 +149,14 @@ let assignId = 0;
return settings.getContext().global.ignoreCache;
}

setDownloadObject(downloadObject: DownloadProgress | null) {
this.downloadObject = downloadObject;
}

setDownloadingMod(downloadingMod: boolean) {
this.downloadingMod = downloadingMod;
}

public static async downloadSpecific(
profile: Profile,
combo: ThunderstoreCombo,
Expand Down Expand Up @@ -298,32 +306,24 @@ let assignId = 0;
this.downloadObject = progressObject;
DownloadModModal.allVersions.push([currentAssignId, this.downloadObject]);
this.downloadingMod = true;
ThunderstoreDownloaderProvider.instance.downloadLatestOfAll(modsWithUpdates, this.ignoreCache, (progress: number, modName: string, status: number, err: R2Error | null) => {
const assignIndex = DownloadModModal.allVersions.findIndex(([number, val]) => number === currentAssignId);
if (status === StatusEnum.FAILURE) {
if (err !== null) {
this.downloadingMod = false;
const existing = DownloadModModal.allVersions[assignIndex]
existing[1].failed = true;
this.$set(DownloadModModal.allVersions, assignIndex, [currentAssignId, existing[1]]);
DownloadModModal.addSolutionsToError(err);
this.$store.commit('error/handleError', err);
return;
}
} else if (status === StatusEnum.PENDING) {
const obj = {
progress: progress,
modName: modName,
initialMods: modsWithUpdates.map(value => `${value.getMod().getName()} (${value.getVersion().getVersionNumber().toString()})`),
assignId: currentAssignId,
failed: false,
}
if (this.downloadObject!.assignId === currentAssignId) {
this.downloadObject = Object.assign({}, obj);
}
this.$set(DownloadModModal.allVersions, assignIndex, [currentAssignId, obj]);
}
}, this.downloadCompletedCallback);
ThunderstoreDownloaderProvider.instance.downloadLatestOfAll(
modsWithUpdates,
this.ignoreCache,
(progress, modName, status, err) => { ModDownloadUtils.downloadProgressCallback(
currentAssignId,
progress,
modName,
status,
err,
modsWithUpdates.map(value => `${value.getMod().getName()} (${value.getVersion().getVersionNumber().toString()})`),
this.downloadObject,
this.setDownloadObject,
this.setDownloadingMod,
(assignIndex: string | number, value: any) => this.$set(DownloadModModal.allVersions, assignIndex, [currentAssignId, value]),
DownloadModModal.allVersions
)},
this.downloadCompletedCallback
);
}

downloadHandler(tsMod: ThunderstoreMod, tsVersion: ThunderstoreVersion) {
Expand All @@ -340,53 +340,32 @@ let assignId = 0;
DownloadModModal.allVersions.push([currentAssignId, this.downloadObject]);
this.downloadingMod = true;
setTimeout(() => {
ThunderstoreDownloaderProvider.instance.download(this.profile.asImmutableProfile(), tsMod, tsVersion, this.ignoreCache, (progress: number, modName: string, status: number, err: R2Error | null) => {
const assignIndex = DownloadModModal.allVersions.findIndex(([number, val]) => number === currentAssignId);
if (status === StatusEnum.FAILURE) {
if (err !== null) {
this.downloadingMod = false;
const existing = DownloadModModal.allVersions[assignIndex]
existing[1].failed = true;
this.$set(DownloadModModal.allVersions, assignIndex, [currentAssignId, existing[1]]);
DownloadModModal.addSolutionsToError(err);
this.$store.commit('error/handleError', err);
return;
}
} else if (status === StatusEnum.PENDING) {
const obj = {
progress: progress,
initialMods: [`${tsMod.getName()} (${tsVersion.getVersionNumber().toString()})`],
modName: modName,
assignId: currentAssignId,
failed: false,
}
if (this.downloadObject!.assignId === currentAssignId) {
this.downloadObject = Object.assign({}, obj);
}
this.$set(DownloadModModal.allVersions, assignIndex, [currentAssignId, obj]);
}
}, this.downloadCompletedCallback);
ThunderstoreDownloaderProvider.instance.download(
this.profile.asImmutableProfile(),
tsMod,
tsVersion,
this.ignoreCache,
(progress, modName, status, err) => { ModDownloadUtils.downloadProgressCallback(
currentAssignId,
progress,
modName,
status,
err,
[`${tsMod.getName()} (${tsVersion.getVersionNumber().toString()})`],
this.downloadObject,
this.setDownloadObject,
this.setDownloadingMod,
(assignIndex: string | number, value: any) => this.$set(DownloadModModal.allVersions, assignIndex, [currentAssignId, value]),
DownloadModModal.allVersions
) },
this.downloadCompletedCallback
);
}, 1);
}

async downloadCompletedCallback(downloadedMods: ThunderstoreCombo[]) {
ProfileModList.requestLock(async () => {
const profile = this.profile.asImmutableProfile();

try {
const modList = await installModsToProfile(downloadedMods, profile);
await this.$store.dispatch('profile/updateModList', modList);

const err = await ConflictManagementProvider.instance.resolveConflicts(modList, this.profile);
if (err instanceof R2Error) {
throw err;
}
} catch (e) {
this.$store.commit('error/handleError', R2Error.fromThrownValue(e));
} finally {
this.downloadingMod = false;
}
});
await ModDownloadUtils.downloadCompletedCallback(this.profile, downloadedMods, this.$store);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's currently possible for this to throw an unhandled exception and the next line never gets executed. (Granted, it was possible earlier too.)

this.downloadingMod = false;
}

static async installModAfterDownload(profile: Profile, mod: ThunderstoreMod, version: ThunderstoreVersion): Promise<R2Error | void> {
Expand Down
89 changes: 89 additions & 0 deletions src/utils/ModDownloadUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import { Store } from "vuex";

import StatusEnum from "../model/enums/StatusEnum";
import R2Error from "../model/errors/R2Error";
import Profile from "../model/Profile";
import ThunderstoreCombo from "../model/ThunderstoreCombo";
import ConflictManagementProvider from "../providers/generic/installing/ConflictManagementProvider";
import ProfileModList from "../r2mm/mods/ProfileModList";
import { installModsToProfile } from "../utils/ProfileUtils";

interface DownloadProgress {
assignId: number;
initialMods: string[];
modName: string;
progress: number;
failed: boolean;
}

export async function downloadCompletedCallback(profile: Profile, downloadedMods: ThunderstoreCombo[], store: Store<any>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • I think calling this downloadCompletedCallback is not ideal, as it semantically binds it to the call site. Ideally a utility function would be a standalone thing. E.g. after your refactorings this might not be called as a callback anymore. If you end up doing it this way, I'd think about what this function actually does and rename it to describe that.
  • Also this function also deals with stuff that happens after the download is completed, so "download utils" is not the location I would place it into
  • Showing the error modal, and thus the try-catch is IMO the UI components responsibility. One of the points of these refactorings is preparing for TCLI to implement these methods. It can't trigger a UI stuff in this manner, it has to be handled by the component.
  • Same thing sort of applies to dispatching 'profile/updateModList', but to a lesser extend, as we can assume that's something the TCLI could implement internally. Generally speaking avoiding passing the Vuex store to an util function is probably a good idea, unless there's no way to work around it.
  • We want to get rid of Profile, so a new function like this should use ImmutableProfile as argument type.
  • Due to the above, I'm not sure this method is a very good refactoring candidate. It currently already calls different utility functions so there's not that much nitty-gritty business logic in the component. However, this is a bit of grey area, so if you want to proceed with this approach, take the comments above into consideration.

const immutableProfile = profile.asImmutableProfile();
await ProfileModList.requestLock(async () => {
try {
const modList = await installModsToProfile(downloadedMods, immutableProfile);
await store.dispatch('profile/updateModList', modList);

const err = await ConflictManagementProvider.instance.resolveConflicts(modList, profile);
if (err instanceof R2Error) {
throw err;
}
} catch (e) {
store.commit('error/handleError', R2Error.fromThrownValue(e));
}
});
}

export async function downloadProgressCallback (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having this as a separate method in the vue component makes sense, as it removed duplicated code. However, I'm not so sure having it as a utility function is a good idea. The fact that this takes 11 arguments which mostly deal with managing the component's internal state means that it's not really a standalone in anyway. It's so tightly coupled with the current component, I'd argue that having it in a separate file actually makes the code harder to read. Also The Big Picture stuff argues against moving this here.

currentAssignId: number,
progress: number,
modName: string,
status: number,
err: R2Error | null,
initialMods: string[],
downloadObject: DownloadProgress | null,
setDownloadObject: Function,
setDownloadingMod: Function,
vueSetCallback: Function,
allVersions: [number, DownloadProgress][]
) {
const assignIndex = allVersions.findIndex(([number, val]) => number === currentAssignId);
if (status === StatusEnum.FAILURE) {
if (err !== null) {
setDownloadingMod(false);
const existing = allVersions[assignIndex]
existing[1].failed = true;
vueSetCallback(assignIndex, existing[1]);
addSolutionsToError(err);
throw(err);
}
} else if (status === StatusEnum.PENDING) {
const obj = {
progress: progress,
initialMods: initialMods,
modName: modName,
assignId: currentAssignId,
failed: false,
}
if (downloadObject!.assignId === currentAssignId) {
setDownloadObject(Object.assign({}, obj));
}
vueSetCallback(assignIndex, obj);
}
}
function addSolutionsToError(err: R2Error): void {
// Sanity check typing.
if (!(err instanceof R2Error)) {
return;
}

if (
err.name.includes("Failed to download mod") ||
err.name.includes("System.Net.WebException")
) {
err.solution = "Try toggling the preferred Thunderstore CDN in the settings";
}

if (err.message.includes("System.IO.PathTooLongException")) {
err.solution = 'Using "Change data folder" option in the settings to select a shorter path might solve the issue';
}
}
Loading