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

Expose truncated state of SpriteText #5636

Merged
merged 3 commits into from Jun 6, 2023

Conversation

Joehuu
Copy link
Member

@Joehuu Joehuu commented Jan 25, 2023

Use case is in ppy/osu@master...Joehuu:osu:truncating-text-tooltips.

The way around the overhead is to limit it to Truncate = true cases + overriding HandlePositionalInput to check when an ellipsis is added. Can't really override that on base SpriteText as it'll affect other inherited classes like DrawableDate.

peppy
peppy previously approved these changes Jan 25, 2023
@peppy peppy requested a review from a team January 25, 2023 08:29
@peppy
Copy link
Sponsor Member

peppy commented Jan 25, 2023

Looks fine, but going to wait on a second opinion.

@Joehuu for the osu! side implementation, i'd not call base.HandlePositionalInput at all (kinda redundant) and i'd also make TruncatingSpriteText a sealed class because abstracting further may lead to unexpected issues with input handling.

@smoogipoo
Copy link
Contributor

I worry that this is going to become a case of "change every text in the game to an OsuTruncatingSpriteText". Are you expecting this to be the case? Note that the optimisation of HandlePositionalInput is based on whether any of the methods have been overridden and not whether they've returned true/false.

@peppy
Copy link
Sponsor Member

peppy commented Jan 26, 2023

Note that the optimisation of HandlePositionalInput is based on whether any of the methods have been overridden and not whether they've returned true/false.

That's precisely why I proposed sealing the class and not calling the base method – it would maintain the correct optimisation in all usages if done that way.

@Joehuu
Copy link
Member Author

Joehuu commented Jan 28, 2023

I worry that this is going to become a case of "change every text in the game to an OsuTruncatingSpriteText". Are you expecting this to be the case?

This won't be applied to every text. The osu!-side PR is only changing a subset of them, mostly text that we have no control over (usernames, beatmap metadata, and skin names).

@peppy peppy force-pushed the sprite-text-ellipsis-added branch from 2e47670 to dd8c362 Compare June 6, 2023 05:11
@peppy
Copy link
Sponsor Member

peppy commented Jun 6, 2023

I don't see any harm in getting this in, regardless of whether we are using it or not. Could probably do with test coverage but honestly truncation doesn't have any yet and it's worked okay, so it can come later if ever.

@peppy peppy enabled auto-merge June 6, 2023 05:13
@peppy peppy changed the title Expose ellipsis added property in sprite text Expose truncated state of SpriteText Jun 6, 2023
@peppy peppy merged commit 5dca93c into ppy:master Jun 6, 2023
11 checks passed
@Joehuu Joehuu deleted the sprite-text-ellipsis-added branch June 6, 2023 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants