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 ability to download beatmaps from context menu of beatmap cards #31468

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

peppy
Copy link
Member

@peppy peppy commented Jan 10, 2025

Closes #31151 I guess?

osu.2025-01-10.at.05.52.39.mp4

@@ -24,6 +25,8 @@ public abstract partial class BeatmapCard : OsuClickableContainer, IHasContextMe

protected const float WIDTH = 345;

public CollapsibleButtonContainer ButtonContainer { get; set; } = null!;
Copy link
Contributor

Choose a reason for hiding this comment

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

I really really dislike this. This is how multiplayer ended up in the place it currently is, where random things are exposed for the purpose of inheriting screens setting them that eventually goes completely out of control.

Let's not make the same mistake again.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree it's not 100% optimal, but also can't read your mind 😅.

I also tried an Action RequestDownload kinda flow but it felt way too verbose for what this is. Would that work better?

What's your preference?

Copy link
Member Author

Choose a reason for hiding this comment

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

@smoogipoo prod on this one, need some direction before attempting something else that doesn't work for you.

Copy link
Contributor

@smoogipoo smoogipoo Jan 15, 2025

Choose a reason for hiding this comment

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

Sorry, I thought your reply was in response to the comment below.

Here's two suggestions:

  1. Make ContextMenuItems virtual and duplicate the implementation.
  2. Add abstract DownloadButton? GetDownloadButton() and implement in every place.

var menuItems = new List<MenuItem>();

if (ButtonContainer.DownloadButton.Enabled.Value)
menuItems.Add(new OsuMenuItem(ContextMenuStrings.DownloadBeatmap, MenuItemType.Highlighted, () => ButtonContainer.DownloadButton.TriggerClick()));
Copy link
Member

@Joehuu Joehuu Jan 10, 2025

Choose a reason for hiding this comment

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

Never said this question, but what makes menu items highlighted?

Relevant search: https://github.com/search?q=repo%3Appy%2Fosu%20MenuItemType.Highlighted&type=code

I thought the highlighting was to indicate the left-click behavior, but this makes it more inconsistent right now.

The "play" button on beatmap panels in song select was a precedent to these "view" items that I made. Other menus that don't show the left-click menu item highlight something else.

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.

Downloading beatmaps from the listing is not good/possible on mobile
3 participants