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

Fix incorrect use of display slot indexes #3283

Closed

Conversation

thibaulthenry
Copy link
Contributor

Sponge | SpongeAPI

In this PR I'm attempting to fix the bug I found updating display slots in API-8.

The use of the display slot indexes was wrong. So for example when I tried to display an objective to the sidebar, it actually appeared in the sidebar.aqua.

I decided to put an id field on the SpongeDisplaySlot. I tested every cases when display slots were used, and it seems to fix the bug.

Also, I noticed that some the parameters ChatFormatting color, @Nullable final Function<ChatFormatting, DisplaySlot> withColorFunction are always null, even for colored display slots.

I can update this PR to fill the color parameter, but for withColorFunction I don't know what I need to put into.
Need your feedbacks for this.

Also, I made a little change in the API, like we talked about a while ago with Zidane and Masa.

Thanks for reading this PR

@dualspiral
Copy link
Contributor

Hi @thibaulthenry - I'm sorry this hasn't been looked at until now.

withColorFunction is really weird here. It's meant to be used as a function to get a DisplaySlot based on the supplied colour. For some reason, it's been made an instance method on DisplaySlot when really, it should not be.

That said, is there a use for getting an objective by colour? I'm not sure there is. If not, the withTeamColor method should be removed, and with it, the withColorFunction parameter. If so, that method should be static and (probably) call into a factory that does the selection for you.

@thibaulthenry
Copy link
Contributor Author

Hey, I'll not be able to look at it until this week-end but yeah, I'll try to fix things with your feedbacks

@dualspiral
Copy link
Contributor

Supersceded by #3405

Thanks for the initial contribution which I have taken forward (what you had was good but that "withTeamColor" thing has proved to be a bit of a headache).

@dualspiral dualspiral closed this May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs updating hey, origin changed, you need to update system: scoreboard version: 1.16 (u) API: 8
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants