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

Added warning before bookmark reset #31780

Merged
merged 3 commits into from
Feb 5, 2025
Merged

Added warning before bookmark reset #31780

merged 3 commits into from
Feb 5, 2025

Conversation

necocat0918
Copy link
Contributor

cc @bdach

@bdach
Copy link
Collaborator

bdach commented Feb 3, 2025

Kinda not sure we want this. Yes it's easy to delete all bookmarks, but undo works with that operation. So you can just ctrl-z, rather than have this gated behind a dialog...

@peppy do you have an opinion here?

@necocat0918
Copy link
Contributor Author

I thought undo isn't possible for bookmarks, you can close this if you don't want this

@peppy
Copy link
Member

peppy commented Feb 5, 2025

I think it's fine to have this, even if it's undoable. Should be used so rarely that the overhead of confirmation is fairly low.

@peppy peppy self-requested a review February 5, 2025 05:44
@peppy
Copy link
Member

peppy commented Feb 5, 2025

Some unrelated follow-up work I'd see eventually happening around bookmarks:

  • Menu items should be disabled when there are no bookmarks to delete / reset
  • Fix proximity deletion as discussed in a recent issue thread

This seems fine for now though 👍

@peppy
Copy link
Member

peppy commented Feb 5, 2025

@necocat0918 please don't merge master into your PR, it's unnecessary. also please choose better branch names if you're contributing again.

@peppy peppy merged commit 8c5b19d into ppy:master Feb 5, 2025
8 of 9 checks passed
@necocat0918 necocat0918 deleted the pr branch February 5, 2025 07:03
@bdach bdach mentioned this pull request Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants