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

Safer destructive actions #618

Merged

Conversation

TheKodeToad
Copy link
Member

@TheKodeToad TheKodeToad commented Dec 14, 2022

Fixes #298
Fixes PolyMC/PolyMC#948. Well, it obviously won't actually close the issue. I went further though - some may say it's too hard to do dumb things now.

  • Confirmation when deleting multiple resources (mods, resource packs, etc.), or if they are folders.
  • Confirmation when deleting servers. Since servers are nothing much more than IP addresses maybe this should only show if it's an IP address rather than a domain.
  • Confirmation when uploading logs and screenshots.
  • Confirmation for removing component customisations either by pressing remove or revert. Some may say this isn't very useful since there's no multi-select.
  • Move a lot of things to trash instead of completely deleting them.
  • Improved consistency.

Unfortunately it still says "may be permanent" which in my opinion is not really great. Perhaps it could be adapted to whether Flatpak is being used, and perhaps the whole dialog logic could be moved into a utility since it's now used a lot.

image

image

image

Signed-off-by: TheKodeToad <TheKodeToad@proton.me>
Signed-off-by: TheKodeToad <TheKodeToad@proton.me>
@Scrumplex Scrumplex added this to the 7.0 milestone Dec 14, 2022
@Scrumplex Scrumplex self-requested a review December 14, 2022 22:13
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.

thanks for the contribution :)

i agree a utility for the confirmation dialogs would be useful here, perhaps somewhere in CustomMessageBox or GuiUtil, possibly with different functions for single item and multiple item confirmations. doesn't necessarily need to be in this PR though

launcher/ui/GuiUtil.cpp Outdated Show resolved Hide resolved
launcher/ui/MainWindow.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/instance/ExternalResourcesPage.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/instance/OtherLogsPage.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/instance/OtherLogsPage.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/instance/ServersPage.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/instance/VersionPage.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/instance/VersionPage.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/instance/WorldListPage.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/instance/ManagedPackPage.cpp Outdated Show resolved Hide resolved
@flowln flowln added the enhancement New feature or request label Dec 16, 2022
@flowln flowln self-requested a review December 17, 2022 13:54
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.

this looks fine to me, thanks!

please add a signoff in f7c5eb1 or make a remediation commit :)

Signed-off-by: TheKodeToad <TheKodeToad@proton.me>
@TheKodeToad
Copy link
Member Author

this looks fine to me, thanks!

please add a signoff in f7c5eb1 or make a remediation commit :)

I've removed them via rebase.

launcher/ui/GuiUtil.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/instance/LogPage.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/instance/WorldListPage.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/instance/VersionPage.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/instance/VersionPage.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/instance/OtherLogsPage.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/instance/ExternalResourcesPage.cpp Outdated Show resolved Hide resolved
launcher/ui/pages/instance/ExternalResourcesPage.cpp Outdated Show resolved Hide resolved
launcher/ui/MainWindow.cpp Outdated Show resolved Hide resolved
launcher/ui/GuiUtil.cpp Outdated Show resolved Hide resolved
You're is used in some other places but im lazy

Signed-off-by: TheKodeToad <TheKodeToad@proton.me>
Signed-off-by: TheKodeToad <TheKodeToad@proton.me>
@Scrumplex
Copy link
Member

Thanks!

@Scrumplex Scrumplex merged commit 6ea1234 into PrismLauncher:develop Dec 26, 2022
TheLastRar added a commit to TheLastRar/PrismLauncher that referenced this pull request Jan 5, 2023
…estructive-actions"

This reverts commit 6ea1234, reversing
changes made to dd3848d.
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.

Confirmation when removing a number of mods give the upload log button a confirmation prompt
3 participants