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

Fix discord "view beatmap" button being shown when editing and hide identifiable information is set #30848

Merged
merged 2 commits into from
Nov 28, 2024

Conversation

Joehuu
Copy link
Member

@Joehuu Joehuu commented Nov 23, 2024

Copy link
Member

@frenzibyte frenzibyte left a comment

Choose a reason for hiding this comment

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

Seems fine otherwise I guess.

if (getBeatmapID(activity.Value) is int beatmapId && beatmapId > 0)
if (getBeatmapID(activity.Value) is int beatmapId
&& beatmapId > 0
&& !(activity.Value is UserActivity.EditingBeatmap && hideIdentifiableInformation))
Copy link
Member

Choose a reason for hiding this comment

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

This is back-to-front coding, UserActivity should expose a method for this rather than hardcoding the types in here. E.g. ShowsBeatmapInfo(bool hideIdentifiableInformation = false)

Copy link
Member

Choose a reason for hiding this comment

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

probably call it CanShowBeatmapInfo or ShouldShowBeatmapInfo, but agree with the change.

Comment on lines +44 to +48
/// <summary>
/// Returns the ID of the beatmap involved in this activity, if applicable and/or available.
/// </summary>
/// <param name="hideIdentifiableInformation"></param>
public virtual int? GetBeatmapID(bool hideIdentifiableInformation = false) => null;
Copy link
Member

Choose a reason for hiding this comment

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

One may say this should not exist in UserActivity itself, but the only other option I can think of is to define an interface that exposes this method and implement such interface on InGame and EditingBeatmap, and I'm smelling some scent of over-engineering in that so I just chose to define it at a base level for now.

@frenzibyte
Copy link
Member

On addressing #30848 (comment), I've had to change my suggestion slightly as it appears we already did pattern matching in DiscordRichPresence to retrieve the beatmap ID, so I added a lookup method for that in UserActivity with the correct behaviour in there.

@frenzibyte frenzibyte requested a review from peppy November 27, 2024 21:58
@peppy peppy merged commit e0fdcaf into ppy:master Nov 28, 2024
9 of 10 checks passed
@Joehuu Joehuu deleted the dicord-fix-beatmap-button-visibility branch November 28, 2024 04:30
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