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 searching by clicking title/artist in beatmap overlay not following original language setting #29913

Merged
merged 5 commits into from
Sep 27, 2024

Conversation

Joehuu
Copy link
Member

@Joehuu Joehuu commented Sep 18, 2024

May also be used in song select title/artist context menus (left-click already searches in song select locally on v2 component).

osu.Game/OsuGame.cs Outdated Show resolved Hide resolved
@bdach
Copy link
Collaborator

bdach commented Sep 18, 2024

In general my primary question would be why this entire PR is what it is and not something like

diff --git a/osu.Game/OsuGame.cs b/osu.Game/OsuGame.cs
index 0ef6a94679..1af86b2d83 100644
--- a/osu.Game/OsuGame.cs
+++ b/osu.Game/OsuGame.cs
@@ -445,10 +445,15 @@ private void load()
                     break;
 
                 case LinkAction.SearchBeatmapSet:
-                    if (link.Argument is RomanisableString romanisable)
-                        SearchBeatmapSet(romanisable.GetPreferred(Localisation.CurrentParameters.Value.PreferOriginalScript));
+                    if (link.Argument is LocalisableString localisable)
+                    {
+                        var localised = Localisation.GetLocalisedBindableString(localisable);
+                        SearchBeatmapSet(localised.Value);
+                        localised.UnbindAll();
+                    }
                     else
                         SearchBeatmapSet(argString);
+
                     break;
 
                 case LinkAction.FilterBeatmapSetGenre:
diff --git a/osu.Game/Overlays/BeatmapSet/BeatmapSetHeaderContent.cs b/osu.Game/Overlays/BeatmapSet/BeatmapSetHeaderContent.cs
index f9e0c6c380..a50043f0f0 100644
--- a/osu.Game/Overlays/BeatmapSet/BeatmapSetHeaderContent.cs
+++ b/osu.Game/Overlays/BeatmapSet/BeatmapSetHeaderContent.cs
@@ -242,7 +242,7 @@ private void load(OverlayColourProvider colourProvider)
                     title.Clear();
                     artist.Clear();
 
-                    title.AddLink(titleText, LinkAction.SearchBeatmapSet, $@"title=""""{titleText}""""");
+                    title.AddLink(titleText, LinkAction.SearchBeatmapSet, LocalisableString.Interpolate($@"title=""""{titleText}"""""));
 
                     title.AddArbitraryDrawable(Empty().With(d => d.Width = 5));
                     title.AddArbitraryDrawable(externalLink = new ExternalLinkButton());
@@ -259,7 +259,7 @@ private void load(OverlayColourProvider colourProvider)
                         title.AddArbitraryDrawable(new SpotlightBeatmapBadge());
                     }
 
-                    artist.AddLink(artistText, LinkAction.SearchBeatmapSet, $@"artist=""""{artistText}""""");
+                    artist.AddLink(artistText, LinkAction.SearchBeatmapSet, LocalisableString.Interpolate($@"artist=""""{artistText}"""""));
 
                     if (setInfo.NewValue.TrackId != null)
                     {

which just gets rid of the weird LinkAction splittage.

(The .UnbindAll() call is weird but required to prevent leaks because non-bindable GetLocalisedString() is internal on LocalisationManager. Not sure it should be.)

@Joehuu
Copy link
Member Author

Joehuu commented Sep 18, 2024

Just forgot GetLocalisedBindableString() exists, have applied.

bdach added a commit to bdach/osu-framework that referenced this pull request Sep 19, 2024
The initial rationale for it being internal was likely that there was
never going to be any meaningful external usage of it, but as it turns
out, there are cases where you want this, one of them being
ppy/osu#29913 - without having the non-bindable
variant exposed, weird games of taking out the bindable and then
manually unbinding it immediately after use have to be played to avoid
reference leaks.

The pre-existing xmldoc should do the job when it comes to warning users
that the method will likely not do what they want it to in most cases.
@bdach
Copy link
Collaborator

bdach commented Sep 19, 2024

FWIW, I would like to get rid of the weird unbind via ppy/osu-framework#6377.

@peppy peppy self-requested a review September 27, 2024 08:39
@peppy peppy force-pushed the fix-romanised-searching branch from c38c6ef to 371cee1 Compare September 27, 2024 08:42
@peppy peppy enabled auto-merge September 27, 2024 08:44
@peppy peppy disabled auto-merge September 27, 2024 09:39
@peppy peppy merged commit a358731 into ppy:master Sep 27, 2024
11 of 13 checks passed
@Joehuu Joehuu deleted the fix-romanised-searching branch September 27, 2024 17:44
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