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

Validate maximum memory allocation #436

Merged
merged 5 commits into from Nov 30, 2022

Conversation

Scrumplex
Copy link
Member

@Scrumplex Scrumplex commented Nov 11, 2022

Closes #426

Add a small checkmark/cross/warning symbol to the memory setting to tell the user if their input is a good idea or not.

Also raise memory limit to 1 TiB :trollface:

Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
@Scrumplex Scrumplex added the enhancement New feature or request label Nov 11, 2022
@Scrumplex Scrumplex added this to the 6.0 milestone Nov 11, 2022
@Scrumplex
Copy link
Member Author

Scrumplex commented Nov 11, 2022

New screenshots:
Valid memory allocation
Memory allocation approaching system memory
Memory allocation exceeding system memory

Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
Copy link
Contributor

@Edgars-Cirulis Edgars-Cirulis left a comment

Choose a reason for hiding this comment

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

Tested with my memory reserved hardwere.

  • Allocate more then 6.9 GB Test passed. instance lunches with 8 GB

Looks good to me

Copy link
Member

@TheKodeToad TheKodeToad left a comment

Choose a reason for hiding this comment

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

updateThresholds is duplicated. Apart from that (mostly) looks good. Maybe sysMiB could be static?

@Scrumplex
Copy link
Member Author

updateThresholds is duplicated. Apart from that (mostly) looks good. Maybe sysMiB could be static?

It is. But sadly, all instance-override settings are like that. We need to clean that up in the future.

@TheKodeToad
Copy link
Member

TheKodeToad commented Nov 11, 2022

updateThresholds is duplicated. Apart from that (mostly) looks good. Maybe sysMiB could be static?

It is. But sadly, all instance-override settings are like that. We need to clean that up in the future.

:P seems like Prism does that a lot (likely inherited from MultiMC).

Maybe you could still make the variable static?
Also, I don't see a need for the block at the end.

@flowln flowln self-requested a review November 20, 2022 12:23
Copy link
Contributor

@flowln flowln left a comment

Choose a reason for hiding this comment

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

looking good to me!

I was gonna say that it would perhaps make sense to also fix #427 in here, but I wasn't able to reproduce that issue neither here nor or develop, so idk 🤷‍♀️

@ghost
Copy link

ghost commented Nov 20, 2022

but I wasn't able to reproduce that issue neither here nor or develop

That issue only occurs with the setup screen

15-31-24.mp4

@flowln
Copy link
Contributor

flowln commented Nov 20, 2022

ah, that makes a bit more sense xD

well then, this PR needs to bump the maximum memory in that place too
i wonder though, maybe it would be nicer if we could remove the VersionSelectWidget thingy from JavaSettingsWidget, and make it generic enough so that we can reuse it in all three Java thingies?

@Scrumplex
Copy link
Member Author

maybe it would be nicer if we could remove the VersionSelectWidget thingy from JavaSettingsWidget, and make it generic enough so that we can reuse it in all three Java thingies?

Ideally, all settings pages should just be able to handle both global and instance-specific pages, depending on how they were instantiated.
That way, we could get rid of a lot of duplicate code.

Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
@flowln
Copy link
Contributor

flowln commented Nov 20, 2022

Ideally, all settings pages should just be able to handle both global and instance-specific pages, depending on how they were instantiated. That way, we could get rid of a lot of duplicate code.

well, yeah, but in this particular case, even if we did that, there'd still be some duplicate code with the widget for the wizard :|

@Scrumplex Scrumplex merged commit 9e1653e into PrismLauncher:develop Nov 30, 2022
@Scrumplex Scrumplex deleted the feat-change-maxmem branch November 30, 2022 17:22
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.

Suggestion with hardwere reserved memory laptops.
5 participants