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

Close context menus when deselecting items in editor #29279

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
de6d8e7
Add the custom context menu to handle the key event
normalid-awa Aug 4, 2024
83aeb27
Replace original menu container to the custom one
normalid-awa Aug 4, 2024
1ff0c7c
Replace original menu container with custom one
normalid-awa Aug 4, 2024
5c5fcd7
Allow key event pass through selection handler
normalid-awa Aug 4, 2024
27d6c4c
Implement on beatmap editor
normalid-awa Aug 4, 2024
3cc5466
Refactor the code to follow IoC principle and more flexible
normalid-awa Aug 4, 2024
5d31171
Fix code quality
normalid-awa Aug 4, 2024
7c83d6a
Cleanup code
normalid-awa Aug 4, 2024
2145368
Merge `EditorContextMenuContainer` into `OsuContextMenuContainer`
normalid-awa Aug 4, 2024
38dacfe
Fix unit test
normalid-awa Aug 4, 2024
7cebf4c
Fix code quality
normalid-awa Aug 4, 2024
b32d97b
Remove decreapted property
normalid-awa Aug 4, 2024
2720bcf
Fix ruleset unit test
normalid-awa Aug 4, 2024
1b25633
Fix headless test
normalid-awa Aug 4, 2024
2098fb8
Fix code quality
normalid-awa Aug 4, 2024
6d385c6
Remove the meaningless `OpenMenu` method
normalid-awa Aug 5, 2024
75c0c6a
Make the `OsuContextMenu` nullable in `SelectionHandler`
normalid-awa Aug 5, 2024
3c8d0ce
Revert the unit test changes
normalid-awa Aug 5, 2024
59ff549
Remove unused using
normalid-awa Aug 5, 2024
22ab6f5
Add back the sample into `OsuContextMenu`
normalid-awa Aug 6, 2024
cb877b7
Close the menu when selecting other object
normalid-awa Aug 6, 2024
b91461e
Refactor + CI fixes
smoogipoo Aug 6, 2024
c574551
Simplify caching
smoogipoo Aug 6, 2024
c26a664
Use InternalChild directly
smoogipoo Aug 6, 2024
41d84ea
Revert all SkinEditor changes (none required)
smoogipoo Aug 6, 2024
aae49d3
Fix unit test code quality
normalid-awa Aug 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions osu.Game.Rulesets.Osu.Tests/Editor/TestSceneObjectMerging.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public void TestSimpleMerge()
});

moveMouseToHitObject(1);
AddAssert("merge option available", () => selectionHandler.ContextMenuItems?.Any(o => o.Text.Value == "Merge selection") == true);
AddAssert("merge option available", () => selectionHandler.ContextMenuItems.Any(o => o.Text.Value == "Merge selection"));

mergeSelection();

Expand Down Expand Up @@ -198,7 +198,7 @@ public void TestIllegalMerge()
});

moveMouseToHitObject(1);
AddAssert("merge option not available", () => selectionHandler.ContextMenuItems?.Length > 0 && selectionHandler.ContextMenuItems.All(o => o.Text.Value != "Merge selection"));
AddAssert("merge option not available", () => selectionHandler.ContextMenuItems.Length > 0 && selectionHandler.ContextMenuItems.All(o => o.Text.Value != "Merge selection"));
mergeSelection();
AddAssert("circles not merged", () => circle1 is not null && circle2 is not null
&& EditorBeatmap.HitObjects.Contains(circle1) && EditorBeatmap.HitObjects.Contains(circle2));
Expand All @@ -222,7 +222,7 @@ public void TestSameStartTimeMerge()
});

moveMouseToHitObject(1);
AddAssert("merge option available", () => selectionHandler.ContextMenuItems?.Any(o => o.Text.Value == "Merge selection") == true);
AddAssert("merge option available", () => selectionHandler.ContextMenuItems.Any(o => o.Text.Value == "Merge selection"));

mergeSelection();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,7 @@ public TestSceneHitObjectComposerDistanceSnapping()
[SetUp]
public void Setup() => Schedule(() =>
{
Children = new Drawable[]
{
composer = new TestHitObjectComposer()
};
Child = composer = new TestHitObjectComposer();

BeatDivisor.Value = 1;

Expand Down
10 changes: 9 additions & 1 deletion osu.Game/Graphics/Cursor/OsuContextMenuContainer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,24 @@

namespace osu.Game.Graphics.Cursor
{
[Cached(typeof(OsuContextMenuContainer))]
public partial class OsuContextMenuContainer : ContextMenuContainer
{
[Cached]
private OsuContextMenuSamples samples = new OsuContextMenuSamples();

private OsuContextMenu menu = null!;

public OsuContextMenuContainer()
{
AddInternal(samples);
}

protected override Menu CreateMenu() => new OsuContextMenu(true);
protected override Menu CreateMenu() => menu = new OsuContextMenu(true);

public void CloseMenu()
{
menu.Close();
}
}
}
22 changes: 15 additions & 7 deletions osu.Game/Screens/Edit/Compose/Components/SelectionHandler.cs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

#nullable disable

using System;
using System.Collections.Generic;
using System.Linq;
Expand All @@ -16,6 +14,7 @@
using osu.Framework.Input;
using osu.Framework.Input.Bindings;
using osu.Framework.Input.Events;
using osu.Game.Graphics.Cursor;
using osu.Game.Graphics.UserInterface;
using osu.Game.Input.Bindings;
using osu.Game.Resources.Localisation.Web;
Expand Down Expand Up @@ -50,14 +49,17 @@ public abstract partial class SelectionHandler<T> : CompositeDrawable, IKeyBindi

private readonly List<SelectionBlueprint<T>> selectedBlueprints;

protected SelectionBox SelectionBox { get; private set; }
protected SelectionBox SelectionBox { get; private set; } = null!;

[Resolved(CanBeNull = true)]
protected IEditorChangeHandler ChangeHandler { get; private set; }
protected IEditorChangeHandler? ChangeHandler { get; private set; }

public SelectionRotationHandler RotationHandler { get; private set; } = null!;

public SelectionRotationHandler RotationHandler { get; private set; }
public SelectionScaleHandler ScaleHandler { get; private set; } = null!;

public SelectionScaleHandler ScaleHandler { get; private set; }
[Resolved(CanBeNull = true)]
protected OsuContextMenuContainer? ContextMenuContainer { get; private set; }

protected SelectionHandler()
{
Expand Down Expand Up @@ -230,7 +232,11 @@ public void OnReleased(KeyBindingReleaseEvent<PlatformAction> e)
/// <summary>
/// Deselect all selected items.
/// </summary>
protected void DeselectAll() => SelectedItems.Clear();
protected void DeselectAll()
{
SelectedItems.Clear();
ContextMenuContainer?.CloseMenu();
}
Comment on lines +235 to +239
Copy link
Contributor

Choose a reason for hiding this comment

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

One situation I'm not sure about is that pasting doesn't close the menu even though it selects the newly-pasted hitobjects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One situation I'm not sure about is that pasting doesn't close the menu even though it selects the newly-pasted hitobjects.

close the menu whatever the selected object changed

        internal virtual void HandleSelected(SelectionBlueprint<T> blueprint)
        {
            // there are potentially multiple SelectionHandlers active, but we only want to add items to the selected list once.
            if (!SelectedItems.Contains(blueprint.Item))
                SelectedItems.Add(blueprint.Item);

            selectedBlueprints.Add(blueprint);

+            ContextMenuContainer?.CloseMenu();
        }


/// <summary>
/// Handle a blueprint becoming selected.
Expand All @@ -243,6 +249,8 @@ internal virtual void HandleSelected(SelectionBlueprint<T> blueprint)
SelectedItems.Add(blueprint.Item);

selectedBlueprints.Add(blueprint);

ContextMenuContainer?.CloseMenu();
}

/// <summary>
Expand Down
15 changes: 12 additions & 3 deletions osu.Game/Screens/Edit/Editor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using JetBrains.Annotations;
using osu.Framework;
Expand Down Expand Up @@ -159,7 +160,7 @@ public bool ReadyForUse

private string lastSavedHash;

private Container<EditorScreen> screenContainer;
private ScreenContainer screenContainer;

[CanBeNull]
private readonly EditorLoader loader;
Expand Down Expand Up @@ -329,7 +330,7 @@ private void load(OsuConfigManager config)
Name = "Screen container",
RelativeSizeAxes = Axes.Both,
Padding = new MarginPadding { Top = 40, Bottom = 50 },
Child = screenContainer = new Container<EditorScreen>
Child = screenContainer = new ScreenContainer
{
RelativeSizeAxes = Axes.Both,
}
Expand Down Expand Up @@ -422,6 +423,7 @@ private void load(OsuConfigManager config)
MutationTracker,
}
});

changeHandler?.CanUndo.BindValueChanged(v => undoMenuItem.Action.Disabled = !v.NewValue, true);
changeHandler?.CanRedo.BindValueChanged(v => redoMenuItem.Action.Disabled = !v.NewValue, true);

Expand Down Expand Up @@ -1007,7 +1009,7 @@ private void onModeChanged(ValueChangedEvent<EditorScreenMode> e)
throw new InvalidOperationException("Editor menu bar switched to an unsupported mode");
}

LoadComponentAsync(currentScreen, newScreen =>
screenContainer.LoadComponentAsync(currentScreen, newScreen =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's this change doing? @smoogipoo looks like one of yours?

I'm not sure it's a big deal but I'm weirded out by it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Previously, the context menu container was stored as a local and cached in this class. Now, the context menu container caches itself via [Cached] on the type itself.

But this moves the dependencies one level lower from Editor itself, and previously this line would load the screen with the dependencies of the Editor rather than the expected lower level.

{
if (newScreen == currentScreen)
{
Expand Down Expand Up @@ -1385,5 +1387,12 @@ public BeatmapEditorToast(LocalisableString value, string beatmapDisplayName)
{
}
}

private partial class ScreenContainer : Container<EditorScreen>
{
public new Task LoadComponentAsync<TLoadable>([NotNull] TLoadable component, Action<TLoadable> onLoaded = null, CancellationToken cancellation = default, Scheduler scheduler = null)
where TLoadable : Drawable
=> base.LoadComponentAsync(component, onLoaded, cancellation, scheduler);
}
}
}
Loading