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

Use prettier arrow in stack traces (to highlight the line with the error) #43237

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

holly-cummins
Copy link
Contributor

@holly-cummins holly-cummins commented Sep 12, 2024

TLDR: the 'after'

EDIT: After testing on Windows, I've changed the arrow.

image

What problem were we solving?

In @phillip-kruger's recent demo of the advanced exception context, I noticed that we use an ascii-art arrow.
image

"Hmm," I thought. "With so many nice arrows in the unicode spec these days, why don't we use one of them instead?" With hindsight, I should have asked @phillip-kruger if he’d tried this already, but I thought “I won’t bother him with a conversation for a one-character change, I’ll just do the PR and we can discuss it there.” Then I started testing, and realised why Philip maybe opted for the ascii art. But by then it was too late, and I was invested.

image

There are two risks with this change:

  • Will the character be available in the terminal and browser font on the developer's system?
  • Can we make the code in the line with the arrow align properly with the rest?

It turns out both are a problem, but I think I got there in the end.

Cross-platform test results

MacOS

image image

Windows

image

image

Linux

image

image

The workings-out

Character width issues

Even on a single machine, I got character-alignment problems. It turns out that fixed-width fonts do not have a consistent width for the arrows. It will presumably vary by OS, and it also varies between browser and terminal. I liked the heavy arrow ⮕, but that was subtly misaligned in the browser:

image

... and hopelessly misaligned in the terminal:

image

⟶ is nice and prominent, perfect in the console,

image

But terrible in the browser

image

In my experiments, ⇨ works in the browser:

image

and also the terminal

image

It's a bit more visible than ⇾, which also worked for me in terms of width:

image image

Character set issues

Once I found an arrow that rendered well in both terminal and browser on Mac, we tried it on Windows, and ... no good.

image

So I went back to the drawing board.

How to test

Here's some text which can be pasted into a terminal to test on other OSes:

Exception in GreetingResource.java:14
          12      @Produces(MediaType.TEXT_PLAIN)
          13      public String hello() {
        ⇨ 14          if (Math.random( )> 0.01) { throw new RuntimeException("test");}
          15          return "Hello from Quarkus RESTyes";
          16      }

(Testing in the browser would need a build of the patch.)

Other options

For reference, here are all the arrows I considered, along with a . to show alignment. Only the fourth one works on Windows:

⟶ .
⭢ .
⮕ .
→ .
⇨ .
⇢ .
⇾ .

@phillip-kruger
Copy link
Member

Very cool !! Thanks. I did not think of that at all :)

@gsmet
Copy link
Member

gsmet commented Sep 12, 2024

TBH, while not pretty, I think the previous behavior was a bit more visible (at least for me and my bad eyes). But I won't fight it :).

Also, we need to check how it behaves on Windows (as it has been quite a pain when going non-ascii in the term).

@holly-cummins
Copy link
Contributor Author

TBH, while not pretty, I think the previous behavior was a bit more visible (at least for me and my bad eyes). But I won't fight it :).

Yes, I agree it's a touch less visible now. I was sad that I couldn't get the width of the chunky arrow to work, since it was most visible.

Also, we need to check how it behaves on Windows (as it has been quite a pain when going non-ascii in the term).

100% I'm polling around a few developers who I think might have windows machines. :)

@holly-cummins holly-cummins marked this pull request as draft September 12, 2024 13:14
@holly-cummins
Copy link
Contributor Author

@gastaldi has kindly tested on Windows, and concerns were justified:
image

@gastaldi
Copy link
Contributor

Here is a screenshot in Edge (Windows 11):

image

@holly-cummins holly-cummins marked this pull request as ready for review September 12, 2024 14:48
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 12, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 718e182.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@gastaldi gastaldi merged commit 45a9d46 into quarkusio:main Sep 12, 2024
67 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 12, 2024
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.

4 participants