Skip to content

Fix DownloadManager and a few code improvements#2356

Closed
mick-lue wants to merge 3 commits intoModOrganizer2:masterfrom
mick-lue:download_fix_and_stuff
Closed

Fix DownloadManager and a few code improvements#2356
mick-lue wants to merge 3 commits intoModOrganizer2:masterfrom
mick-lue:download_fix_and_stuff

Conversation

@mick-lue
Copy link
Copy Markdown
Contributor

Hi there,
I have been wanting to fix the stuck downloads for quite a while now and finally managed to do it.
In the meantime, I've gone through most of the other code and (with the help of static analysis) found a few instances of possible improvements.

MO runs completely fine for me now and have had no issues. I've already reverted changes that were causing issues for me (before pushing the changes that is) and didn't find any more funky behavior.

The commented-out code can be deleted if it won't be used anymore.

Main Change (DownloadManager)

  1. When the download start, we don't immediately create a meta file. Only pausing or finishing a download will do that now
  2. Always connect the finished signal from the reply to downloadFinished (although we are checking for duplicate downloads before that)
  3. We emit aboutToUpdate() only when we will also emit update() (otherwise we use update(downloadInfo))
  4. writeData(info) now does more checks to protect from failed actions
  5. For each download, now generate a random downloadID to avoid potential ID overlaps. As downloadID isn't used anywhere else, this won't effect other stuff
  6. Also slightly improved redirect handling as this was my first theory as to why we got stuck downloads (it wasn't that).
  7. Added a fallback to getFileNameFromNetworkReply(QNetworkReply* reply)
  8. Commented out m_hasData as it is now obsolete
  9. Slight improvements to the logic of downloadFinished

Notable Changes

  • Patched Qt deprecation: Using QMetaType with typeId() instead of type()
  • I commented out GeometrySettings::saveDocks(const QMainWindow* mw) and GeometrySettings::restoreDocks(const QMainWindow* mw) as its comment states that the issue is long fixed in Qt

Generic Changes

  • Replaced a lot of inserts and push_backs with emplace or try_emplace or emplace_back when possible
    • emplace can construct objects in-place but can't be used on all occasions
  • Passed objects by (const) reference when possible
    • lots of for-loops and a few other instances were copying objects instead of using a reference
  • Passed most references to lambdas explicitely instead of using &
    • the compiler can help us better this way and we can clearly see what references are being used in the IDE
  • Fixed some typos in header file definitions
  • Changed some functions for cleaner code
    • e.g. using isEmpty() instead of size() == 0
    • moving from erase-remove_if-idioms to erase_if
  • Reordered some class inits according to their defined order in the header
  • commented out a few unused variables
  • removed calls to std::move that weren't able to actually move something in the context used

@mick-lue
Copy link
Copy Markdown
Contributor Author

I'll probably split this PR up into more digestible pieces. That'll take a few days though

@Silarn
Copy link
Copy Markdown
Member

Silarn commented Apr 13, 2026

We were planning to look at this soon but if you think that would be better.

I'll admit I wasn't sure how relevant all the emplace_back and lambda scope changes were to the download manager.

@mick-lue
Copy link
Copy Markdown
Contributor Author

yea it is unrelated as I went through basically the whole code base doing that when I needed a break from figuring out the DownloadManager. It does make MO2 a bit faster though (and the scope changes are for making intentionality clear).
This PR will be reduced to the DownloadManager related stuff though, as I am unhappy with how everything is mixed together to be honest.
Or maybe creating new PRs would be better, what do you think?

@Silarn
Copy link
Copy Markdown
Member

Silarn commented Apr 13, 2026

Rather than complexifying things, I'd probably rebase a couple branches and make separate PRs, I think.

@Silarn Silarn closed this Apr 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants