Identify zip file contents #576

Ryex · 2022-12-10T04:08:06Z

Purpose

This PR will add support to identify zip archives (and in some cases bare folders) as containing:

With this support it will also add check during instance creation to ensure zip files are placed appropriately.

Added as a draft for tracking and review purposes.

Related Issues

When complete this should fix #349.

Additional Info

will enable the creation of support for managing datapacks.

launcher/minecraft/mod/DataPack.cpp

+    switch (type) {
+        default: {
+            auto res = Resource::compare(other, type);
+            if (res.first != 0)
+                return res;
+        }
+        case SortType::PACK_FORMAT: {
+            auto this_ver = packFormat();
+            auto other_ver = cast_other.packFormat();
+
+            if (this_ver > other_ver)
+                return { 1, type == SortType::PACK_FORMAT };
+            if (this_ver < other_ver)
+                return { -1, type == SortType::PACK_FORMAT };
+        }
+    }


launcher/minecraft/mod/Mod.cpp

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

moves the reading of mod files into `ModUtils` namespace Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

Ryex · 2022-12-24T19:15:04Z

Ok, just added a simple validator for Shaderpacks. Only world saves left and then I can write a function to take a file name and tell you what it is if it is any of these things.

I'm a little hung up on how to validate world saves

There are two real questions:

Should this have it's own Resource class and parse task like the others? if so where should these classes live? launcher/minecraft/mod feels inappropriate but it's where everything else is.
Should the validator identify and mark the save format? I know what your thinking "save format"? but there are two major ways a save could present.
an archive containing

saves/
├─ <word_name>/
│  ├─ level.dat

an archive containing

<word_name>/
├─ level.dat

The first is done to make extracting the save as easy as unpacking the archive to the minecraft folder while the second requires knowing to unpack to the saves folder but both are commonly used and distributed.

I think this change is significant enough that code handling the world save would like to know beforehand without needing to check itself. Should it use a m_format enum?

flowln · 2022-12-24T19:44:15Z

Should this have it's own Resource class and parse task like the others? if so where should these classes live? launcher/minecraft/mod feels inappropriate but it's where everything else is.

I agree that the name isn't the best, but that folder should really be called resources instead of mod, considering all that is there now. I just didn't do it because it'll be a big big diff :p
Anyway, i think the world stuff can go there regardless of the current name of that folder

Should the validator identify and mark the save format?

I think you can make it a separate enum if you really think it would change a lot of the code. It would possibly be similar to ResourceType, but only for Worlds. However, I don't see how that'd be such a big thing in the code for parsing it. Wouldn't it just be a simple check like this?

if ("saves" folder exists)
    goToSubfolder("saves");
// Continue parsing ...

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

Ryex · 2022-12-25T01:11:00Z

Ok, all the new validators and tests are in place. taking off WIP for preliminary review.

In order to truly close #349 I just have to add the check to the instance creation task after the files are downloaded

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

Ryex · 2022-12-25T03:44:57Z

And... DONE!
Added a check on FlameInstaceCreationTask to validate zip resources are in the right place.

checked with these modpacks

https://www.curseforge.com/minecraft/modpacks/foh-survival-legacy/ 1.7.10 modpack that uses Mo's Creatures per CurseForge mods distributed as .zip get downloaded to the resourcepacks folder #349 (mod in .zip)
https://www.curseforge.com/minecraft/modpacks/better-mc-fabric Has datapacks
https://www.curseforge.com/minecraft/modpacks/medieval-mc-fabric has datapacks

launcher/modplatform/flame/FlameInstanceCreationTask.cpp

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

Ryex · 2022-12-25T23:54:12Z

validated that world saves are handled correctly via https://www.curseforge.com/minecraft/modpacks/mythcraft-revilo2

Ryex · 2022-12-25T23:57:26Z

.. the build fails.... oh that is weird. in my machine somehow it knows how to convert QString to QFileInfo...

OH! it's a qt 5 vs 6 thing

fix: validatePath in validateZIPResouces Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

Ryex · 2022-12-26T00:17:12Z

ok, fixed

flowln

sorry for the big amount of comment changes 😅

When you changed the process functions to return a bool instead of void, you also added checks to see if they returned the expected values. In principle, that's good, however maybe that's a bit too strict. For instance, suppose a resource pack doesn't have a pack.png file. It is a valid state, since it will just use the default image on the game, but when you check its result, and returns false from the entire processing of the resource pack, you make it so that the lack of an image is considered as a failure. Shouldn't that be changed, so that optional fields don't trigger this kind of failure?

otherwise the logic looks mostly fine, thanks ☺️

launcher/minecraft/mod/DataPack.h

launcher/minecraft/mod/Mod.cpp

launcher/minecraft/mod/ResourcePack.cpp

launcher/modplatform/flame/FlameInstanceCreationTask.cpp

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

Ryex · 2022-12-26T21:58:06Z

ok, all cleaned up now. sorry for the mess.

flowln

ok, all cleaned up now. sorry for the mess.

don't worry about it, it's a lot of code to keep track off, some stuff inevitably slips by 🙂

i only have a single note here, otherwise this looks fine for me i think!

launcher/modplatform/flame/FlameInstanceCreationTask.cpp

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

flowln

(only noting the newlines :p)

this looks good to me, thank you!

launcher/minecraft/mod/tasks/LocalResourceParse.h

launcher/minecraft/mod/tasks/LocalResourceParse.cpp

Co-authored-by: flow <flowlnlnln@gmail.com> Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

Scrumplex · 2023-01-07T16:25:17Z

When you changed the process functions to return a bool instead of void, you also added checks to see if they returned the expected values. In principle, that's good, however maybe that's a bit too strict. For instance, suppose a resource pack doesn't have a pack.png file. It is a valid state, since it will just use the default image on the game, but when you check its result, and returns false from the entire processing of the resource pack, you make it so that the lack of an image is considered as a failure. Shouldn't that be changed, so that optional fields don't trigger this kind of failure?

any comment about this?

As long as other metadata checks out, it probably shouldn't fail if pack.png doesn't exist/is bad.

Scrumplex

LGTM

flowln · 2023-01-07T16:30:37Z

any comment about this?

As long as other metadata checks out, it probably shouldn't fail if pack.png doesn't exist/is bad.

they changed it already iirc, see here

Scrumplex · 2023-01-07T16:33:22Z

Oh I see, I misread that code 👀

github-advanced-security bot found potential problems Dec 10, 2022

View reviewed changes

launcher/minecraft/mod/Mod.cpp Fixed Show fixed Hide fixed

Ryex force-pushed the identify-zip-packs branch from f78b742 to 2a739ec Compare December 10, 2022 08:32

flowln added bug Something isn't working enhancement New feature or request labels Dec 10, 2022

flowln added this to the 7.0 milestone Dec 10, 2022

Ryex added 3 commits December 24, 2022 09:42

feat: add initial support for parseing datapacks

64c51a7

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

fix: force add of ignored testdata files

25e23e5

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

feat: add a ModUtils::validate

878614f

moves the reading of mod files into `ModUtils` namespace Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

Ryex force-pushed the identify-zip-packs branch from 2a739ec to ebd905e Compare December 24, 2022 18:59

feat: add shaderpack validation

ccfe605

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

Ryex force-pushed the identify-zip-packs branch from ebd905e to db03845 Compare December 25, 2022 00:44

Ryex marked this pull request as ready for review December 25, 2022 01:11

Ryex changed the title ~~WIP: Identify zip file contents~~ Identify zip file contents Dec 25, 2022

Ryex added 4 commits December 24, 2022 20:43

feat: worldSave parsing and validation

eb31a95

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

feat: validate world saves

a7c9b2f

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

fix: update parse tests

cfce54f

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

feat: zip resource validation check for flame

8422e3a

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

Ryex force-pushed the identify-zip-packs branch from 3c05705 to 8422e3a Compare December 25, 2022 03:43

github-advanced-security bot found potential problems Dec 25, 2022

View reviewed changes

launcher/modplatform/flame/FlameInstanceCreationTask.cpp Fixed Show fixed Hide fixed

feat: support installing worlds during flame pack import.

78984ee

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

fix: explicit QFileInfo converison for qt6

b2082bf

fix: validatePath in validateZIPResouces Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

flowln self-requested a review December 26, 2022 13:18

flowln reviewed Dec 26, 2022

View reviewed changes

Ryex force-pushed the identify-zip-packs branch 3 times, most recently from 5fe5a94 to bc0da56 Compare December 26, 2022 21:43

fix: cleanup and suggested changes

3691f3a

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

Ryex force-pushed the identify-zip-packs branch from bc0da56 to 3691f3a Compare December 26, 2022 21:50

flowln reviewed Dec 29, 2022

View reviewed changes

launcher/modplatform/flame/FlameInstanceCreationTask.cpp Outdated Show resolved Hide resolved

Ryex added 2 commits December 29, 2022 17:21

refactor: use std::filesystem::rename insted of copy and then moving.

c470f05

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

refactor: add an identify function to make easy to reuse

7f43842

Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

flowln approved these changes Dec 30, 2022

View reviewed changes

launcher/minecraft/mod/tasks/LocalResourceParse.h Outdated Show resolved Hide resolved

launcher/minecraft/mod/tasks/LocalResourceParse.cpp Outdated Show resolved Hide resolved

fix newlines

0ebf04a

Co-authored-by: flow <flowlnlnln@gmail.com> Signed-off-by: Rachel Powers <508861+Ryex@users.noreply.github.com>

Ryex mentioned this pull request Dec 31, 2022

Import all the things! #696

Merged

Scrumplex reviewed Jan 7, 2023

View reviewed changes

Scrumplex approved these changes Jan 7, 2023

View reviewed changes

Scrumplex merged commit f3f6284 into PrismLauncher:develop Jan 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Identify zip file contents #576

Identify zip file contents #576

Ryex commented Dec 10, 2022 •

edited

Ryex commented Dec 24, 2022

flowln commented Dec 24, 2022

Ryex commented Dec 25, 2022 •

edited

Ryex commented Dec 25, 2022 •

edited

Ryex commented Dec 25, 2022

Ryex commented Dec 25, 2022 •

edited

Ryex commented Dec 26, 2022

flowln left a comment

Ryex commented Dec 26, 2022

flowln left a comment

flowln left a comment

Scrumplex commented Jan 7, 2023

Scrumplex left a comment

flowln commented Jan 7, 2023

Scrumplex commented Jan 7, 2023

Identify zip file contents #576

Identify zip file contents #576

Conversation

Ryex commented Dec 10, 2022 • edited

Purpose

Related Issues

Additional Info

Ryex commented Dec 24, 2022

flowln commented Dec 24, 2022

Ryex commented Dec 25, 2022 • edited

Ryex commented Dec 25, 2022 • edited

Ryex commented Dec 25, 2022

Ryex commented Dec 25, 2022 • edited

Ryex commented Dec 26, 2022

flowln left a comment

Choose a reason for hiding this comment

Ryex commented Dec 26, 2022

flowln left a comment

Choose a reason for hiding this comment

flowln left a comment

Choose a reason for hiding this comment

Scrumplex commented Jan 7, 2023

Scrumplex left a comment

Choose a reason for hiding this comment

flowln commented Jan 7, 2023

Scrumplex commented Jan 7, 2023

Ryex commented Dec 10, 2022 •

edited

Ryex commented Dec 25, 2022 •

edited

Ryex commented Dec 25, 2022 •

edited

Ryex commented Dec 25, 2022 •

edited