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

[VilppeRiskidev]

• Remove assignId field
• Remove downloadObject field
• Rename some fields and functions to be more descriptive
• assignId is no longer passed unnecessarily when calling vuex store functions

[anttimaki]

The changes match mostly what was planned, so in that sense this could be approved, but I think the suggested improvements are worthy of implementing right away.

One exception is that you didn't implement a getter for "next assign id" as was planned, nor was the plan augmented. Why's that?

Also generally speaking, while it's good to stick to the plan, I consider going a bit further than the plan if further improvement changes are discovered during implementation. Scope creep needs to be kept in mind though. I guess what I'm trying to say is to beware of the mentality where the plan is executed blindly, as it doesn't necessarily lead to best outcome.

[anttimaki]

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;
        },

[anttimaki]

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.

[anttimaki]

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

[anttimaki]

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)

[anttimaki]

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

[anttimaki]

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.

[anttimaki]

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?