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

Do not add checkbox padding to the left of menu items if no item actually needs it #30101

Merged
merged 2 commits into from
Oct 4, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Oct 3, 2024

RFC. As per https://discord.com/channels/188630481301012481/188630652340404224/1291346164976980009.

2024-10-03.15-34-24.mp4

The diff is extremely dumb (linq in update method) but I tried a few smarter methods and they're either not fully correct or don't work. Primary problem is that menu items are mutable externally and there's no hook provided by the framework to know that items changed. One could probably be made but I'd prefer that this change be examined visually first before I engage in too much ceremony and start changing framework around. Measured overhead in profiling is ~16us per call.

…ally needs it

RFC. As per
https://discord.com/channels/188630481301012481/188630652340404224/1291346164976980009.

The diff is extremely dumb but I tried a few smarter methods and they're
either not fully correct or don't work. Primary problem is that menu
items are mutable externally and there's no hook provided by the
framework to know that items changed. One could probably be made but I'd
prefer that this change be examined visually first before I engage in
too much ceremony and start changing framework around.
{
base.Update();

bool showCheckboxes = Items.Any(i => i is StatefulMenuItem);
Copy link
Member

Choose a reason for hiding this comment

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

The unfortunate truth is that this allocates out the rear:

JetBrains Rider-EAP 2024-10-04 at 08 50 38

I'm fine with a non-LINQ version though:

diff --git a/osu.Game/Graphics/UserInterface/OsuMenu.cs b/osu.Game/Graphics/UserInterface/OsuMenu.cs
index 719100e138..6e7dad2b5f 100644
--- a/osu.Game/Graphics/UserInterface/OsuMenu.cs
+++ b/osu.Game/Graphics/UserInterface/OsuMenu.cs
@@ -3,7 +3,6 @@
 
 #nullable disable
 
-using System.Linq;
 using osu.Framework.Allocation;
 using osu.Framework.Audio;
 using osu.Framework.Audio.Sample;
@@ -47,10 +46,19 @@ protected override void Update()
         {
             base.Update();
 
-            bool showCheckboxes = Items.Any(i => i is StatefulMenuItem);
+            bool showCheckboxes = false;
 
-            foreach (var drawableItem in ItemsContainer.OfType<DrawableOsuMenuItem>())
-                drawableItem.ShowCheckbox.Value = showCheckboxes;
+            foreach (var drawableItem in ItemsContainer)
+            {
+                if (drawableItem.Item is StatefulMenuItem)
+                    showCheckboxes = true;
+            }
+
+            foreach (var drawableItem in ItemsContainer)
+            {
+                if (drawableItem is DrawableOsuMenuItem osuItem)
+                    osuItem.ShowCheckbox.Value = showCheckboxes;
+            }
         }
 
         protected override void AnimateOpen()

@peppy peppy added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Oct 4, 2024
@bdach bdach merged commit 96b6780 into ppy:master Oct 4, 2024
11 of 13 checks passed
@bdach bdach deleted the menu-padding-left branch October 4, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/S type:cosmetic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants