Mod installation progress tracking #1625

VilppeRiskidev · 2025-01-23T14:02:07Z

No description provided.

src/components/mixins/DownloadMixin.vue

src/components/profiles-modals/ImportProfileModal.vue

src/components/views/DownloadModModal.vue

src/pages/DownloadMonitor.vue

src/store/modules/DownloadModule.ts

src/utils/ProfileUtils.ts

…talling and display it on the UI

src/components/mixins/DownloadMixin.vue

src/components/profiles-modals/ImportProfileModal.vue

src/pages/DownloadMonitor.vue

src/utils/ProfileUtils.ts

anttimaki

In addition to these there was some buggy behaviour shared in voice chat.

src/pages/DownloadMonitor.vue

src/r2mm/downloading/BetterThunderstoreDownloader.ts

VilppeRiskidev · 2025-02-03T15:53:59Z

Buggy behaviour should be at least better now, I'll have to test a bit more to see if it still fails in some conditions.

VilppeRiskidev · 2025-02-03T16:21:07Z

https://github.com/ebkr/r2modmanPlus/pull/1617/files#r1939046141 "Any error from .getModList() is silently ignored" has been fixed

VilppeRiskidev · 2025-02-03T16:22:43Z

https://github.com/ebkr/r2modmanPlus/pull/1617/files#r1939046141

Depending on how .resolveConflicts() is used elsewhere, and how it's implemented internally it could make sense to not pass modList as an argument but read it internally (it seems only profile is required to do that). However, keeping ProfileModList and the implemenation of ConflictManagementProvider loosely coupled might be the correct idea too
.resolveConflicts() is called separately for each installed mod in this loop, and it might be unnecessary if calling it only once in the end with the final modList yields the same results. However, if we want to fail early to keep the profile from corrupting somehow, the current approach might be the correct idea

These two seem a bit trickier, I think we shuld create a new ticket for these if it is something we want to do.

anttimaki

I don't consider any of the comments alone a blocker, but given there's a few of them, consider if some should be implemented now.

Ideas for future tasks:

Prevent background mod list updates while there are active downloads
Allow clearing one/all completed downloads via DownloadMonitor
Allow retrying failed downloads via DownloadMonitor
Use the actual file size information in ThunderstoreVersion to give different weights to different size mods when determining the progress

src/components/views/DownloadModModal.vue

src/pages/DownloadMonitor.vue

anttimaki · 2025-02-05T11:21:38Z

src/r2mm/downloading/BetterThunderstoreDownloader.ts

                downloadCount += 1;
            } else {
                console.error(`Ignore unknown status code "${status}"`);
                return;
            }
-            totalProgressCallback(totalProgress, combo.getMod().getName(), status, err);
+            totalProgressCallback(Math.round(totalDownloadProgress), modInProgressName, status, err);


I think other places use Math.floor, any reason to use .round instead here? (If changed, should be changed to TSMM sibling PR as well.)

The reason was that the generateProgressPercentage() sometimes returns like 99.9999999 instead of 100 (floating point number calculations are sometimes a tiny bit inaccurate) and therefore the download progress got stuck at 99%. Should the generateProgressPercentage actually only return integers (round the number before returning)? (I implemented that as a part of the changes)

If the change is OK, should/could the rounding of the progress percentage be removed from the UI components or is it a best practice to round just in case?

In hindsight this whole thing should probably be designed so that it's the Vuex module that receives updates of each mods progress and internally keeps tracks how many mods is completed out of how many mods total, and what's the process of currently downloading mod. And it should then be the one that provides a getter or some other mechanic for providing the total progress to UI (rounded in one place and in one consistent way). That would also allow us to show it as percentage for progress bars, or as "Downloading mod 5 out of 6" for other status indicators.

Anyway, I'd be careful in changing generateProgressPercentage() as it's called from elsewhere too. If you think this change improves the overall situation, feel free to go ahead with it, but do test it thoroughly (all functions that call generateProgressPercentage() and related UI parts).

src/utils/ProfileUtils.ts

anttimaki

Bonus: it seems at some point the order of downloads has changed. Develop downloads the target mod first and then the dependencies, while this branch downloads dependencies first. IDK if it matters at all, but inadvertently changing feels like begging for problems.

src/pages/DownloadMonitor.vue

src/r2mm/downloading/BetterThunderstoreDownloader.ts

anttimaki

I'm now happy with these changes, with exception of this comment not being addressed:

Bonus: it seems at some point the order of downloads has changed. Develop downloads the target mod first and then the dependencies, while this branch downloads dependencies first. IDK if it matters at all, but inadvertently changing feels like begging for problems.

I think you should either change the order back; or get a confirmation from ebkr that changing the order doesn't matter.

It's probably easy to overlook the comments that are not assigned to any row, so I'll try to avoid them in future reviews even if the comment isn't really related to any specific row. That being said, read the non-line review comments anyway.

VilppeRiskidev · 2025-02-14T11:46:27Z

Fixed the main mod vs. dependencies ordering thing 👍

anttimaki · 2025-02-17T07:37:44Z

src/pages/DownloadMonitor.vue

+                                    <Progress
+                                        :max='100'
+                                        :value='100'
+                                        :className="['is-success']"


Why is the failed state using is-success? Wasn't this fixed once already? Are there likely be other regressions with the latest changes? Why wasn't this detected when you tested the changes?

🤦‍♂️ Oops, my bad, should be all good now.

anttimaki

In addition my testing revealed some buggy behaviour that is hard to reproduce. We can discuss these in the daily. Until I understand what happened in those cases, I don't feel this is ready for release yet.

anttimaki · 2025-02-19T08:25:39Z

src/pages/DownloadMonitor.vue

+                                    <Progress
+                                        :max='100'
+                                        :value='100'
+                                        :className="['is-error']"


The diff says this used to be is-danger. As far as I can tell, the component doesn't support is-error class at all.

I could've sworn I copied it back from the diff, but I guess not. 💩💩💩

anttimaki

Alright, looks finally good to me. Couldn't reproduce the problems from yesterday anymore so lets hope they were once in a blue moon sort of thing.

VilppeRiskidev self-assigned this Jan 23, 2025

VilppeRiskidev requested a review from anttimaki January 23, 2025 14:31

VilppeRiskidev force-pushed the download-completed-callback-improvement branch from e433150 to 911baa4 Compare January 23, 2025 14:44

VilppeRiskidev force-pushed the mod-installation-progress-tracking branch 2 times, most recently from 4ef5cbc to f27752a Compare January 23, 2025 15:03

anttimaki requested changes Jan 24, 2025

View reviewed changes

VilppeRiskidev force-pushed the mod-installation-progress-tracking branch from f27752a to 0ca299f Compare January 28, 2025 12:55

VilppeRiskidev force-pushed the download-completed-callback-improvement branch from 5242ff6 to 9ca4173 Compare January 28, 2025 12:59

VilppeRiskidev added 6 commits January 28, 2025 15:00

Simplify download logic by omitting callback

2d0cc91

Move download progress callbacks to DownloadMixin

09fd3bf

Show failed download status unambiguously in DownloadMonitor

7680c4b

Rename progress to downloadProgress

fb6ad5d

Add installationProgress to the Vuex store, update the value when ins…

1c37df5

…talling and display it on the UI

Show installation progress in the DownloadMonitor correclty

a7b06bd

VilppeRiskidev force-pushed the mod-installation-progress-tracking branch 2 times, most recently from dfb6b56 to 03af2b9 Compare January 28, 2025 14:33

VilppeRiskidev requested a review from anttimaki January 28, 2025 14:35

anttimaki requested changes Jan 29, 2025

View reviewed changes

VilppeRiskidev force-pushed the download-completed-callback-improvement branch from 9ca4173 to db6a4f9 Compare January 29, 2025 13:31

VilppeRiskidev force-pushed the mod-installation-progress-tracking branch 3 times, most recently from 61bd4bb to 17bfa4f Compare January 30, 2025 13:54

VilppeRiskidev force-pushed the download-completed-callback-improvement branch from db6a4f9 to 7ea20ca Compare January 30, 2025 14:05

VilppeRiskidev force-pushed the mod-installation-progress-tracking branch from 17bfa4f to 0845cb9 Compare January 30, 2025 14:07

VilppeRiskidev requested a review from anttimaki January 30, 2025 14:08

anttimaki mentioned this pull request Feb 3, 2025

Download completed callback improvement #1617

Open

anttimaki reviewed Feb 3, 2025

View reviewed changes

src/pages/DownloadMonitor.vue Outdated Show resolved Hide resolved

src/r2mm/downloading/BetterThunderstoreDownloader.ts Outdated Show resolved Hide resolved

VilppeRiskidev force-pushed the download-completed-callback-improvement branch from 7ea20ca to 9ca4173 Compare February 3, 2025 13:48

VilppeRiskidev force-pushed the mod-installation-progress-tracking branch from 0845cb9 to 03af2b9 Compare February 3, 2025 13:51

Further improve installation progress tracking

c359b28

VilppeRiskidev force-pushed the mod-installation-progress-tracking branch from 03af2b9 to c359b28 Compare February 3, 2025 14:01

Fix issues with mod download/install progress tracking

d03f65a

VilppeRiskidev requested a review from anttimaki February 3, 2025 16:23

anttimaki reviewed Feb 5, 2025

View reviewed changes

VilppeRiskidev force-pushed the mod-installation-progress-tracking branch from 628656b to c868b98 Compare February 5, 2025 15:12

VilppeRiskidev requested a review from anttimaki February 5, 2025 15:17

anttimaki reviewed Feb 7, 2025

View reviewed changes

src/pages/DownloadMonitor.vue Show resolved Hide resolved

src/pages/DownloadMonitor.vue Show resolved Hide resolved

src/r2mm/downloading/BetterThunderstoreDownloader.ts Outdated Show resolved Hide resolved

VilppeRiskidev force-pushed the mod-installation-progress-tracking branch from c868b98 to 82ad2c5 Compare February 11, 2025 11:50

VilppeRiskidev requested a review from anttimaki February 11, 2025 13:26

anttimaki approved these changes Feb 12, 2025

View reviewed changes

VilppeRiskidev force-pushed the mod-installation-progress-tracking branch from 82ad2c5 to a04686c Compare February 14, 2025 11:45

anttimaki requested changes Feb 17, 2025

View reviewed changes

VilppeRiskidev force-pushed the mod-installation-progress-tracking branch 2 times, most recently from bda4456 to b1395dd Compare February 18, 2025 15:27

VilppeRiskidev requested a review from anttimaki February 18, 2025 15:32

anttimaki requested changes Feb 19, 2025

View reviewed changes

Fix getModList errors being silently ignored

a362322

VilppeRiskidev force-pushed the mod-installation-progress-tracking branch from b1395dd to a362322 Compare February 19, 2025 12:45

anttimaki approved these changes Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mod installation progress tracking #1625

Mod installation progress tracking #1625

VilppeRiskidev commented Jan 23, 2025

anttimaki left a comment

VilppeRiskidev commented Feb 3, 2025

VilppeRiskidev commented Feb 3, 2025

VilppeRiskidev commented Feb 3, 2025

anttimaki left a comment

anttimaki Feb 5, 2025

VilppeRiskidev Feb 5, 2025 •

edited

Loading

VilppeRiskidev Feb 5, 2025

anttimaki Feb 7, 2025

anttimaki left a comment

anttimaki left a comment

VilppeRiskidev commented Feb 14, 2025

anttimaki Feb 17, 2025

VilppeRiskidev Feb 18, 2025

anttimaki left a comment

anttimaki Feb 19, 2025

VilppeRiskidev Feb 19, 2025

anttimaki left a comment

Mod installation progress tracking #1625

Are you sure you want to change the base?

Mod installation progress tracking #1625

Conversation

VilppeRiskidev commented Jan 23, 2025

anttimaki left a comment

Choose a reason for hiding this comment

VilppeRiskidev commented Feb 3, 2025

VilppeRiskidev commented Feb 3, 2025

VilppeRiskidev commented Feb 3, 2025

anttimaki left a comment

Choose a reason for hiding this comment

anttimaki Feb 5, 2025

Choose a reason for hiding this comment

VilppeRiskidev Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

VilppeRiskidev Feb 5, 2025

Choose a reason for hiding this comment

anttimaki Feb 7, 2025

Choose a reason for hiding this comment

anttimaki left a comment

Choose a reason for hiding this comment

anttimaki left a comment

Choose a reason for hiding this comment

VilppeRiskidev commented Feb 14, 2025

anttimaki Feb 17, 2025

Choose a reason for hiding this comment

VilppeRiskidev Feb 18, 2025

Choose a reason for hiding this comment

anttimaki left a comment

Choose a reason for hiding this comment

anttimaki Feb 19, 2025

Choose a reason for hiding this comment

VilppeRiskidev Feb 19, 2025

Choose a reason for hiding this comment

anttimaki left a comment

Choose a reason for hiding this comment

VilppeRiskidev Feb 5, 2025 •

edited

Loading