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

Text.to_display_text returns Text #5971

Closed
JaroslavTulach opened this issue Mar 16, 2023 · 4 comments · Fixed by #6174
Closed

Text.to_display_text returns Text #5971

JaroslavTulach opened this issue Mar 16, 2023 · 4 comments · Fixed by #6174
Assignees
Labels
--bug Type: bug -libs Libraries: New libraries to be implemented

Comments

@JaroslavTulach
Copy link
Member

Especially when handling exceptions it is important to have proper implementation of to_display_text (which is invoked from PanicException.getMessage(). However Text! has some wrong implementation. Try:

main = IO.println "Ahoj".to_display_text

and it prints Text. That's wrong. I should print Ahoj.

@JaroslavTulach JaroslavTulach added -libs Libraries: New libraries to be implemented triage labels Mar 16, 2023
@JaroslavTulach JaroslavTulach added this to the Design Partners milestone Mar 16, 2023
@JaroslavTulach
Copy link
Member Author

Proposed fix:

diff --git engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/text/AnyToDisplayTextNode.java engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/text/AnyToDisplayTextNode.java
index e0bed6343..589f9f49a 100644
--- engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/text/AnyToDisplayTextNode.java
+++ engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/text/AnyToDisplayTextNode.java
@@ -31,6 +31,11 @@ public abstract class AnyToDisplayTextNode extends Node {
       throw new IllegalStateException(e);
     }
   }
+  
+  @Specialization
+  Text identity(Text self) {
+    return self;
+  }

@4e6 4e6 added --bug Type: bug and removed triage labels Mar 16, 2023
@radeusgd
Copy link
Member

I think there was some story here.

I remember to_display_text was being added because to_text would sometimes return too big payloads and overwhelm the IDE. After all the default Text visualization is planned to be Lazy to handle big texts.

If we do just identity here, stuff like error messages which want to display this text can get huge (for example if my Text is a contents of a whole book).

So I suggest that maybe we should truncate the returned Text at some character limit? (though if we do so we need to be careful to not cut the text between two codepoints belonging to a single grapheme cluster - so some slight care will need to be taken to correctly find a Break between proper Characters (BreakIterator.getCharacterInstance should be our friend)

@jdunkerley
Copy link
Member

Agree with Radek's comment on truncating the text - maybe first 80 grapheme clusters.
Otherwise @JaroslavTulach's fix looks like the right one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--bug Type: bug -libs Libraries: New libraries to be implemented
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants