Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add modpack update page in instance settings #32

Merged
merged 30 commits into from Dec 10, 2022

Conversation

flowln
Copy link
Contributor

@flowln flowln commented Oct 18, 2022

Closes #180
Closes #170

TODO: I'm satisfied with the state of this for now, so I'll open it for review :p

  • Modrinth modpack page
  • Flame modpack page
  • Fix icons with managed packs not from MR/CF (or probably hide them since we can't update them yet anyways)
  • Make the changelog thing parse the content as markdown
  • Add link to the modpack page in the mod provider
    • Modrinth URLs
    • Flame URLs not really sure how to do this. Maybe we should have another field in instance.cfg, or do another NetJob to get the thing (though it would be slow, blocking and really not super useful)

and probably some more stuff i forgot about :x

Status (as of 9cf53f8):

FO modpack page

Nomifactory modpack page

@flowln flowln added the enhancement New feature or request label Oct 18, 2022
@CutestNekoAqua
Copy link

Button to update Modpack automatically too?

@CutestNekoAqua
Copy link

PS: What about a notification that there are updates for a modpack?

@flowln
Copy link
Contributor Author

flowln commented Oct 18, 2022

Button to update Modpack automatically too?

i don't feel great about automatically updating a modpack, since people should really backup their stuff before an update. But the idea is to have a button that allows for easy updating if the user wants to :)

PS: What about a notification that there are updates for a modpack?

maybe we could have something like that, but i guess you should make an issue about that, because it's unlikely i'll be able to do that in this single PR :p

@CutestNekoAqua
Copy link

Button to update Modpack automatically too?

i don't feel great about automatically updating a modpack, since people should really backup their stuff before an update. But the idea is to have a button that allows for easy updating if the user wants to :)
Yea, I did mean semi-automatic. You click the button and it does the update on its own. But you need to click it again for the next update.

PS: What about a notification that there are updates for a modpack?

maybe we could have something like that, but i guess you should make an issue about that, because it's unlikely i'll be able to do that in this single PR :p

Sure! I can even work on it as soon as you finished this PR myself :)

@Scrumplex
Copy link
Member

Is this scheduled for 5.0?

@flowln
Copy link
Contributor Author

flowln commented Oct 18, 2022

Is this scheduled for 5.0?

if we are going to release 5.0 today, no :iea:

@HyperSoop
Copy link
Contributor

Some of my own opinions on this:

  • make this menu a button in "version" instead, I'd rather not have any of my instances individually cursed/blessed with having a whole separate menu on the sidebar for modpack information.
  • make sure an instance's modpack status can be completely removed by the user through the launcher

@flowln
Copy link
Contributor Author

flowln commented Oct 23, 2022

make this menu a button in "version" instead, I'd rather not have any of my instances individually cursed/blessed with having a whole separate menu on the sidebar for modpack information.

i don't think only a button is enough for this. It doesn't provide useful information such as changelogs and other metadata, which can in fact be very important when updating. My original design was to make it a tab inside a 'Overview' page, instead of being in the sidebar, but that'll take more time to be done.

make sure an instance's modpack status can be completely removed by the user through the launcher

Maybe, but currently this status doesn't add any restriction on its own, so if you don't want to use an instance as a managed pack, you can just not use the tab :p

Perhaps it could be an option in the future (similar to adding such a status to an already existing instance).

@HyperSoop
Copy link
Contributor

i don't think only a button is enough for this. It doesn't provide useful information such as changelogs and other metadata, which can in fact be very important when updating. My original design was to make it a tab inside a 'Overview' page, instead of being in the sidebar, but that'll take more time to be done.

Well, a button that opens the menu in a popup, that is.

@flowln
Copy link
Contributor Author

flowln commented Oct 23, 2022

Well, a button that opens the menu in a popup, that is.

why have it in a popup? it just seems unnecessary to me :/

@HyperSoop
Copy link
Contributor

To not clutter the sidebar and to not have wast amounts of emptiness in the window in case the edit instance menu is stretched out to cover the whole screen

@flowln
Copy link
Contributor Author

flowln commented Oct 23, 2022

To not clutter the sidebar and to not have wast amounts of emptiness in the window in case the edit instance menu is stretched out to cover the whole screen

image

i think adding something here would be more clutter than having a page on the sidebar though

@HyperSoop
Copy link
Contributor

we really should do something to this too though. we could swap the mod loader install section with the upper one, and move the folder shortcuts to the bottom.

Not to say there isn't clutter on the left.

@HyperSoop
Copy link
Contributor

HyperSoop commented Oct 23, 2022

Maybe, but currently this status doesn't add any restriction on its own, so if you don't want to use an instance as a managed pack, you can just not use the tab :p

Perhaps it could be an option in the future (similar to adding such a status to an already existing instance).

I wouldn't like to have 50% of my instances permanently marked as modpacks just because they are based off SO. There also needs to be an option to completely disable new modpack instances from getting modpack metadata.

The modpack updater should be treated in the same way the mod updater is - it should not be in your eyes too much, it's just something that you use once in a while and then forget about it for a few days. If it's not so, it's bloat.

@HyperSoop
Copy link
Contributor

Is it for sure that the "if a new instance of a modpack is created, annoyingly prompt the suer to update an existing one" will be removed with this PR? Again, I want to update my modpack instance when i want to update it, not when i create a new instance of it.

@flowln
Copy link
Contributor Author

flowln commented Nov 7, 2022

Dunno, some people seem to like it. It really depends on whether we can make the UX not be obnoxious for those that don't want it, but useful for those that do. Anyway, that'd not be done in this particular PR anyway, since it's not in the intended scope to change that.

@HyperSoop
Copy link
Contributor

Can we make a toggle for this in the options? Like "Prompt to update modpack on instance creation"

@WaterSword WaterSword mentioned this pull request Nov 11, 2022
1 task
@flowln flowln marked this pull request as ready for review November 18, 2022 20:41
@DioEgizio
Copy link
Member

on curseforge packs, going to the curseforge tab, going back to version and then going back to curseforge causes a segmentation fault

Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
…pending

Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
This allows us to pass to the creation instances their actual pack ID
and version ID, that in Flame's case, are only available before starting
to create an instance.

Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
Prevents versions to undergo mitosis.

Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
Also carry on the original ID to avoid updating the wrong instance.

Signed-off-by: flow <flowlnlnln@gmail.com>
Signed-off-by: flow <flowlnlnln@gmail.com>
Since the exact version string is only available in the manifest,
there's no easy way of getting it before commiting to the update, so
there's not much of a good way of showing the updated name in the UI,
and using the displayName is weird and gives some buggy behavior.

We may want to re-enable it in the future if we find a reliable way of
showing the correct info on the UI before starting the update.

Signed-off-by: flow <flowlnlnln@gmail.com>
Copy link
Member

@Scrumplex Scrumplex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

third approval :trollface:

Copy link
Member

@ZekeZDev ZekeZDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me, everything seems functional and I cant find any glaring issues. I do think that we will need to consider future changes to make updates more accessible however as the page is I think it does its job well.

@flowln flowln merged commit 4a13d72 into PrismLauncher:develop Dec 10, 2022
@flowln flowln deleted the modpack_update_page branch December 10, 2022 14:34
@RaptaG
Copy link
Contributor

RaptaG commented Dec 10, 2022

\ö/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically update mods when the Minecraft version is changed Modpack versioning
9 participants