Use offsets from `.bgv` file to select appropriate text #10510

JaroslavTulach · 2025-01-18T06:47:21Z

Enso is trying to generate IGV graph for our internal IR:

Dump compiler IR to IGV enso-org/enso#12061

We had issues to provide proper nodeSourceLocation information. I had to debug visualizer code to find out what's wrong. While doing that I made few fixes.

we were providing -1 as line location - IGV was then doing nothing - with 75d41ee it at least opens the file
I was getting NullPointerExceptions so I am adding checks and assert to catch that earlier
Last, but not least: we do provide start and end offsets for Enso nodes. Let's use them!

With these changes IGV picks up start and end offset from location property and is capable to select appropriate source range. CCing @tkrodriguez, @sdedic, @Akirathan

JaroslavTulach · 2025-01-18T16:06:46Z

...zer/SourceRepository/src/main/java/org/graalvm/visualizer/source/impl/ui/LocationOpener.java

+
+    private Line findLine(EditorCookie cake, int line) {
+        try {
+            return cake.getLineSet().getOriginal(line - 1);


Possibly IGV should honor https://www.graalvm.org/graphio/javadoc/jdk/graal/compiler/graphio/GraphLocations.html#locationOffsetStart(java.lang.Object) when the line isn't positive.

JaroslavTulach · 2025-01-20T13:38:41Z

...zer/SourceRepository/src/main/java/org/graalvm/visualizer/source/impl/ui/LocationOpener.java

@@ -63,24 +67,48 @@ private void openOrView(boolean focus) {
            return;
        }
        int line = location.getLine();
+        int[] offsets = location.getOffsetsOrNull();


Text based languages (like Truffle ones) are likely to fill offsets rather than line numbers. Passing the offsets to the LocationOpener in faba84a and using the offsets rather than line when present. More an experiment for your consideration than bullet proof code right now.

I don't have much opinion since I never use this functionality so you should make it work in some rational way. If the two ways of addressing the text disagree then it's unclear what you should do. Naively I might assume that the line number is more to be trusted given that offsets seem a bit squishy given differences in line terminations between windows and unix, plus the lack of actual terminator characters in a UI element that displays text. How are these mismatches expected to be dealt with?

re. different newline terminators: the information about the OS the code was compiled in is lost during the compilation step, and cannot be even dumped to BGV. Maybe a hint for truffle to represent positions using line:column pair rather than offsets.

From the user's standpoint I agree that the whole line is too coarse; it's far better to point at a specific position, if we have such data.

I suggest to make an option for this: if the IGV knows (the user configures it) the line terminator use on the compiling machine, it can then recompute the offset to this machine's newline style and to line:column coordinates.

I hope we'll find a way that works for current as well as future use-cases without a configuration option. I hope #10510 (comment) is a sound approach to achieve so.

…(if provided)

tkrodriguez · 2025-01-23T19:26:23Z

...zer/SourceRepository/src/main/java/org/graalvm/visualizer/source/impl/ui/LocationOpener.java

+            task.removeTaskListener(whenShowing[0]);
+            Mutex.EVENT.postReadRequest(() -> {
+                if (offsets == null) {
+                    StatusDisplayer.getDefault().setStatusText(Bundle.ERR_LineNotFound());


I'm not sure if I'm understanding the control flow here but it looks to me that if the location had a valid line but no offsets you'd report ERR_LineNotFound. Am I missing some subtlety?

I think you are right Tom, 8955e69 would address that

I have to admit I am not sure if JEditorPane.select is the right way to highlight the selection

maybe there is something better like attaching some annotation to a Line.Part

I can try to implement whatever you guys think is the best way to handle the highlight

Thinking a bit more, the editor will have a position that corresponds to the line termination so a unix file and the editor pane should agree about offsets so I think select will do the right thing. I'm unsure what would happen if the underlying file had windows CR+LF terminators so it would be good to test that out and make it work correctly. I don't know anything about best practices in UI code so I can't offer an opinion about which API to use, but select seems reasonable to me.

sdedic · 2025-02-01T15:06:23Z

...zer/SourceRepository/src/main/java/org/graalvm/visualizer/source/impl/ui/LocationOpener.java

@@ -63,24 +67,48 @@ private void openOrView(boolean focus) {
            return;
        }
        int line = location.getLine();
+        int[] offsets = location.getOffsetsOrNull();


re. different newline terminators: the information about the OS the code was compiled in is lost during the compilation step, and cannot be even dumped to BGV. Maybe a hint for truffle to represent positions using line:column pair rather than offsets.

From the user's standpoint I agree that the whole line is too coarse; it's far better to point at a specific position, if we have such data.

I suggest to make an option for this: if the IGV knows (the user configures it) the line terminator use on the compiling machine, it can then recompute the offset to this machine's newline style and to line:column coordinates.

...zer/SourceRepository/src/main/java/org/graalvm/visualizer/source/impl/ui/LocationOpener.java

JaroslavTulach · 2025-02-02T04:08:52Z

Thank you for the reviews. Yup, line endings may cause some misalignment. Let's try:

if there are no offset information, use line (just like we did so far)
if there is no line information, use offset (possibly accepting misalignment of Windows)
if there are both information present
- compute the line from offset
  - if the lines are different - use just line (to keep current behavior)
  - if the lines are the same - select the column region on the line

That way we favor existing behavior and enhance it when additional information is present. I'll write some tests and rewrite the logic now.

JaroslavTulach · 2025-02-02T05:43:19Z

Maybe a hint for truffle to represent positions using line:column pair rather than offsets.

Shouldn't we rather specify that offsets in the IGV treat new line as a single character? That's how Truffle Source does it and also how NetBeans/Swing javax.swing.text.Document does it. Things will be simpler then.

Given Enso is the first system trying to use the offsets, we can make some small adjustments to the specification to make things easier.

JaroslavTulach · 2025-02-02T06:05:43Z

With c4f059d I like what I see, CCing @Akirathan:

Looks like with this PR the IGV can work for Enso IR graphs well enough. I hope that with #10510 (comment) we have a good compromise to keep the existing behavior unmodified.

Can you do some testing on your side and possibly integrate this enhancement? It would increase our (Enso compiler team) productivity when exploring Enso IR graphs a lot. Btw. We'd be in a need of a new IGV release then...

Jaroslav Tulach added 2 commits January 18, 2025 07:39

More robust agsint NPE

9057b04

Open at least the file, when a line number isn't valid

75d41ee

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jan 18, 2025

JaroslavTulach commented Jan 18, 2025

View reviewed changes

enso-bot bot mentioned this pull request Jan 20, 2025

Function metadata call enso-org/enso#5226

Open

Keep startOffset and endOffset and use it instead of line when present

faba84a

JaroslavTulach commented Jan 20, 2025

View reviewed changes

fniephaus requested a review from tkrodriguez January 21, 2025 13:34

JaroslavTulach changed the title ~~More robust visualizer when reading custom .bgv file~~ Use offsets from .bgv file to select appropriate text Jan 22, 2025

Jaroslav Tulach added 2 commits January 22, 2025 18:18

Offsets are now part of Location equality

97baf8f

Do Line.show (if line exists) and then select range based on offsets …

b7bb680

…(if provided)

tkrodriguez reviewed Jan 23, 2025

View reviewed changes

Only warn when neither line nor offsets are available

8955e69

sdedic reviewed Feb 1, 2025

View reviewed changes

Unit testing LocationOpener

9ee80e5

Use Annotation rather than playing with JEditorPane

c4f059d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use offsets from `.bgv` file to select appropriate text #10510

Use offsets from `.bgv` file to select appropriate text #10510

JaroslavTulach commented Jan 18, 2025 •

edited

Loading

JaroslavTulach Jan 18, 2025

JaroslavTulach Jan 20, 2025

tkrodriguez Jan 23, 2025

sdedic Feb 1, 2025

JaroslavTulach Feb 2, 2025

tkrodriguez Jan 23, 2025

JaroslavTulach Jan 23, 2025

tkrodriguez Jan 23, 2025

sdedic Feb 1, 2025

JaroslavTulach commented Feb 2, 2025

JaroslavTulach commented Feb 2, 2025

JaroslavTulach commented Feb 2, 2025

Use offsets from .bgv file to select appropriate text #10510

Are you sure you want to change the base?

Use offsets from .bgv file to select appropriate text #10510

Conversation

JaroslavTulach commented Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach commented Feb 2, 2025

JaroslavTulach commented Feb 2, 2025

JaroslavTulach commented Feb 2, 2025

Use offsets from `.bgv` file to select appropriate text #10510

Use offsets from `.bgv` file to select appropriate text #10510

JaroslavTulach commented Jan 18, 2025 •

edited

Loading