Fix beatmap links not truncating correctly on playlists/multiplayer #25396
Conversation
| @@ -17,7 +17,7 @@ public partial class OsuHoverContainer : OsuClickableContainer | |||
|
|
|||
| protected Color4 HoverColour; | |||
|
|
|||
| protected Color4 IdleColour = Color4.White; | |||
| public Color4 IdleColour = Color4.White; | |||
There was a problem hiding this comment.
If going to do this, better make sure that the change of it takes effect immediately when any consumer sets it. I don't see this happening in this diff.
| // best effort to confine the auto-sized text to parent bounds | ||
| // TODO: remove when text/link flow can support truncation with ellipsis natively. | ||
| beatmapText.MaxWidth = mainFillFlow.DrawWidth; |
There was a problem hiding this comment.
Can this sort of thing go live as a default in TruncatingSpriteText? Sprinkling it everywhere that we've got this class already seems... like a waste of effort.
I know the wedge usage is slightly different but maybe it can be padded differently to achieve the same effect?
There was a problem hiding this comment.
I had a normalising suggestion when I was implementing the truncated text tooltips (also with RelativeSizeAxes removal):
I also tried centralising the
MaxWidthandRelativeSizeAxes = Axes.Xassigning toTruncatingSpriteText(Joehuu@dd8e1ba), but not sure about the solution as I'm working around a framework limitation and probably should be done more directly inSpriteText.
But now links are in the mix, and here's what my updated solution is (using ChildSize.X and omited X and the RelativeSizeAxes removal for now): e45a2b2
May need to incorporate the RelativeSizeAxes removal as it makes MaxWidth a relative value.
Or just isolate the non-link text and make a TruncatingLinkSpriteText : OsuHoverContainer but then I have to port all the OsuSpriteText properties or make it public...
There was a problem hiding this comment.
What is even the framework limitation being worked around here?
There was a problem hiding this comment.
Not sure what I was saying at the time, but probably putting said logic framework-side in a proper way (through the use of properties) so framework consumers can use it and there is no usage of Update() game-side, but probably not feasible at the moment.
Early returns were never hit.
There is already a truncating visual test for `DrawableRoom` and `DrawableRoomPlaylistItem`, but they are currently broken due to `BeatmapLookupCache` logic.
|
Updated this as there were conflicts. |
|
Updated to fix a compile error with master and remove the changes of |
There was a problem hiding this comment.
I don't know what's going on here but the entirety of the changes in this file just put me off wanting to review this instantly. The getFirstNonAutoSizedWidthAncestor() upwards traversal, the three obsoleted new members, all of this looks way too cursed for my pain threshold.
I'd expect a proper and exhaustive explanation of what's going on and clear evidence there is no better alternative in here before I approach this pull again.
I'd rather even revert to #25396 (comment) than do this.
There was a problem hiding this comment.
Totally cursed, did some research on why obsoleting properties this way are against OOP.
Will revert as implementing that logic is probably a framework-level task.
There are NRT changes I'm not sure I did correctly. Also not sure about making
OsuHoverContainer.IdleColourpublic, can make nested private classes instead if better.