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

Allow copying beatmap link in song select carousel context menu #29537

Conversation

normalid-awa
Copy link
Contributor

@normalid-awa normalid-awa commented Aug 20, 2024

Why

This could be very useful when playing on mobile, which player won't need to open their browser to just get the link. At the other side, desktop players can reduce the step of getting URLs, they don't need to jump to the browser and copy the link, instead just do it inside the game

Though the /np command can send the current beatmap to the chat, it doesn't make a use on if we want to share through the other app, like if we want to send it to discord.

Implementation

Add Copy link item into both DrawableCarouselBeatmapSet and DrawableCarouselBeatmap
While DrawableCarouselBeatmapSet Copy URL as $@"{api.WebsiteRootUrl}/beatmapsets/{beatmapSet.OnlineID}#{ruleset.ShortName}" and DrawableCarouselBeatmap copy as $@"{api.WebsiteRootUrl}/beatmapsets/{beatmapSet.OnlineID}#{ruleset.ShortName}/{beatmapInfo.OnlineID}"

Screenshot 2024-08-21 at 11 37 17 AM

@Joehuu
Copy link
Member

Joehuu commented Aug 20, 2024

Just to list some other methods of doing the same thing in-game:

  • Details... -> right click external link icon -> Copy URL
    • downsides: two extra steps and icon is potentially too small for mobile
  • /np in chat, if the intention is sending the link there
    • downsides: the command is not known by many (probably would be noticed if there's some kind of UI when typing / like discord)

Note that the above only works while logged in, so that's probably another argument.

@normalid-awa
Copy link
Contributor Author

Just to list some other methods of doing the same thing in-game:

  • Details... -> right click external link icon -> Copy URL

    • downsides: two extra steps and icon is potentially too small for mobile
  • /np in chat, if the intention is sending the link there

    • downsides: the command is not known by many (probably would be noticed if there's some kind of UI when typing / like discord)

Note that the above only works while logged in, so that's probably another argument.

Updated the description for better clarity

@bdach
Copy link
Collaborator

bdach commented Aug 21, 2024

Normally I'd reject this based on the premise that you can already do this in other ways as @Joehuu said, but the number of reactions on the OP is giving me a little pause. I'm not sure whether people didn't know you could do this already (albeit, admittedly, in a more roundabout manner), or just don't think that flow is good enough.

@peppy
Copy link
Sponsor Member

peppy commented Aug 21, 2024

I think this is handy to have here. Other methods are hidden or require more steps.

@peppy peppy self-requested a review August 21, 2024 16:43
@peppy
Copy link
Sponsor Member

peppy commented Aug 21, 2024

That said, this PR is basically a "needs full rewrite".

@pull-request-size pull-request-size bot added size/M and removed size/S labels Aug 21, 2024
peppy
peppy previously approved these changes Aug 21, 2024
Copy link
Sponsor Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Probably fine. Batch localisation should probably happen for context menus at some point.

@peppy peppy requested a review from bdach August 21, 2024 17:18
@Joehuu Joehuu self-requested a review August 21, 2024 18:24
@Joehuu Joehuu changed the title Allow copying url in song select carousel context menu Allow copying beatmap link in song select carousel context menu Aug 21, 2024
@@ -288,6 +296,9 @@ public MenuItem[] ContextMenuItems

items.Add(new OsuMenuItem("Collections") { Items = collectionItems });

if (beatmapInfo.GetOnlineURL(api) is string url)
items.Add(new OsuMenuItem("Copy link", MenuItemType.Standard, () => clipboard.SetText(url)));
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't have the OSD feedback like in ExternalLinkButton.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May I ask about what the OSD stands for?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

You can google for an explanation. But please leave this alone, I'll fix it to keep things moving.

Copy link
Member

Choose a reason for hiding this comment

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

OnScreenDisplay, but I dunno if it's better to copy the logic from ExternalLinkButton two more times, or add another method to OsuGame.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Latter is more valid. I believe we will be using this in many more places going forward.

@peppy peppy self-requested a review August 22, 2024 04:20
@@ -288,6 +295,9 @@ public MenuItem[] ContextMenuItems

items.Add(new OsuMenuItem("Collections") { Items = collectionItems });

if (beatmapInfo.GetOnlineURL(api) is string url)
Copy link
Collaborator

Choose a reason for hiding this comment

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

No ruleset spec here? There is one on the beatmap set panels. And it does matter for stuff like converts.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Yeah right, this was on my todo and I forgot to act on it.

@bdach bdach merged commit bd6943e into ppy:master Aug 22, 2024
13 checks passed
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.

4 participants