Generalize Mod models / pages and APIs to Resources #675
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
flowln
added
enhancement
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| m_ui->sortByBox->addItem(sorting.readable_name, QVariant(sorting.index)); | ||
| } | ||
|
|
||
| bool ResourcePage::setCurrentPack(ModPlatform::IndexedPack pack) |
Check notice
Code scanning / CodeQL
Large object passed by value
|
|
||
| class ResourcePage; | ||
|
|
||
| class ResourceDownloadDialog : public QDialog, public BasePageProvider { |
Check notice
Code scanning / CodeQL
Undisciplined multiple inheritance
Comment on lines
+82
to
+88
| switch (type) { | ||
| case ModPlatform::ResourceType::MOD: | ||
| return "mod"; | ||
| default: | ||
| qWarning() << "Invalid resource type for Modrinth API!"; | ||
| break; | ||
| } |
Check notice
Code scanning / CodeQL
No trivial switch statements
Comment on lines
+22
to
+26
| switch (type) { | ||
| default: | ||
| case ModPlatform::ResourceType::MOD: | ||
| return 6; | ||
| } |
Check notice
Code scanning / CodeQL
No trivial switch statements
Comment on lines
+361
to
+369
| switch (network_error_code) { | ||
| default: | ||
| // Network error | ||
| QMessageBox::critical(nullptr, tr("Error"), tr("A network error occurred. Could not load mods.")); | ||
| break; | ||
| case 409: | ||
| // 409 Gone, notify user to update | ||
| QMessageBox::critical(nullptr, tr("Error"), | ||
| QString("%1").arg(tr("API version too old!\nPlease update %1!").arg(BuildConfig.LAUNCHER_DISPLAYNAME))); | ||
| break; | ||
| } |
Check notice
Code scanning / CodeQL
No trivial switch statements
flowln
force-pushed
the
generalize_mod_model
branch
2 times, most recently
from
e3a58f3
to
e8b099e
Compare
flowln
force-pushed
the
generalize_mod_model
branch
from
e8b099e
to
6ff50e5
Compare
Firstly, this abstract away behavior in the mod download models that can also be applied to other types of resources into a superclass, allowing other resource types to be implemented without so much code duplication. For that, this also generalizes the APIs used (currently, ModrinthAPI and FlameAPI) to be able to make requests to other types of resources. It also does a general cleanup of both of those. In particular, this makes use of std::optional instead of invalid values for errors and, well, optional values :p This is a squash of some commits that were becoming too interlaced together to be cleanly separated. Signed-off-by: flow <flowlnlnln@gmail.com>
Puts them all inside the 'ResourceDownload' namespace, so that it's a bit clearer from the outside that those belong to the same 'module'. Signed-off-by: flow <flowlnlnln@gmail.com>
when i.e. clicking on links or just using the downloader at all, this prevents some flickering and the widget never getting hidden in some cases. Signed-off-by: flow <flowlnlnln@gmail.com>
No need for multiple files since the subclasses are so small now Signed-off-by: flow <flowlnlnln@gmail.com>
This makes it so that we don't need a reference to the parent page in the model. It will be useful once we change the page from a widget-based one to a QML page. It also makes tasks be created in the dialog instead of the page, so that the dialog can also have the necessary information to mark versions as selected / deselected easily. It also makes the task pointers into smart pointers. Signed-off-by: flow <flowlnlnln@gmail.com>
in preparation for QML interop. Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
This refactors the sorting methods to join every bit of it into a single list, easing maintanance. It also removes the weird index contraint on the list of methods by adding an index field to the DS that holds the method. Lastly, it puts the available methods on their respective API, so other resources on the same API can re-use them later on. Signed-off-by: flow <flowlnlnln@gmail.com>
This prevents a crash in which the pack list gets updated in a search request meanwhile a versions / extra info request is being processed. Previously, this situation would cause the reference in the latter callbacks to be invalidated by an internal relocation of the pack list. Signed-off-by: flow <flowlnlnln@gmail.com>
This allows us to check whether a search request is already on-going, in which case we don't need to make another one (and shouldn't). Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
While implementing the resource pack downloader in another branch, I noticed that most of the code in the success callback was identical in both cases, safe for a few minute differences in strings. So, this tries to make it easier to share this piece of code. However, it still leaves the possibility of extending the methods in ResourceModel to accomodate for cases where this similarity may not hold. Signed-off-by: flow <flowlnlnln@gmail.com>
shush Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
This makes it easier to create resource apis that aren't network-based. Signed-off-by: flow <flowlnlnln@gmail.com>
This allows the standard QAbstractItemModelTester to work without shenanigans! Signed-off-by: flow <flowlnlnln@gmail.com>
______very_____ basic indeed, creating tests is super boring :c Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
…rces Signed-off-by: flow <flowlnlnln@gmail.com>
flowln
force-pushed
the
generalize_mod_model
branch
from
6ff50e5
to
c294c2d
Compare
Prevents a single problematic mod from invalidating all the API response. Signed-off-by: flow <flowlnlnln@gmail.com>
The spec says that this can be null, and indeed some mods have it set to null, and should still be considered as valid. Signed-off-by: flow <flowlnlnln@gmail.com>
Scrumplex
approved these changes
Jan 20, 2023
timoreo22
approved these changes
Jan 23, 2023
*sobbing in messy legal stuff i know nothing about* Signed-off-by: flow <flowlnlnln@gmail.com>
Scrumplex
approved these changes
Jan 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This aims to generalize the existing logic for the mod downloader, in order to allow for other resource downloaders (like RPs, TPs, Shaders and Worlds) to be implemented without much too code duplication. I wrote a big big description here, talking that stuff can still change as I develop those downloader and find things that could change for the better here, but GH deleted it, so yea that's basically it 😭
It also aims to decouple the model from the page, in order to facilitate a future switch to a QML based interface.
Be sure to check out the commit descriptions, I wrote quite a lot in a couple of them to explain what I was doing and why.
TODO:
Add some tests: There could be a couple automated tests, especially for the model, testing some basic stuff. For more complex stuff though, we could have something like a Dummy subclass of ResourceAPI to provide testing data for the model. We could have a minimal target compiled for testing that just shows the ResourceDownloadDialog with the model hooked up to that API. No idea how to implement that though, and it seems like a lot of work, so if anyone has better ideas, please share! 🙂I've concluded that if I really go forward with adding all those tests here, i'll never finish this PR bc I don't really want to make these tests at this very moment. So, I've laid down some basic initial infrastructure to support more tests in the future, when someone (or me) is up to the task.