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

colors working incorrectly on tf #116

Open
3151 opened this issue Aug 21, 2020 · 10 comments
Open

colors working incorrectly on tf #116

3151 opened this issue Aug 21, 2020 · 10 comments
Labels
amnuts-parity features that amnuts also has enhancement
Milestone

Comments

@3151
Copy link

3151 commented Aug 21, 2020

While using the Talker with tf5 within tmux the colors changes for unknown reason.
Its lines over lines in for example green, than it changes to blue for a lot of lines than blue.
I could not see any trigger to that.

@marado marado added the bug label Aug 21, 2020
@marado marado added this to the 0.7.0 milestone Aug 21, 2020
@marado
Copy link
Owner

marado commented Aug 21, 2020

We got more than one tf5 user with the same symptom, in the same running instance, where a telnet user sees no problem, so... maybe this is something reproducible with the use of tf5. Also, "after doing an .who everything is green, so it is as if the color reset command never came in."

@marado
Copy link
Owner

marado commented Aug 21, 2020

Mike says: I can see the problem in ncat
Mike says: there is no reset
Mike says: there is a [39m
Mike says: personally I'd throw a 0m in there

@marado
Copy link
Owner

marado commented Aug 22, 2020

This seems like a tf5 bug. Here's a command repro'ing it:

var formatters = require('../utils/formatters.js');
  
exports.command = {
    name: "tfcolors",
    autoload: true,
    unloadable: false,
    min_rank: 0,
    display: "debug command.",
    help: "debug command",
    usage: ".tfcolors",

    execute: function(socket, command, command_access) {

        var chalk = require('chalk');
        command_access.sendData(socket, chalk.blue("+blue+") + "no color");
        command_access.sendData(socket, "\r\n");
    }
}

This works well on several clients but not on tf5: While what is sent to the user is:

.tfc
ÿü^A^[[34m+blue+^[[39mno color^M

tf seems to completely ignore the "grey" command, and shows it all as blue.

@marado
Copy link
Owner

marado commented Aug 22, 2020

tf indeed does not support 39m:

/set start_color_black                  ^[[30m
/set start_color_red                    ^[[31m
/set start_color_green                  ^[[32m
/set start_color_yellow                 ^[[33m
/set start_color_blue                   ^[[34m
/set start_color_magenta                ^[[35m
/set start_color_cyan                   ^[[36m
/set start_color_white                  ^[[37m

/set start_color_bgblack                ^[[40m
/set start_color_bgred                  ^[[41m
/set start_color_bggreen                ^[[42m
/set start_color_bgyellow               ^[[43m
/set start_color_bgblue                 ^[[44m
/set start_color_bgmagenta              ^[[45m
/set start_color_bgcyan                 ^[[46m
/set start_color_bgwhite                ^[[47m

@marado
Copy link
Owner

marado commented Aug 22, 2020

According to the ANSI standard, 39 is "Default foreground color". While it is tf's fault that this isn't supported we should probably figure out a way to acomodate this (other than "disable colors").

Sending resets more often (eg,, after the output of each command) might be an interesting precaution, and mitigate (even if not totally fix) this issue.

I need more time to think, but... I might want to do two things here:

  • open an issue on tf5
  • implement an "after command reset"

@marado
Copy link
Owner

marado commented Aug 23, 2020

@marado
Copy link
Owner

marado commented Aug 23, 2020

An after command reset will cause some problems, and would not fix all the potentially affected people (since we would only be able to send the chalk.reset() to the command caller). While I'm not very happy with the idea of implementing something like this, we might have to, and the solution will pass by reviewing everything that is socket-written, and spread reset()'s on the appropriate places.

We could - temporarily - hack all the writes and replace 39's by 0's (fg defaults with resets), but that would need to be fixed as soon as we tried to use background colors.

@marado marado added amnuts-parity features that amnuts also has enhancement and removed bug labels Aug 23, 2020
@marado
Copy link
Owner

marado commented Aug 23, 2020

Well... better yet, we can replace 39's by a default color (eg. white), that can actually be configurable in the talker. That would actually turn a "clients limitation" into a feature. Also, we should also deal with 49's (Default background color), which suffer from exactly the same problem.

@marado marado changed the title Changing colors colors working incorrectly on tf Aug 24, 2020
@marado
Copy link
Owner

marado commented Aug 24, 2020

Unlike with the foreground color, there is a big difference between painting with a (talker defined) foreground color (instead of using the client-defined), and painting with a talker defined background color instead of using the clients: the background color might only affect the areas where text exists, and if you have a client with transparencies and so on, there's a big difference between having a defined background color and having none.

ie, we should probably find a way to "partially reset" colors... If we get a 39 (default foreground color), se can instead send a reset (0) + the previously defined background color (which we would need to know). And vice-versa. Doing this is a PITA unless it is done by chalk itself. Chalk has no way to do that now, and isn't properly "extensible" in order to do that. It might be feasible to alter chalk in order to support a "partial ANSI" mode that does not know 39 and 49 and deals with 0's instead... to investigate.

@marado
Copy link
Owner

marado commented Oct 10, 2021

Now that #87 is done, not only we have a reset in place, but it might even be triggering on the right places already.
It would be useful to review this - how does TalkerNode with color on work with tf at the moment, and if there are still places where the color is bleeding, we can now easily deal with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
amnuts-parity features that amnuts also has enhancement
Projects
None yet
Development

No branches or pull requests

2 participants