Make behaviour of clickable text in song select consistent #33687
Conversation
|
needs merge conflict resolution |
| // Add another context menu container dedicated to the wedges area, | ||
| // since the LeftSideInteractionContainer layer that's directly behind us | ||
| // blocks right clicks from reaching the outer context menu container. | ||
| new OsuContextMenuContainer | ||
| { | ||
| RelativeSizeAxes = Axes.Both, | ||
| Spacing = new Vector2(0f, 4f), | ||
| Direction = FillDirection.Vertical, | ||
| Children = new Drawable[] | ||
| // Temporarily un-shear here so that context menus don't appear sheared. | ||
| Shear = -OsuGame.SHEAR, | ||
| Child = wedgesContainer = new FillFlowContainer | ||
| { | ||
| new ShearAligningWrapper(titleWedge = new BeatmapTitleWedge()), | ||
| new ShearAligningWrapper(detailsArea = new BeatmapDetailsArea()), | ||
| }, | ||
| RelativeSizeAxes = Axes.Both, | ||
| Spacing = new Vector2(0f, 4f), | ||
| Direction = FillDirection.Vertical, | ||
| // Shear again after un-shearing in parent. | ||
| Shear = OsuGame.SHEAR, | ||
| Children = new Drawable[] | ||
| { | ||
| new ShearAligningWrapper(titleWedge = new BeatmapTitleWedge()), | ||
| new ShearAligningWrapper(detailsArea = new BeatmapDetailsArea()), | ||
| }, | ||
| } |
There was a problem hiding this comment.
this is horrible
I'd much rather have something in ContextMenuContainer that can force the menu to open. previously (that one was about closing the menu, but to me same difference really)
yes it'd be an escape hatch, but to me that's a much less dodgy escape hatch than whatever this is
There was a problem hiding this comment.
As in change ContextMenuContainer so right clicks cannot block opening menus when handled by components inside the hierarchy (i.e. LeftSideInteractionContainer in this case)?
That is a tough framework-side change to approach, and it would be rough to block this PR because of it.
I also can't work around this by letting LeftSideInteractionController let right clicks by, because right click in song select also signifies absolute scrolling, and we don't want to allow absolute scrolling in the wedges area I would imagine. But...maybe it's possible to let right click by but handle drag as well to block it from enabling absolute scroll, I can try that I guess.
There was a problem hiding this comment.
Seems to be an alternative option:
diff --git a/osu.Game/Screens/Select/SongSelect.cs b/osu.Game/Screens/Select/SongSelect.cs
index f923154873..c1d412a48d 100644
--- a/osu.Game/Screens/Select/SongSelect.cs
+++ b/osu.Game/Screens/Select/SongSelect.cs
@@ -1147,7 +1147,11 @@ public LeftSideInteractionContainer(Action resetCarouselPosition)
// as those will usually correspond to other interactions like adjusting volume.
protected override bool OnScroll(ScrollEvent e) => !e.ControlPressed && !e.AltPressed && !e.ShiftPressed && !e.SuperPressed;
- protected override bool OnMouseDown(MouseDownEvent e) => true;
+ // intentionally let right mouse button clicks go through, otherwise context menus cannot be open by components inside the wedge.
+ protected override bool OnMouseDown(MouseDownEvent e) => e.Button == MouseButton.Left;
+
+ // block drags to disable absolute scrolling in the wedges area.
+ protected override bool OnDragStart(DragStartEvent e) => true;
protected override bool OnHover(HoverEvent e)
{
diff --git a/osu.Game/Screens/SelectV2/SongSelect.cs b/osu.Game/Screens/SelectV2/SongSelect.cs
index 37ab0e7afb..744c990317 100644
--- a/osu.Game/Screens/SelectV2/SongSelect.cs
+++ b/osu.Game/Screens/SelectV2/SongSelect.cs
@@ -191,27 +191,16 @@ private void load()
RelativeSizeAxes = Axes.Both,
},
},
- // Add another context menu container dedicated to the wedges area,
- // since the LeftSideInteractionContainer layer that's directly behind us
- // blocks right clicks from reaching the outer context menu container.
- new OsuContextMenuContainer
+ wedgesContainer = new FillFlowContainer
{
RelativeSizeAxes = Axes.Both,
- // Temporarily un-shear here so that context menus don't appear sheared.
- Shear = -OsuGame.SHEAR,
- Child = wedgesContainer = new FillFlowContainer
+ Spacing = new Vector2(0f, 4f),
+ Direction = FillDirection.Vertical,
+ Children = new Drawable[]
{
- RelativeSizeAxes = Axes.Both,
- Spacing = new Vector2(0f, 4f),
- Direction = FillDirection.Vertical,
- // Shear again after un-shearing in parent.
- Shear = OsuGame.SHEAR,
- Children = new Drawable[]
- {
- new ShearAligningWrapper(titleWedge = new BeatmapTitleWedge()),
- new ShearAligningWrapper(detailsArea = new BeatmapDetailsArea()),
- },
- }
+ new ShearAligningWrapper(titleWedge = new BeatmapTitleWedge()),
+ new ShearAligningWrapper(detailsArea = new BeatmapDetailsArea()),
+ },
},
}
},
There was a problem hiding this comment.
As in change
ContextMenuContainerso right clicks cannot block opening menus when handled by components inside the hierarchy (i.e.LeftSideInteractionContainerin this case)?
no, as in have a ContextMenuContainer.OpenMenu() method or similar
Seems to be an alternative option:
I don't think this is gonna work on mobile
| new OsuMenuItem(ContextMenuStrings.ViewProfile, MenuItemType.Standard, () => linkHandler?.HandleLink(new LinkDetails(LinkAction.OpenUserProfile, user!))), | ||
| new OsuMenuItem(ContextMenuStrings.SearchOnline, MenuItemType.Standard, () => linkHandler?.HandleLink(new LinkDetails(LinkAction.OpenUserProfile, user!))), |
There was a problem hiding this comment.
why do these have the same action twice?
There was a problem hiding this comment.
whoops, the second one is supposed to be searching in beatmap listing.
| public float MaxWidth | ||
| { | ||
| set => text.MaxWidth = value; | ||
| } |
There was a problem hiding this comment.
good find, it's supposed to be used in MetadataDisplay.
There was a problem hiding this comment.
- why have a method called
SetHyphen()? what on earth is the meaning of "setting a hyphen". call itSetNone()orClear()or whatever, except for - why does
SetHyphen()even exist ifSetText(null)does the same thing - oh god there are more of these methods?
SetUser()SetDate()? what does all of this achieve? use composition or something, so that you can setChildof this normally rather than do this weird polymorphism of this being a display of anything and everything. the only thing that makes some semblance of sense of all this isSetLoading()because at least that's somewhat uniform handling
There was a problem hiding this comment.
I can rename SetHyphen to SetNone, but I find it more clear than SetText(null), I would rather want to see less nulls used in the code, it feels ambiguous as to what it means when not enough context is given (i.e. SetText(nullableField) vs SetText(null)).
I'm not sure I follow what you're proposing by using composition or such, I find it much easier to have a component (MetadataDisplay) which allows displaying different types of drawables based on data given, rather than invent N number of classes with each having loading layer support and extra code copied across.
Now that the structure has long since persisted in the code base, I think now is a good time to address this.
I've took the time to refactor
MetadataDisplaycompletely and make it more explicit than it usually was written (wherenullvalues was arbitrarily interpreted as sometimes loading and sometimes n/a). The new structure should read better, and as a result, simplify all external usages of the component.