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

#414: slow StyledTextRenderer for unicode and unprintable control cha… #1439

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tmssngr
Copy link
Contributor

@tmssngr tmssngr commented Sep 3, 2024

…racters

This test replaces the characters 0x00-0x1F with some alternative character (by default '?') for rendering. This improves the performance significantly.

The previous pull request (#1425) did not work.

Note, that when changing replaceBelowSpaceCharacters to return the commented unicode characters, the snippet will fail with a "No more handles" exception. I reckon, we best should stick to a US-ASCII character from the range 0x20<=..<0x7F.

@tmssngr tmssngr requested a review from niraj-modi as a code owner September 3, 2024 09:48
Copy link
Contributor

github-actions bot commented Sep 3, 2024

Test Results

   486 files  ±0     486 suites  ±0   7m 43s ⏱️ +4s
 4 151 tests ±0   4 143 ✅ ±0   8 💤 ±0  0 ❌ ±0 
16 358 runs  ±0  16 266 ✅ ±0  92 💤 ±0  0 ❌ ±0 

Results for commit c37643d. ± Comparison against base commit d882cfb.

♻️ This comment has been updated with latest results.

return chr >= ' '
? chr
//: (char)(0x2400 + chr);
: '?';
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that \t\r\n all fall into this category, can this really be correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. The bug was not directly visible, but caused weird effects when editing.

@tmssngr tmssngr force-pushed the feature/414-styledtext-slow-rendering2 branch from af468e0 to 176f48a Compare September 3, 2024 17:52
@tmssngr
Copy link
Contributor Author

tmssngr commented Sep 3, 2024

My commit used the correct, approved email address, but I've created the pull request with my GitHub account. Don't know why eclipsefdn/eca complains.

@iloveeclipse
Copy link
Member

My commit used the correct, approved email address, but I've created the pull request with my GitHub account. Don't know why eclipsefdn/eca complains.

This commit should only have committers / authors with ECA signed, but you see there two different identities : syntevo@176f48a

ECA wants clarity regarding the code origin, so that every contribution is legal.

…able control characters

This test replaces the characters 0x00-0x1F with some alternative
character (by default '?') for rendering. This improves the performance
significantly.
@tmssngr tmssngr force-pushed the feature/414-styledtext-slow-rendering2 branch from 176f48a to c37643d Compare September 3, 2024 18:27
@mihnita
Copy link
Contributor

mihnita commented Sep 6, 2024

I wonder if it has something to do with the "Interpret ASCII control characters" setting
(under Settings -- Run/Debug -- Console)

I know that setting is only about the Console, but the console is implemented on top of StyledText.
So maybe the root cause is that processing.
I don't know if that "interpreting" is done in the console, or somewhere lower level, in StyledText somewhere

@@ -2854,6 +2854,14 @@ public boolean isDisposed () {
return device == null;
}

private static char replaceBelowSpaceCharacters(char chr) {
if (chr >= 0x20 || chr == '\t' || chr == '\r' || chr == '\n') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider allowing more?
Other characters that are used in CLI applications:

  • \u0007 : BELL, makes a "beep" sound
  • \u0008 : BS, backspace, moves back one character
  • \u001B : ESC, used for ANSI escape sequences (changing colors, cursor movement, clear screen, etc). This one is used quite a lot.

Since the Eclipse Console is built on top of the styled text widget not allowing ESC might break quite a lot of things.
(see https://eclipse.dev/eclipse/news/4.25/platform.php#debug-ansi-support)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants