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

Conversation

normalid-awa
Copy link
Contributor

@normalid-awa normalid-awa commented Aug 4, 2024

Closed #29132

Implementation:

  • Expose CloseMenu method in OsuContextMenuContainer
  • Cache OsuContextMenuContainer At the SkinEditor and Editor
  • Resolve OsuContextMenuContainer in SelectionHandler
  • Call OsuContextMenuContainer.CloseMenu method when SelectionHandler.DeselectAll was called

Video

Beatmap editor:

2024-08-04.20-00-52.mp4

Skin editor:

2024-08-04.20-01-40.mp4

@normalid-awa normalid-awa changed the title Make the OsuContextMenu close when deselected the items. Make the OsuContextMenu close when deselecting items in editor Aug 4, 2024
@normalid-awa normalid-awa marked this pull request as ready for review August 4, 2024 13:18
Comment on lines +237 to +241
protected void DeselectAll()
{
SelectedItems.Clear();
ContextMenuContainer?.CloseMenu();
}
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();
        }

@pull-request-size pull-request-size bot removed the size/L label Aug 6, 2024
@smoogipoo
Copy link
Contributor

Just cleaned things up a bit for review. Lgtm. Requesting another reviewer.

I noticed one problem with context menus in general - if a mouse down is handled inside them then the menu isn't closed. It's an issue here because BlueprintContainer handles it, but it can't be fixed in this PR (and is likely an o!f issue):

protected override bool OnMouseDown(MouseDownEvent e)
{
bool selectionPerformed = performMouseDownActions(e);
bool movementPossible = prepareSelectionMovement(e);
// check if selection has occurred
if (selectionPerformed)
{
// only unmodified right click should show context menu
bool shouldShowContextMenu = e.Button == MouseButton.Right && !e.ShiftPressed && !e.AltPressed && !e.SuperPressed;
// stop propagation if not showing context menu
return !shouldShowContextMenu;
}
// even if a selection didn't occur, a drag event may still move the selection.
return e.Button == MouseButton.Left && movementPossible;
}

smoogipoo
smoogipoo previously approved these changes Aug 6, 2024
@smoogipoo smoogipoo requested a review from a team August 6, 2024 07:13
@bdach bdach changed the title Make the OsuContextMenu close when deselecting items in editor Close context menus when deselecting items in editor Aug 7, 2024
@bdach bdach self-requested a review August 7, 2024 10:50
@@ -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.

@bdach bdach merged commit 437812e into ppy:master Aug 7, 2024
11 of 13 checks passed
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.

Editor context menu persists after object is deleted
3 participants