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

Implement basic bookmark support in editor #30960

Merged
merged 4 commits into from
Dec 10, 2024
Merged

Implement basic bookmark support in editor #30960

merged 4 commits into from
Dec 10, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Dec 3, 2024

2024-12-03.15-21-03.mp4

RFC.

Move bookmarks out of BeatmapInfo

Not sure why I didn't do that in #28473...

Implement basic bookmark support in editor

Should work, but is a bit touch-and-go:

  • Bookmarks only display on bottom timeline. I thought a bit about having them show on the top timeline too, but I'm not sure I can fit it in there and not make the timeline even more oversaturated with info than it already is.
  • I added the same bookmark hotkeys that are in stable, but two of them are not bound, and that's seek to previous/next bookmark - that is because their binding is taken by seeking to previous/next object now:
    new KeyBinding(new[] { InputKey.Control, InputKey.Left }, GlobalAction.EditorSeekToPreviousHitObject),
    new KeyBinding(new[] { InputKey.Control, InputKey.Right }, GlobalAction.EditorSeekToNextHitObject),
  • The only exposed way to mutate the bookmarks from the UI that is not the hotkey is in the timing menu. That's different from stable where there are bookmark controls near the bottom timeline, but again, not sure how to make that happen without cluttering things further.
  • Not sure about using red (dangerous) stuff in the top menu. Might need a colour change at the least.

I'm hoping this will be considered as an ok baseline to build upon at least, but am open to adjustments as necessary. To that end I didn't even try to write tests for this, until the shape has solidified; once that happens, I will consider adding test coverage.

@bdach bdach added area:editor next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! labels Dec 3, 2024
@peppy
Copy link
Member

peppy commented Dec 3, 2024

Ctrl+Shift+Left or Ctrl+Alt+Left should work for bindings I think.

Not sure about this being in the "timing" menu, wouldn't be the first place I'd look for bookmark controls. Still thinking about where would work best though.

@frenzibyte
Copy link
Member

Never been an editor user, but considering the functionality I've seen from the video, I find "View" to be a good initial place for it as opposed to "Timing". Although it's a bit too crowded in that tab...

Randomly looking through Safari's menu tabs inspired me to say, maybe "Bookmarks" deserves its own tab in the top-level menu? It's unique from anything else, it's not purely visual to be in "View", nor does it bare any perceivable changes to beatmap timing to be in a tab named "Timing", so maybe it should just be its own tab item in the top menu?

@peppy peppy self-requested a review December 6, 2024 11:24
@bdach
Copy link
Collaborator Author

bdach commented Dec 7, 2024

Ctrl+Shift+Left or Ctrl+Alt+Left should work for bindings I think.

Doesn't appear to fail relevant tests if I checked them all, so applied in dc4a51b.

Not sure about this being in the "timing" menu, wouldn't be the first place I'd look for bookmark controls. Still thinking about where would work best though.

Was best place I could think of that already exists without inventing a new one.

Never been an editor user, but considering the functionality I've seen from the video, I find "View" to be a good initial place for it as opposed to "Timing". Although it's a bit too crowded in that tab...

Pretty opposed to this, it makes no sense to put it in view. View is for things that affect the visual appearance of the editor, not for actual things that are saved to a beatmap etc.

@peppy
Copy link
Member

peppy commented Dec 10, 2024

As mentioned IRL, probably change this to use alt-left/right as the new binding choice conflicts with seeking between sample points on hitobjects.

2024-12-10 13 56 21@2x

@peppy peppy merged commit 48bf2fa into ppy:master Dec 10, 2024
9 of 10 checks passed
@bdach bdach deleted the bookmarks branch December 20, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:editor next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants