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

Clean up the download state management Vuex store and it's usages #1596

Open
wants to merge 1 commit into
base: download-callback-refactor
Choose a base branch
from
+65 −93
Open

Clean up the download state management Vuex store and it's usages #1596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
101 changes: 33 additions & 68 deletions src/components/views/DownloadModModal.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
<template>
<div>
<div id='downloadProgressModal' :class="['modal', {'is-active':downloadingMod}]" v-if="$store.state.download.downloadObject !== null">
<div id='downloadProgressModal' :class="['modal', {'is-active':downloadingMod}]" v-if="$store.getters['download/currentDownload'] !== null">
<div class="modal-background" @click="downloadingMod = false;"></div>
<div class='modal-content'>
<div class='notification is-info'>
<h3 class='title'>Downloading {{$store.state.download.downloadObject.modName}}</h3>
<p>{{Math.floor($store.state.download.downloadObject.progress)}}% complete</p>
<h3 class='title'>Downloading {{$store.getters['download/currentDownload'].modName}}</h3>
<p>{{Math.floor($store.getters['download/currentDownload'].progress)}}% complete</p>
<Progress
:max='100'
:value='$store.state.download.downloadObject.progress'
:value="$store.getters['download/currentDownload'].progress"
:className="['is-dark']"
/>
</div>
Expand Down Expand Up @@ -60,14 +60,7 @@ import ConflictManagementProvider from '../../providers/generic/installing/Confl
import ProfileInstallerProvider from '../../providers/ror2/installing/ProfileInstallerProvider';
import ThunderstoreDownloaderProvider from '../../providers/ror2/downloading/ThunderstoreDownloaderProvider';
import ProfileModList from '../../r2mm/mods/ProfileModList';

interface DownloadProgress {
assignId: number;
initialMods: string[];
modName: string;
progress: number;
failed: boolean;
}
import { DownloadProgress } from "../../store/modules/DownloadModule";

@Component({
components: {
Expand Down Expand Up @@ -95,8 +88,9 @@ interface DownloadProgress {
const tsMod = combo.getMod();
const tsVersion = combo.getVersion();

store.commit('download/increaseAssignId');
const currentAssignId = store.state.download.assignId;
const currentAssignId = store.getters["download/currentDownload"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor+: On a further thought, there seems to be this repeated pattern of "get id, create blank progress object, add it to store". In addition to repetition, the current implementation of getting the id for example unnecessarily places the logic into component, and leaks store's internal implementation details. I suggest we combine these to an action instead, i.e. something like:

        async addDownload(state: State, initialMods: string[]): Promise<number> {
            const assignId = state.allDownloads.length;
            const downloadObject: DownloadProgress = {
                assignId,
                initialMods,
                modName: '',
                progress: 0,
                failed: false,
            };
            state.allDownloads = [...state.allDownloads, downloadObject];
            return assignId;
        },

? store.getters["download/currentDownload"].assignId + 1
: 0;

const progressObject = {
progress: 0,
Expand All @@ -106,22 +100,17 @@ interface DownloadProgress {
failed: false,
};

store.commit('download/pushDownloadObjectToAllVersions', {
assignId: currentAssignId,
downloadObject: progressObject
});
store.commit('download/addDownload', progressObject);

setTimeout(() => {
ThunderstoreDownloaderProvider.instance.download(profile.asImmutableProfile(), tsMod, tsVersion, ignoreCache, (progress: number, modName: string, status: number, err: R2Error | null) => {
const assignIndex = store.state.download.allVersions.findIndex(([number, val]: [number, DownloadProgress]) => number === currentAssignId);
if (status === StatusEnum.FAILURE) {
if (err !== null) {
const existing = store.state.download.allVersions[assignIndex]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor+: Similar to addDownload, it seems we can take updateDownload even further. In the store:

interface UpdateObject {
    assignId: number;
    progress?: number;
    modName?: string;
    failed?: boolean;
}

updateDownload(state: State, update: UpdateObject) {
    const downloads = [...state.allDownloads];
    const index = downloads.findIndex((old) => old.assignId === update.assignId);
    if (index === -1) {
        throw new R2Error(...);
    }
    downloads[index] = {...downloads[index], ...update};
    state.allDownloads = downloads;
},

And then on the call sites:
this.$store.commit('download/updateDownload', {assignId, failed: true}); &
this.$store.commit('download/updateDownload', {assignId, progress, modName});

As a bonus we don't need to export DownloadProgress, as the UI component doesn't care about this internal implementation detail anymore. Nevermind, it's still needed by the DownloadMonitor.

existing[1].failed = true;
store.commit('download/updateDownloadObject', {
assignId: assignIndex,
downloadVersion: [currentAssignId, existing[1]]
const existing = store.state.download.allDownloads.find((dlObj: DownloadProgress) => {
return dlObj.assignId === currentAssignId;
});
existing.failed = true;
store.commit('download/updateDownload', existing);
DownloadModModal.addSolutionsToError(err);
return reject(err);
}
Expand All @@ -133,10 +122,7 @@ interface DownloadProgress {
assignId: currentAssignId,
failed: false,
}
store.commit('download/updateDownloadObject', {
assignId: assignIndex,
downloadVersion: [currentAssignId, obj]
});
store.commit('download/updateDownload', obj);
}
}, async (downloadedMods: ThunderstoreCombo[]) => {
ProfileModList.requestLock(async () => {
Expand Down Expand Up @@ -167,8 +153,9 @@ interface DownloadProgress {
this.closeModal();
const modsWithUpdates: ThunderstoreCombo[] = await this.$store.dispatch('profile/getCombosWithUpdates');

this.$store.commit('download/increaseAssignId');
const currentAssignId = this.$store.state.download.assignId;
const currentAssignId = this.$store.getters["download/currentDownload"]
? this.$store.getters["download/currentDownload"].assignId + 1
: 0;

const progressObject = {
progress: 0,
Expand All @@ -177,23 +164,17 @@ interface DownloadProgress {
assignId: currentAssignId,
failed: false,
};
this.$store.commit('download/setDownloadObject', progressObject);
this.$store.commit('download/pushDownloadObjectToAllVersions', {
assignId: currentAssignId,
downloadObject: progressObject
});
this.$store.commit('download/addDownload', progressObject);
this.downloadingMod = true;
ThunderstoreDownloaderProvider.instance.downloadLatestOfAll(modsWithUpdates, this.ignoreCache, (progress: number, modName: string, status: number, err: R2Error | null) => {
const assignIndex = this.$store.state.download.allVersions.findIndex(([number, val]: [number, DownloadProgress]) => number === currentAssignId);
if (status === StatusEnum.FAILURE) {
if (err !== null) {
this.downloadingMod = false;
const existing = this.$store.state.download.allVersions[assignIndex];
existing[1].failed = true;
this.$store.commit('download/updateDownloadObject', {
assignId: assignIndex,
downloadVersion: [currentAssignId, existing[1]]
const existing = this.$store.state.download.allDownloads.find((dlObj: DownloadProgress) => {
return dlObj.assignId === currentAssignId;
});
existing.failed = true;
this.$store.commit('download/updateDownload', existing);
DownloadModModal.addSolutionsToError(err);
this.$store.commit('error/handleError', err);
return;
Expand All @@ -206,13 +187,7 @@ interface DownloadProgress {
assignId: currentAssignId,
failed: false,
}
if (this.$store.state.download.downloadObject!.assignId === currentAssignId) {
this.$store.commit('download/setDownloadObject', obj);
}
this.$store.commit('download/updateDownloadObject', {
assignId: assignIndex,
downloadVersion: [currentAssignId, obj]
});
this.$store.commit('download/updateDownload', obj);
}
}, (downloadedMods) => {
this.downloadCompletedCallback(downloadedMods);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Critical: found a bug from previous PR: downloadCompletedCallback should be awaited on both call sites to prevent the modal closing too early.

Expand All @@ -223,8 +198,9 @@ interface DownloadProgress {
downloadHandler(tsMod: ThunderstoreMod, tsVersion: ThunderstoreVersion) {
this.closeModal();

this.$store.commit('download/increaseAssignId');
const currentAssignId = this.$store.state.download.assignId;
const currentAssignId = this.$store.getters["download/currentDownload"]
? this.$store.getters["download/currentDownload"].assignId + 1
: 0;

const progressObject = {
progress: 0,
Expand All @@ -233,24 +209,19 @@ interface DownloadProgress {
assignId: currentAssignId,
failed: false,
};
this.$store.commit('download/setDownloadObject', progressObject);
this.$store.commit('download/pushDownloadObjectToAllVersions', {
assignId: currentAssignId,
downloadObject: this.$store.state.download.downloadObject
});
this.$store.commit('download/addDownload', progressObject);

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 = this.$store.state.download.allVersions.findIndex(([number, val]: [number, DownloadProgress]) => number === currentAssignId);
if (status === StatusEnum.FAILURE) {
if (err !== null) {
this.downloadingMod = false;
const existing = this.$store.state.download.allVersions[assignIndex];
existing[1].failed = true;
this.$store.commit('download/updateDownloadObject', {
assignId: assignIndex,
downloadVersion: [currentAssignId, existing[1]]
const existing = this.$store.state.download.allDownloads.find((dlObj: DownloadProgress) => {
return dlObj.assignId === currentAssignId;
});
existing.failed = true;
this.$store.commit('download/updateDownload', existing);
DownloadModModal.addSolutionsToError(err);
this.$store.commit('error/handleError', err);
return;
Expand All @@ -263,13 +234,7 @@ interface DownloadProgress {
assignId: currentAssignId,
failed: false,
}
if (this.$store.state.download.downloadObject!.assignId === currentAssignId) {
this.$store.commit('download/setDownloadObject', obj);
}
this.$store.commit('download/updateDownloadObject', {
assignId: assignIndex,
downloadVersion: [currentAssignId, obj]
});
this.$store.commit('download/updateDownload', obj);
}
}, (downloadedMods) => {
this.downloadCompletedCallback(downloadedMods);
Expand Down
12 changes: 7 additions & 5 deletions src/pages/DownloadMonitor.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
</div>
</template>
<template v-else>
<div v-for="([assignId, downloadObject], index) of activeDownloads" :key="`download-progress-${index}`">
<div v-for="(downloadObject, index) of activeDownloads" :key="`download-progress-${index}`">
<div>
<div class="container margin-right">
<div class="border-at-bottom pad pad--sides">
Expand Down Expand Up @@ -42,10 +42,12 @@

<script lang="ts">

import Timeout = NodeJS.Timeout;
Copy link
Collaborator

Choose a reason for hiding this comment

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

An idea for a "next" PR:

  • Test if timeouts are no longer needed now that the state is managed in Vuex
  • Add a getter, e.g. "newestFirst" that returns all downloads, in reverse order
  • I think the store getter can be used directly in the template part, but if a separate get method is needed in the Vue component, name it something else than "activeDownloads" as the list clearly includes also the completed downloads
  • See if we can stop exporting DownloadProgress from the store after the above changes (it might be needed if we need the get method to satisfy TypeScript)

import { Component, Vue } from 'vue-property-decorator';

import { Hero } from '../components/all';
import Progress from '../components/Progress.vue';
import Timeout = NodeJS.Timeout;
import { DownloadProgress } from "../store/modules/DownloadModule";

@Component({
components: {
Expand All @@ -55,12 +57,12 @@ import Timeout = NodeJS.Timeout;
})
export default class DownloadMonitor extends Vue {
private refreshInterval!: Timeout;
private activeDownloads: [number, any][] = [];
private activeDownloads: DownloadProgress[] = [];

created() {
this.activeDownloads = [...this.$store.state.download.allVersions].reverse();
this.activeDownloads = [...this.$store.state.download.allDownloads].reverse();
this.refreshInterval = setInterval(() => {
this.activeDownloads = [...this.$store.state.download.allVersions].reverse();
this.activeDownloads = [...this.$store.state.download.allDownloads].reverse();
}, 100);
}

Expand Down
45 changes: 25 additions & 20 deletions src/store/modules/DownloadModule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { GetterTree } from "vuex";

import { State as RootState } from "../../store";
import R2Error from "../../model/errors/R2Error";

export interface DownloadProgress {
assignId: number;
Expand All @@ -11,9 +13,7 @@ export interface DownloadProgress {


interface State {
allVersions: [number, DownloadProgress][],
assignId: number,
downloadObject: DownloadProgress | null,
allDownloads: DownloadProgress[],
}

/**
Expand All @@ -23,34 +23,39 @@ export const DownloadModule = {
namespaced: true,

state: (): State => ({
allVersions: [],
assignId: 0,
downloadObject: null, // TODO: Check if the last element of allVersions can be used instead of this
allDownloads: [],
}),

getters: <GetterTree<State, RootState>>{
activeDownloadCount(state) {
const active = state.allVersions.filter(
dl => !dl[1].failed && dl[1].progress < 100
const active = state.allDownloads.filter(
dl => !dl.failed && dl.progress < 100
);
return active.length;
},
currentDownload(state) {
return state.allDownloads[state.allDownloads.length-1] || null;
},
},

mutations: {
setDownloadObject(state: State, downloadObject: DownloadProgress) {
state.downloadObject = {...downloadObject};
},
pushDownloadObjectToAllVersions(state: State, params: { assignId: number, downloadObject: DownloadProgress }) {
state.allVersions = [...state.allVersions, [params.assignId, params.downloadObject]];
addDownload(state: State, downloadObject: DownloadProgress) {
state.allDownloads = [...state.allDownloads, downloadObject];
},
updateDownloadObject(state: State, params: { assignId: number, downloadVersion: [number, DownloadProgress]}) {
const newVersions = [...state.allVersions];
newVersions[params.assignId] = params.downloadVersion;
state.allVersions = newVersions;
updateDownload(state: State, downloadObject: DownloadProgress) {
const newVersions = [...state.allDownloads];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: "versions" is still used in the variable names. E.g. in my suggestion above I used just "downloads".

const index = newVersions.findIndex((oldDownloadObject: DownloadProgress) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor, and this also applies to my suggested code in an earlier comment. If everything here works as I assume it should work, we should be able to check something like if (newVersions[downloadObject.assignId].assignId !== downloadObject.assignId). AFAICT the assignId and objects position in the array should always be in sync, and a sanity check like this might actually be the only reason why we need to store the assignId in the object at all.

That being said, please test this change thoroughly.

return oldDownloadObject.assignId === downloadObject.assignId;
});
if (index === -1) {
throw new R2Error(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: Did you test this? Is the error message shown to user in the error modal like it's written here, or does it get overwritten somewhere along the way? If this error is shown to the user, I think the middle line might be too technical while offering the user no actionable information, nor provides anything for us to debug the issue. Something like "DownloadProgress with id ${oldDownloadObject.assignId} was not found" would at least help in debugging if the value turns out to be undefined or something?

'Failed to update download status.',
'Download status object with a specified index was not found, which means that there\'s a mismatch between the download statuses in memory and what\'s being iterated over.',
'Try initiating the download again or restarting the app.'
);
}
newVersions[index] = downloadObject;
state.allDownloads = newVersions;
},
increaseAssignId(state: State) {
state.assignId++;
}
},
}
Loading