Pack import fixes and improvements

[Trial97]

Multiple fixes and improvements regarding pack import:

• Replace \ with / in the file path for modrinth export (this pack: https://modrinth.com/modpack/another-rpg-pack failed to correctly install because of this)
• Added a new dialog in pack import to ask the user what optional mods should be enabled (this is for both flame and modrinth)
• Fixed some issues introduced by Preserve file extension of instance icon when importing #1456
• Fixed Auto-name downloaded icons from platforms which don't get proper names #536
• Fixed Mods were downloaded into root .minecraft folder instead of mods/ while importing a Modrinth modpack "Create Extra" #1652

[DioEgizio]

wouldn't not selecting a mod = not downloading it make more sense? this behavior is kinda weird. Apart from that this pr seems fine

[Trial97]

Well not really: If you would want to add them later how would you know what mods need to be added and what version?
Some alternatives would be:

• reading the manifest file and downloading the files yourself(that's not easy at all)
• if you have access to a modlist compare it with the current one and again download the correct versions of the mods you want
• reinstall the pack (this is the easier option) but would overwrite config and disabled mods

Also the previous implementation the mods are always downloaded but they are disabled if they are optional.

[DioEgizio]

https://docs.modrinth.com/docs/modpacks/format_definition/#env-optional

The modrinth docs seem to prefer the behaviour I mentioned (which is honestly the one which makes the most sense to me and it's what launchers that have this option do, and what we do for atlauncher packs)

honestly it doesn't make much sense to have the page if you're still gonna download it but disabled anyways

[TheKodeToad]

I think it's nice as it allows the user to review the mods which will be disabled by default

[Trial97]

In the documentation, I do not see anywhere how should I handle the unselected resources(doesn't say that I should not download them).
I still want to keep them downloaded: in case you want to enable an optional mod in the future.
Also checked against :

• curseforge downloads the optional and marks them as disabled
• modrinth downloads the optional and doesn't mark them as disabled

All of them download the optional files.

[TheKodeToad]

I assume in the case of Modrinth it is either unimplemented or a bug :P

[TheKodeToad]

Maybe we should stop checking extension and just add .disabled?

[Trial97]

I thought that was something modrinth does, on flame there is no such check.
I just copied the code from the previous implementation so we can drop it

[TheKodeToad]

Not necessarily in this pr - but resource packs etc. can also be optional

[nopeless]

hey @Trial97 , I like the pull request, but I think [^a-zA-Z0-9] is too restrictive.

something like [\0-\31\/:*?"<>|] would be better

[Trial97]

Hi @nopeless,
I'm not sure as I understand what you want to achieve with the new regex.
Why the need to change that?
Do we have collisions between the ATLauncher packs with the current regex?
We use that field only for the logo filename so if there are no collisions between the packs I would prefer to remain the same.

[nopeless]

Hi @nopeless,
I'm not sure as I understand what you want to achieve with the new regex.
Why the need to change that?
Do we have collisions between the ATLauncher packs with the current regex?
We use that field only for the logo filename so if there are no collisions between the packs I would prefer to remain the same.

No, and I doubt there will be any collisions unless the filename is unicode (which is very unlikely either)

However, I don't see why the regexp has to be so restrictive either. If it needs to be filename safe, then the regex should show that intent.

At the very least, I think you should add a space to the allow character list so spaces are preserved for easier reading in case of a need to debug.

[nopeless]

Or I may have misread the code I'm assuming safeName is for storing in local. If there's another use for it, I can see where I'm wrong

[Trial97]

That name is precisely only used when we import the pack to give the name of the icon of the pack, nothing else.
And I stand firm in my decision to not change the regex.
Obviously, if you understand what you do please open a PR with your suggested change, and explain why we would want that.

[TayouVR]

everything seems to work in my testing.

this "Create Extra" pack from mr confused me a bit as it seems to put a lot of libraries like fabric API and fabric language kotlin, etc. as optional. But I guess thats just how it is..
It seems a bit odd to me, but I have to assume its just the pack making weird choices in what should be optional and what not...

UNLESS thats something coming from prism due to some dependency resolution stuff that needs to be considered here? thats just how the pack comes, imma merge