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.

Sign up for GitHub

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

Jump to bottom

🚀 Better console color system #134

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

🚀 Better console color system #134

wants to merge 10 commits into from

Conversation

ItsTheSky
Copy link
Contributor

These small changes allow better customization & organization of console colors.

Example code:

on script load:
    print "<orange>This is in orange, <yellow>and this in yellow.<reset>"
    print "<cyan><b>This is bold cyan,<reset> and this is not bold neither colored!"
    print "<pink><u>This is underlined pink,<reset><color=purple_background> I'M BATMAN!"

Output:

image

Main Changes:

  • Each color has its own code only. The escaping symbol and delimiter are added after.
  • Each color can now have aliases, for example, orange_bright could also be used as yellow.
  • Color, style and background can now be superposed (wasn't possible before, it was clearing the color when applying a style)

Olyno
Olyno suggested changes Jun 28, 2022
View reviewed changes
Copy link
Contributor

@Olyno Olyno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi 👋🏻 Thanks you for your contributions. Here is a list of some changes i would like to see in a first review.

src/main/java/io/github/syst3ms/skriptparser/expressions/ExprAsk.java
@@ -0,0 +1,53 @@
package io.github.syst3ms.skriptparser.expressions;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this pull request

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was made basing myself on my other PR.
Not sure how should I do now.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can delete the files locally and push the changes here. It will remove the files from the PR as Github compares with the latest commit in the master branch.

src/main/java/io/github/syst3ms/skriptparser/tags/TagColor.java Outdated Show resolved Hide resolved
src/main/java/io/github/syst3ms/skriptparser/util/color/ConsoleColors.java
ITALIC("3", true, "o"),
UNDERLINE("4", true, "n", "u"),
STRICKTHROUGH("9", true, "m", "strike"),
;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something wrong here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks line 46

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't see what's wrong, is it the constant's name ?
It has been renamed by Mwexim

src/main/java/io/github/syst3ms/skriptparser/util/color/ConsoleColors.java Outdated Show resolved Hide resolved
src/main/java/io/github/syst3ms/skriptparser/util/color/ConsoleColors.java Outdated Show resolved Hide resolved
@ItsTheSky ItsTheSky requested a review from Olyno June 28, 2022 20:04
Mwexim
Mwexim requested changes Jul 8, 2022
View reviewed changes
Copy link
Owner

@Mwexim Mwexim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great additions!

src/main/java/io/github/syst3ms/skriptparser/tags/TagColor.java
@Override
public boolean init(String key, String[] parameters) {
final Optional<ConsoleColors> optional;
if (key.equalsIgnoreCase("color") && parameters.length != 0) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should check for length 1.

src/main/java/io/github/syst3ms/skriptparser/tags/TagReset.java
@@ -24,7 +25,7 @@ public boolean init(String key, String[] parameters) {
}

public String getValue(String affected) {
return affected;
return ConsoleColors.RESET + affected;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this implementation is correct for now, it reminds me that a small overhaul of the tag system is needed in order to give addons the flexibility of changing the reset tag.

src/main/java/io/github/syst3ms/skriptparser/util/color/ConsoleColors.java Outdated Show resolved Hide resolved
src/main/java/io/github/syst3ms/skriptparser/util/color/ConsoleColors.java Outdated Show resolved Hide resolved
src/main/java/io/github/syst3ms/skriptparser/tags/TagColor.java Outdated Show resolved Hide resolved
WeeskyBDW
WeeskyBDW reviewed Aug 3, 2022
View reviewed changes
src/main/java/io/github/syst3ms/skriptparser/Parser.java
@@ -153,7 +153,7 @@ public static void printLogs(List<LogEntry> logs, Calendar time, boolean tipsEna
for (LogEntry log : logs) {
ConsoleColors color = ConsoleColors.WHITE;
if (log.getType() == LogType.WARNING) {
color = ConsoleColors.YELLOW;
color = ConsoleColors.ORANGE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont think change to orange is a good idea since yellow is in general associate with warning so it would imo create bad idea of warning for skript users that would next switch to another language

@WeeskyBDW
Copy link
Contributor

👋 @ItsTheSky
This PR is open since a while and i see no more activity about it, is it ready for reviewing ? do you need help ? this comment isn't and shouldn't be take as blaming at all

@ItsTheSky
Copy link
Contributor Author

Hi! Sorry for taking so long to reply too 😭
I don't currently plan to work on this PR anymore as it seems this repo is archived.
However if it's still updated and have a future I'll be glad to go hop in again :D

@Mwexim
Copy link
Owner

Mwexim commented Mar 27, 2024

I think the tag system needs a major overhaul, which is one of the reasons this pull request stays open currently. The code itself is good and if there would be any changes I'd want to make, I'd probably make a quick commit myself.

If I ever get more time to work on this project, this should be handled rather quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WeeskyBDW WeeskyBDW WeeskyBDW left review comments

@Mwexim Mwexim Mwexim requested changes

@Olyno Olyno Awaiting requested review from Olyno

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ItsTheSky @WeeskyBDW @Mwexim @Olyno