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 support for displaying "mapper" badge in comment section #27164

Merged
merged 9 commits into from Feb 14, 2024

Conversation

frenzibyte
Copy link
Member

For a quick summary on the interaction between the client and the API on determining the "mapper" badge, there exists a property in each comment bundle named commentable_meta. The property contains information about the page which the user is commenting on ("commentable object"). The field owner_id defines the user which owns the page (e.g. the mapper in a beatmap page), and the field owner_title which has the title given to the user (e.g. "mapper").

Interestingly, the commentable_meta property is actually a list of potentially multiple CommentableMeta elements, instead of a singular one. I've inquired about this in discord and it seems there is somewhat of a technical reason behind it? Either way, I've went with a sane approach of finding the most relevant CommentableMeta by comparing against the commentable ID associated with the comment (which sort of matches web?).

The part I'm most unsure of is the structure of passing the CommentableMeta data. I've went with the simplest approach of passing it via constructor from CommentableContainer down to each Comment, adding an extra parameter in each GetDrawableComment call. Open to feedback on any other method to better approach this.

@bdach
Copy link
Collaborator

bdach commented Feb 14, 2024

Have applied changes. Mostly minor, up until 68247fa - typo in json property name meant that this would have never actually worked outside of tests because

var ownerMeta = meta.FirstOrDefault(m => m.Id == comment.CommentableId && m.Type == comment.CommentableType);

would fail to match due to null commentable_type on one side of the comparison (which shows that there was clearly no attempt to test in the real world).

@bdach bdach enabled auto-merge February 14, 2024 12:28
@bdach bdach merged commit f1363dc into ppy:master Feb 14, 2024
10 of 11 checks passed
@@ -14,18 +14,18 @@ namespace osu.Game.Tests.Visual.UserInterface
{
public abstract partial class ThemeComparisonTestScene : OsuGridTestScene
{
private readonly bool showNoColourProvider;
private readonly bool showWithoutColourProvider;
Copy link
Member Author

Choose a reason for hiding this comment

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

Oh haha, this is the name I chose initially for the variable. It read slightly weird to me so I replaced Without with No.

@frenzibyte frenzibyte deleted the comment-mapper-pill branch February 14, 2024 22:23
@frenzibyte
Copy link
Member Author

(which shows that there was clearly no attempt to test in the real world).

Kindly stop, I literally tested this on production. It was only the fact that I had altered the conditions of querying the metadata on the last minutes of opening a PR as I was checking against web (read second paragraph of #27164 (comment)). Saying "clearly no attempt" is straight up an insult to me, I'm sure you know that.

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.

Mapper pill is missing in-game
2 participants