eraseLine() malfunction!

Osiris-Team commented 3 years ago

Note: This took me several hours to figure out, so please bear 1-2 minutes to read this, thanks! Context: Trying to update/replace multiple lines with new data. Bug: When then console is already filled with stuff, eraseLine()/restoreCursorPostion() wont work! Confirmed broken on versions: Jansi 2.2.0 (Windows and Linux) and Jansi 1.18 (Linux) Confirmed working on versions: Jansi 1.18 (Windows) Java version: 1.8 Code:

try{
                // Print out TEST 15 times. This shouldn't fill out the console window and the erase function works just fine.
                // BUT if we change this to 30 or 50, so that the console window is filled out, and then use the erase function it breaks and does not erase any lines! Instead of replacing it writes new lines.
                for (int i = 0; i < 30; i++) {
                    System.out.println("TEST");
                }
                AnsiConsole.systemInstall(); 
                // Step 1: save the cursor.
                System.out.print(Ansi.ansi().saveCursorPosition());
                for (int i = 0; i < 5; i++) {
                    // Step 2: print the data
                    System.out.println(Ansi.ansi().a("Line 1 with data "+i));
                    System.out.println(Ansi.ansi().a("Line 2 with data "+i));
                    // Step 3: define interval for next update
                    Thread.sleep(1000);
                    // Step 4: jump back to start, erase the data and jump back to start again
                    System.out.print(Ansi.ansi().restoreCursorPosition());
                    System.out.print(Ansi.ansi().eraseLine(Ansi.Erase.FORWARD));
                    System.out.print(Ansi.ansi().restoreCursorPosition());
                }
            } catch (Exception e) {
                e.printStackTrace();
            }

gnodet commented 3 years ago

I've tried to make them work on my OSX terminal, but I can't find a way to reliably use the restoreCursorPosition / saveCursorPosition commands. Even using the sequences provided by the terminal using infocmp seems to fail. So I'm not sure the problem is in jansi...

gnodet commented 3 years ago

If you want to do advanced stuff, I suggest you use JLine's terminal and in particular the Display class which provides a really and easy way to update the few last lines of the console (which seems to be your case).

Osiris-Team commented 3 years ago

@gnodet Oh that's sad :( But thanks for the help anyway. And I'll take a look at JLine.

gnodet commented 3 years ago

I've tried to make them work on my OSX terminal, but I can't find a way to reliably use the restoreCursorPosition / saveCursorPosition commands. Even using the sequences provided by the terminal using infocmp seems to fail. So I'm not sure the problem is in jansi...

So using \E7 and \E8 instead of \E[s and \E[u seems to work better on some terminals, at least in the simple case where the number of printed lines is small, but still fails when scrolling has been toggled. I really suggest JLine's Display.

Osiris-Team commented 3 years ago

@gnodet Are there any examples of how to do this with Jline? I couldn't figure it out...

gnodet commented 3 years ago

Try something like

Terminal terminal = TerminalBuilder.terminal();
for (int i = 0; i < 30; i++) {
    terminal.writer().println("TEST");
}
Display display = new Display(terminal, false);
for (int i = 0; i < 5; i++) {
    display.update(Arrays.asList(
            AttributedString.fromAnsi("Line 1 with data " + i),
            AttributedString.fromAnsi("Line 2 with data " + i)
    ), -1);
    Thread.sleep(1000);
}

Osiris-Team commented 3 years ago

@gnodet My output running your code:

Line 1 with data 0java.lang.ArithmeticException: / by zero
        at org.jline.utils.Display.update(Display.java:304)
        at org.jline.utils.Display.update(Display.java:103)

gnodet commented 3 years ago

Yeah, I haven't tested it. You need to initialize the size on the display with

Size size = terminal.getSize();
display.resize(size.getRows(), size.getColumns());

Osiris-Team commented 3 years ago

@gnodet Thanks for the help ;)

Osiris-Team commented 3 years ago

@gnodet Hey it's me again! Sry for bothering you again, but I'm not able to make it work for my application :/ The example you provided just works fine as expected though.

The code below is responsible for printing stuff to the console. What I am trying to achieve with it is actually pretty simple: Each line represents one currently running thread. That line contains information related to its progress. But somehow the output is completely buggy.

Easy way to reproduce my error:

Clone https://github.com/Osiris-Team/Better-Thread

Run the "Startup-Windows.cmd" script

/**
* Uses jansi & jline to display the live status of
* all active BetterThreads.
* Runs until there are no more active BetterThreads.
*/
public class BetterThreadDisplayer extends Thread {
private Terminal terminal;
private Display display;
private BetterThreadManager manager;
private String label = "[MyAppName]";
private String threadType = "[PROCESS]";
private LocalDateTime now;
private boolean showWarnings;
private boolean showDetailedWarnings;
private int refreshInterval;
private DateTimeFormatter dateFormatter = DateTimeFormatter.ofPattern("HH:mm:ss");
private List<BetterWarning> allWarnings = new ArrayList<>();

private byte anim;

/**
 * Creates a new ThreadDisplayer with default
 * values set.
 * To customize these values use the other constructor.
 * @param manager
 */
public BetterThreadDisplayer(BetterThreadManager manager) {
    this(manager, null);
}

public BetterThreadDisplayer(BetterThreadManager manager, String label) {
    this(manager, label, null);
}

public BetterThreadDisplayer(BetterThreadManager manager, String label, String threadType) {
    this(manager, label, threadType, null);
}

public BetterThreadDisplayer(BetterThreadManager manager, String label, String threadType,
                             DateTimeFormatter formatter) {
    this(manager, label, threadType, formatter, false);
}

public BetterThreadDisplayer(BetterThreadManager manager, String label, String threadType,
                             DateTimeFormatter formatter, boolean showWarnings) {
    this(manager, label, threadType, formatter, showWarnings, false);
}

public BetterThreadDisplayer(BetterThreadManager manager, String label, String threadType,
                             DateTimeFormatter formatter, boolean showWarnings, boolean showDetailedWarnings) {
    this(manager, label, threadType, formatter, showWarnings, showDetailedWarnings, 250);
}

/**
 * Creates a new ThreadDisplayer.
 * You can customize very aspect of it by
 * passing over the wanted values.
 * @param manager
 * @param label
 * @param threadType
 * @param formatter
 * @param showWarnings
 * @param showDetailedWarnings
 * @param refreshInterval
 * @throws RuntimeException if there was an error getting the systems terminal.
 */
public BetterThreadDisplayer(BetterThreadManager manager, String label, String threadType, DateTimeFormatter formatter,
                             boolean showWarnings, boolean showDetailedWarnings, int refreshInterval) {
    this.manager = manager;
    if (label!=null) this.label = label;
    if (threadType!=null) this.threadType = threadType;
    if (formatter!=null) this.dateFormatter = formatter;
    this.showWarnings = showWarnings;
    this.showDetailedWarnings = showDetailedWarnings;
    this.refreshInterval = refreshInterval;

    // Check if Jansi console was already started
    //AnsiConsole.systemInstall();
    try{
        terminal = TerminalBuilder.terminal();
    } catch (Exception e) {
        e.printStackTrace();
        throw new RuntimeException("There was an error getting the systems terminal!");
    }
}

@Override
public void run() {
    super.run();
    try{
        resize();
        terminal.writer().println(" ");
        while (printAll()){
            sleep(refreshInterval);
        }
    } catch (Exception e) {
        e.printStackTrace();
    }
}

private void resize(){
    display = new Display(terminal, false);
    Size size = terminal.getSize(); // Need to initialize the size on the display with
    display.resize(size.getRows(), size.getColumns());
}

/**
 * Prints the current status of all processes.
 * This method should be used with a pause of 250ms.
 * Returns false when all processes have finished!
 * @return false when all processes have finished!
 */
public boolean printAll(){

    // Get current date
    now = LocalDateTime.now();

    // Fill this list with threads details and update the console after this
    List<AttributedString> list = new ArrayList<>();
    manager.getAll().forEach(thread -> {
        // Display the information for each thread.
        // Each thread gets one line.
        StringBuilder builder = new StringBuilder();

        //Format the output for a single process
        //First add the date, label and process info labels

        builder.append(ansi()
                .bg(WHITE)
                .fg(BLACK).a("["+dateFormatter.format(now)+"]")
                .fg(CYAN).a(label)
                .fg(BLACK).a(threadType)
                .reset());

        //Add the loading animation

        if (thread.isFinished()){
            if(thread.isSkipped()){
                builder.append(ansi()
                        .fg(BLUE).a(" [#] ")
                        .reset());
            }
            else if (thread.isSuccess()){
                builder.append(ansi()
                        .fg(GREEN).a(" [#] ")
                        .reset());
            }
            else{
                builder.append(ansi()
                        .fg(RED).a(" [#] ")
                        .reset());
            }
        }
        else{
            switch (anim) {
                case 1:
                    builder.append(ansi().a(" [*] ")); // \\
                    break;
                case 2:
                    builder.append(ansi().a(" [|] "));
                    break;
                case 3:
                    builder.append(ansi().a(" [+] "));
                    break;
                default:
                    anim = 0;
                    builder.append(ansi().a(" [-] "));
            }
        }

        // Add the actual process details and finish the line
        final String name = thread.getName();
        final long now = thread.getNow();
        final long max = thread.getMax();
        final byte percent  = thread.getPercent();
        final String status = thread.getStatus();

        if (now > 0){
            if (thread.isSkipped())
                builder.append(ansi()
                        .a("> ["+name+"] "+status));
            else
                builder.append(ansi()
                        .a("> ["+name+"]["+percent+"%] "+status));
        }
        else{
            builder.append(ansi()
                    .a("> ["+name+"] "+status));
        }
        builder.append(ansi().reset());

        // Add this message to the list
        list.add(new AttributedString(builder.toString()));
    });

    // This must be done outside the for loop otherwise the animation wont work
    anim++;

    if (manager.getAll().size()==0){
        list.add(new AttributedString("No threads! Waiting..."));
    }
    else{
        // This means we finished and should stop looping
        // We print the last warnings message and stop.
        if (manager.isFinished()){
            display.update(list, -1); // Update one last time
            this.allWarnings = manager.getAllWarnings();
            formatWarnings();
            return false;
        }
    }

    display.update(list, -1);
    return true;
}

private long getPercentage(long now, long max){
    return (now*100/max);
}

/**
 * This is will be shown when all processes finished.
 */
private void formatWarnings(){

    Ansi ansiDate = ansi()
            .bg(WHITE)
            .fg(BLACK).a("["+dateFormatter.format(now)+"]")
            .fg(CYAN).a(label)
            .fg(BLACK).a("[SUMMARY]")
            .reset();

    if (allWarnings.isEmpty()){
        System.out.println(ansi()
                .a(ansiDate)
                .fg(GREEN)
                .a(" Executed all tasks successfully!")
                .reset());
    }
    else if (showWarnings) {
        System.out.println(ansi()
                .a(ansiDate)
                .fg(YELLOW)
                .a(" There are " + allWarnings.size() + " warnings:")
                .reset());

        if (showDetailedWarnings) {
            BetterWarning betterWarning;
            for (int i = 0; i < allWarnings.size(); i++) {
                betterWarning = allWarnings.get(i);
                StringBuilder builder = new StringBuilder();
                builder.append(ansiDate);
                builder.append(ansi()
                        .fg(YELLOW).a("[WARNING-" + i + "][" + betterWarning.getThread().getName() + "][Message: " + betterWarning.getException().getMessage() +
                                "][Cause: " + betterWarning.getException().getCause() +
                                "][Extra: " + betterWarning.getExtraInfo() +
                                "][Trace: " + Arrays.toString(betterWarning.getException().getStackTrace())).reset());
                System.out.println(builder.toString());
            }
        }
        else {
            BetterWarning betterWarning;
            for (int i = 0; i < allWarnings.size(); i++) {
                betterWarning = allWarnings.get(i);
                StringBuilder builder = new StringBuilder();
                builder.append(ansiDate);
                builder.append(ansi()
                        .fg(YELLOW).a("[WARNING-" + i + "][" + betterWarning.getThread().getName() + "][Message: " + betterWarning.getException().getMessage() + "]").reset());
                System.out.println(builder.toString());
            }
        }
    }
    else{
        System.out.println(ansi()
                .fg(YELLOW).a(" There are "+ allWarnings.size()+" warnings! Enable 'show-warnings' to view them, or check your log for further details!")
                .reset());
    }
}

public BetterThreadManager getManager() {
    return manager;
}

public void setManager(BetterThreadManager manager) {
    this.manager = manager;
}

public String getLabel() {
    return label;
}

public void setLabel(String label) {
    this.label = label;
}

public String getThreadType() {
    return threadType;
}

public void setThreadType(String threadType) {
    this.threadType = threadType;
}

public boolean isShowWarnings() {
    return showWarnings;
}

public void setShowWarnings(boolean showWarnings) {
    this.showWarnings = showWarnings;
}

public boolean isShowDetailedWarnings() {
    return showDetailedWarnings;
}

public void setShowDetailedWarnings(boolean showDetailedWarnings) {
    this.showDetailedWarnings = showDetailedWarnings;
}

public int getRefreshInterval() {
    return refreshInterval;
}

public void setRefreshInterval(int refreshInterval) {
    this.refreshInterval = refreshInterval;
}

public DateTimeFormatter getDateFormatter() {
    return dateFormatter;
}

public void setDateFormatter(DateTimeFormatter dateFormatter) {
    this.dateFormatter = dateFormatter;
}

public List<BetterWarning> getAllWarnings() {
    return allWarnings;
}

public void setAllWarnings(List<BetterWarning> allWarnings) {
    this.allWarnings = allWarnings;
}
}

gnodet commented 3 years ago

@Osiris-Team the problem is that you're not building the strings correctly. In order to build an AttributedString, you need to use AttributedString.fromAnsi(ansiString) instead of new AttributedString(ansiString). Ideally, you'd use an AttributedStringBuilder instead of using a StringBuilder and writing ansi codes directly, but that's not a big problem. So try with the following patch:

diff --git a/src/main/java/com/osiris/betterthread/BetterThreadDisplayer.java b/src/main/java/com/osiris/betterthread/BetterThreadDisplayer.java
index 2ee493f..4144b6e 100644
--- a/src/main/java/com/osiris/betterthread/BetterThreadDisplayer.java
+++ b/src/main/java/com/osiris/betterthread/BetterThreadDisplayer.java
@@ -220,7 +220,7 @@ public class BetterThreadDisplayer extends Thread {
             builder.append(ansi().reset());

             // Add this message to the list
-            list.add(new AttributedString(builder.toString()));
+            list.add(AttributedString.fromAnsi(builder.toString()));
         });

         // This must be done outside the for loop otherwise the animation wont work
(END)

Osiris-Team commented 3 years ago

@gnodet Wow that really did it! Never thought that that would be the issue. Thank you so much!

Osiris-Team commented 3 years ago

@gnodet Ran into another issue :/ For the first run, everything works fine. But the second run is malformatted like below.

[12:06:19][MyAppName][PROCESS] [#] > [BetterThread][100%] Initialising...
[12:06:19][MyAppName][PROCESS] [#] > [BetterThread][100%] Initialising...
[12:06:19][MyAppName][PROCESS] [#] > [BetterThread][100%] Initialising...
[12:06:19][MyAppName][SUMMARY] Executed all tasks successfully!
SIOJASDIOASD
SIOJASDIOASD
20][MyAp5][MyAppName][PROCESS] [#] > [BetterThread][100%] Initialising...
SIOJASD25][MyAppName][PROCESS] [#] > [BetterThread][100%] Initialising...
       25][MyAppName][PROCESS] [#] > [BetterThread][100%] Initialising...
[12:06:25][MyAppName][SUMMARY] Executed all tasks successfully!

Also "SIOJASDIOASD" has been printed 4 times, but its last 2 lines get somehow overwritten.

Osiris-Team commented 3 years ago

@gnodet Oh, and there is another thing. It also breaks if another Thread writes stuff to the console, while it's displaying the progresses.

gnodet commented 3 years ago

The Display class has to be the only one writing to the console in order to have a correct display. If you have threads writing to the console directly using System.out, you need to write a custom System.out stream that can grab the output and hand it to the Terminal in an orderly way.

Osiris-Team commented 3 years ago

@gnodet Ok did as you said: https://github.com/Osiris-Team/Better-Thread/commit/9e110ce3830e021ba3b8d5f7a3f65e7ae8ebecc3 Added two new classes: MyDisplay and MyLine MyDisplay extends Jlines Display class but provides functionality for adding lines and updating lines more precisely. MyLine represents one line in the terminal/console.

Now i can do this:

// This will update/replace the line at position 0.
        // Adds the line to the console, if position 0 doesn't exist yet.
        MY_DISPLAY.updateLines(new MyLine("hello1", 0));

        // This adds the line to the console.
        MY_DISPLAY.addLines(new MyLine("hello1"));
       // This does the same:
       MY_DISPLAY.addLines("hello1"));

The only thing that I'm not completly happy with is the way Im doing this. Because the way Im achieving this is by calling update() in MyDisplay.updateLines(MyLine... lines), which causes all old lines to get updated to I guess:

this.update(convertToAttributedStringList(allLines), -1);

Maybe you got a better way of doing this?

*Edit: And also that means that I would have to keep every single line in memory :/

Osiris-Team commented 3 years ago

The Display class has to be the only one writing to the console in order to have a correct display. If you have threads writing to the console directly using System.out, you need to write a custom System.out stream that can grab the output and hand it to the Terminal in an orderly way.

I thought instead of using System.out, I would now use:

MY_DISPLAY.addLines(new MyLine("hello1"));

gnodet commented 3 years ago

Yes, this is not the correct way. You need to use

// erase the lines
display.update(Collections.emptyList(), 0); 
// ... print lines to the terminal
terminal.writer().println(str); 
// usual print
display.update(lines, -1);

Osiris-Team commented 3 years ago

So basically the display knows of the lines printed by the terminal.writer()? But not of the lines printed by System.out

gnodet commented 3 years ago

So basically the display knows of the lines printed by the terminal.writer()? But not of the lines printed by System.out

Not really. The first line erases what the lines previously written to the terminal by Display. You can then use terminal.writer() (or System.out if you really want to, but those could be different if the terminal is not the system terminal). After that, we ask the Display to draw again the lines. This needs to be synced with the normal display loop somehow.

Osiris-Team commented 3 years ago

@gnodet I quite dont understand what -1 in display.update(lines, -1) means? *Edit: Its says targetCursorPostion but which position exactly? Horizontal or vertical?

Osiris-Team commented 3 years ago

Oh and this happens when running this code (I have absolutely no clue why): *Note that when we loop only 30 times it works fine.

        for (int i = 0; i < 100; i++) {
            MY_DISPLAY.add("TEST");
        }
        Thread.sleep(1000);
        MY_DISPLAY.updateLine(0, "0");
        Thread.sleep(1000);
        MY_DISPLAY.updateLine(10, "10");
        Thread.sleep(1000);
        MY_DISPLAY.updateLine(25, "25");

TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
0
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST
TEST

10

25

D:\Coding\JAVA\Better-Thread>pause

gnodet commented 3 years ago

@gnodet I quite dont understand what -1 in display.update(lines, -1) means? *Edit: Its says targetCursorPostion but which position exactly? Horizontal or vertical?

The position is expressed as a number of characters, so if your terminal is 80 columns, 79 will be the last column on the first line and 80 the first column on the second line. -1 means the last written character.

gnodet commented 3 years ago

As for your last question, the Display class is not supposed to be given a number of lines bigger than the terminal's height. You need to trim down the number of lines before handing them to the Display class.

Osiris-Team commented 3 years ago

As for your last question, the Display class is not supposed to be given a number of lines bigger than the terminal's height. You need to trim down the number of lines before handing them to the Display class.

Ok got it chef! Thanks!

Osiris-Team commented 3 years ago

@gnodet I quite dont understand what -1 in display.update(lines, -1) means? *Edit: Its says targetCursorPostion but which position exactly? Horizontal or vertical?

The position is expressed as a number of characters, so if your terminal is 80 columns, 79 will be the last column on the first line and 80 the first column on the second line. -1 means the last written character.

Ok but if -1 means the last written char, then why does this happen:

display.update(lines, -1); 
display.update(lines, -1);  // This method starts writting at the same old -1 position instead of the new last written char
// and thus replaces/updates these lines

*Edit: This means that the "real" -1 position doesn't get updated through the update(lines, -1) method? **Edit: In that case how would I update that position?

gnodet commented 3 years ago

I think you misunderstood the purpose of the Display class. That class is used to display one or more lines (there's also the fullscreen mode), and update those managed lines with whatever input you give it. The update(lines, pos) will update the display so that lines written by a previous call to update will be erased and the new ones will appear. The lines to display are given by the first parameter and the cursor position after the update is given by the second parameter. The original use case is the raw update of the LineReader : the prompt + editing line (or multiple lines) are completely handled by Display and the cursor position (you can move using arrow keys for example) is handled by the second parameter.

Osiris-Team commented 3 years ago

Yeah that makes much more sense. Thanks!

Osiris-Team commented 3 years ago

So basically the display knows of the lines printed by the terminal.writer()? But not of the lines printed by System.out

Not really. The first line erases what the lines previously written to the terminal by Display. You can then use terminal.writer() (or System.out if you really want to, but those could be different if the terminal is not the system terminal). After that, we ask the Display to draw again the lines. This needs to be synced with the normal display loop somehow.

Found a workaround for that btw:

        PrintStream old = System.out;
        MyPrintStream myPrintStream = new MyPrintStream(old);
        System.setOut(myPrintStream);

The idea is to "cache" all the stuff that was written to the custom PrintStream, while doing the "updating-loop", instead of printing/writing it. When the "updating-loop" finishes we reset the PrintStream and display the stuff that was written to it.

fusesource / jansi

eraseLine() malfunction! #195