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 the gravity output to use the ${OVER} sequence #2725

Merged
merged 3 commits into from
Oct 8, 2023

Conversation

rdwebdesign
Copy link
Member

What does this PR aim to accomplish?:

The "OVER" sequence CR + ESC[K is not working as expected.

The current code is just replacing the "OVER" sequence with a newline and printing the text after the previous line.

Also, the FTL gravity_parseList() function is sending only a Carriage Return (\r) instead of the full OVER sequence, because the web interface is not a terminal. This prints one very long line with the text of all Progress lines contatenated.

  [i] Processed 1% of downloaded list  [i] Processed 2% of downloaded list  [i] Processed 3% of downloaded list ...

progress_wrong

How does this PR accomplish the above?:

The new code fixes this:
When "OVER" is found, the last line on the screen is REPLACED by the new text (like it was in v5).

progress_fixed

Link documentation PRs if any are needed to support this PR:

none


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

  • I have read the above and my PR is ready for review. Check this box to confirm

@rdwebdesign rdwebdesign force-pushed the fix/gravity_progress branch 2 times, most recently from 1926532 to c52cb8c Compare October 7, 2023 06:33
@rdwebdesign rdwebdesign marked this pull request as ready for review October 7, 2023 17:37
@rdwebdesign rdwebdesign requested review from DL6ER and a team October 7, 2023 17:37
@rdwebdesign rdwebdesign force-pushed the fix/gravity_progress branch from c52cb8c to 4156d96 Compare October 7, 2023 17:55
Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

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

This code has a severe limitation: If \r is found, it actually needs to be the first character or the rest of the code (removing the immediately preceding line) does not make much sense and causes incorrect behavior.

In other words, I could also say that this code really depends on not more than one line being sent at the same time.

This is true in our current API design (each line is immediately pushed as a chunk), but it may be useful to be somewhat more explicit in the code (see additional review comments).

ta.append(string);

// If a Carriage Return is found ...
if (string.indexOf("\r") === -1) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this functionally equivalent to the easier to read/understand

Suggested change
if (string.indexOf("\r") === -1) {
if (string[0] !== '\r') {

?

See also my main review comment why I think this is the case.

Copy link
Member Author

@rdwebdesign rdwebdesign Oct 7, 2023

Choose a reason for hiding this comment

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

They are not the same:

string.indexOf("\r") === -1 will be false if \r is found in the string (in any position).

Your suggestion will be false only if the first character is \r.

var string = "test\r";
if (string.indexOf("\r") === -1) {
  console.log("CR not found");
} else {
  console.log("CR found at "+string.indexOf("\r"));
}

Output: CR found at 4.

Notes:

  • In v5 we always sent one line at a time and we still do it.
  • we always sent \r as the first character.

Copy link
Member

Choose a reason for hiding this comment

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

Sure that, but your example even underlines my point :-)

Assume we already have

abc
def
ghi

and not receive your chunk test\r.

The trailing \r would trigger removing ghi even though this is completely wrong.

With my suggested change, however, the code would not remove ghi and the output would become:

abc
def
ghi
test

IMO this is the correct behavior.

ta.text(ta.text().substring(0, ta.text().lastIndexOf("\n")) + "\n");
// ... and append the new text to the end of the output,
// without ${OVER} ("CR + ESC[K") or Carriage Return.
ta.append(string.replaceAll("\r", "").replaceAll("\r", ""));
Copy link
Member

Choose a reason for hiding this comment

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

This .replaceAll() kinda feels wrong to me but being aware of the one-line-after-another chunking, it is still doing what it should - even when it doesn't feel right (subjective feeling all the way).

Copy link
Member Author

Choose a reason for hiding this comment

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

We can create a function to proper cleanup the string, removing "\r�[K", "\r".
We can even allow the use of color codes and replace them with HTML <span> tags with CSS classes to use color output (like we had in v5).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, color codes may be even nicer but it may be over the top. If we want to go entirely crazy we could even make FTL send <span ...> ... </span> already by itself when requesting via the API.

Copy link
Member

Choose a reason for hiding this comment

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

Let's not get crazy here... formatting should be done by the receiving/displaying component, not the sending one, I think.

@rdwebdesign
Copy link
Member Author

I could also say that this code really depends on not more than one line being sent at the same time.

Actually that was my assumption, because this was the behavior for v5 and this is the behavior I saw with the current code. We can change that if needed, but: "Do we need to change this?"

To tell the truth, I just adapted the code we currently have in v5.
The main difference is PHP is handling both escape sequences and passing "<-------" to javascript.

In v6 I moved the 2 replace calls to javascript.

@rdwebdesign rdwebdesign marked this pull request as draft October 8, 2023 18:58
@rdwebdesign rdwebdesign force-pushed the fix/gravity_progress branch 2 times, most recently from 22792b8 to 8ef2a64 Compare October 8, 2023 20:59
@rdwebdesign rdwebdesign marked this pull request as ready for review October 8, 2023 20:59
@rdwebdesign rdwebdesign force-pushed the fix/gravity_progress branch from 8ef2a64 to d62077c Compare October 8, 2023 21:02
@rdwebdesign rdwebdesign requested review from yubiuser and DL6ER October 8, 2023 21:05
DL6ER
DL6ER previously approved these changes Oct 8, 2023
@rdwebdesign rdwebdesign merged commit 48e6682 into development-v6 Oct 8, 2023
@rdwebdesign rdwebdesign deleted the fix/gravity_progress branch October 8, 2023 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants