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

TextFormat: Add colors and harmonize class #6483

Closed
wants to merge 2 commits into from

Conversation

dadodasyra
Copy link

Introduction

The new bedrock colors in chat are not usable on server side (console and command/chat parsing) due to TextFormat not being up to date.
The class was mixing self:: and TextFormat:: use reference the class itself. I opted to harmonize it to self::

Relevant issues

None

Changes

API changes

None

Behavioural changes

The colors will now be parsed and be shown correctly server side.

Backwards compatibility

None

The names and specific colors where extract from the "semi-official" wiki https://minecraft.wiki/w/Formatting_codes

@jasonw4331
Copy link
Contributor

Sadly I believe this has the same blocker as #5838

@jasonw4331 jasonw4331 added Category: API Related to the plugin API Category: Gameplay Related to Minecraft gameplay experience Type: Enhancement Contributes features or other improvements to PocketMine-MP Status: Blocked Depends on other changes which are yet to be completed labels Nov 5, 2024
@ShockedPlot7560
Copy link
Member

The unique blocked is the BC on removing underline and strikethrough

@dadodasyra
Copy link
Author

Sadly I believe this has the same blocker as #5838

Yup my bad I tought this was a more recent thing (the color update) so I didn't checked PR that's old.

The unique blocked is the BC on removing underline and strikethrough

So what's next ? Basically his PR is more detailed than mine (and does not forget the whole terminal stuff and regexes)..

@ShockedPlot7560
Copy link
Member

ShockedPlot7560 commented Nov 6, 2024

First, when you need to add an and word in your PR it mean that you need to create multiple PRs.

But for now, if we want to include the new colour, feel free to add only the colours and not to remove the underline and strikethrough, which are BC (not until PM6). But each time the PRs do everything at the same time, which blocks the merge.

@dktapps
Copy link
Member

dktapps commented Nov 6, 2024

First, when you need to add an add word in your PR it mean that you need to create multiple PRs.

You mean an "and" word?

@dktapps
Copy link
Member

dktapps commented Nov 6, 2024

But for now, if we want to include the new colour, feel free to add only the colours and not to remove the underline and strikethrough, which are BC (not until PM6).

This still creates problems for terminal usage, since people using the new colour constants will get unexpected text effects on the terminal. There's a reason no one added these already. It's BC-breaking no matter what change is made.

@dadodasyra
Copy link
Author

I mean, is it really an issue that the old underline and strikethrough change to a color ? No one is using it anymore since it's not usable in game.. The BC break seems "acceptable" to me

@dktapps
Copy link
Member

dktapps commented Nov 6, 2024

No BC breaks on minor or patch versions. Those are the rules.

@dadodasyra
Copy link
Author

Can we even qualify a color change on a console (so an administrator tool) a BC break ?

@AzaleeX
Copy link

AzaleeX commented Nov 8, 2024

Alternatively, why not block colors directly in the terminal by resetting the color with §r

@dktapps dktapps added the Multiple changes PR contains multiple changes and requires separation label Nov 14, 2024
@dktapps dktapps changed the title Add colors and harmonize class TextFormat: Add colors and harmonize class Nov 14, 2024
@dktapps
Copy link
Member

dktapps commented Nov 29, 2024

Closing this as #5838 is more complete and doesn't contain unrelated changes.

@dktapps dktapps closed this Nov 29, 2024
@dktapps dktapps added the Resolution: Declined PR has been declined by maintainers label Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: API Related to the plugin API Category: Gameplay Related to Minecraft gameplay experience Multiple changes PR contains multiple changes and requires separation Resolution: Declined PR has been declined by maintainers Status: Blocked Depends on other changes which are yet to be completed Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants