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

Pretty print code blocks according to widget size #1897

Merged
merged 7 commits into from
Jun 16, 2024
Merged

Conversation

nitinprakash96
Copy link
Collaborator

@nitinprakash96 nitinprakash96 commented Jun 4, 2024

Closes #1794

Changes include:

  • Introduce prettyTextWidth that takes in number of allowed characters in a line as an argument.
  • Introduce pparens' which is mostly similar to pparens except that it only encloses a document
    in parantheses and does not do anything else.
  • Make type signatures in renderers aware of the widget width.
  • Replace usage of hardline with softline for multiline pretty printing.

@nitinprakash96 nitinprakash96 requested review from byorgey and kostmo June 4, 2024 06:03
@byorgey
Copy link
Member

byorgey commented Jun 4, 2024

I am not sure that prettyTextLine is the right way to solve this. It works by doing layout with an unbounded width, which will never introduce newlines at all. If we are printing a code block that is too wide for its text box I think that means it will just run off the right side instead of wrapping appropriately.

@nitinprakash96
Copy link
Collaborator Author

It's true that prettyTextLine works using Unbounded width. But it doesn't run off the right side. It just means that newlines won't be introduced but everything will be printed in one line inside the available area (that's the closest I can get to interpreting it). And for our case, the widget is the available area. Here's what it ends up looking like:

CleanShot 2024-06-04 at 18 03 56@2x

@byorgey
Copy link
Member

byorgey commented Jun 4, 2024

Ah, OK, that's better than I thought. In that case I think we can merge this, since it's definitely better than the status quo.

However, I think we should leave #1794 open. The pretty-printer is carefully written to insert appropriate line breaks depending on how much horizontal space is available (the reason it looks so bad right now is that for some reason it thinks there is very little horizontal space so it inserts every possible line break). So what I had in mind is to query brick for the amount of horizontal space we have to work with, and then render the code block with a layout configured specifically for the given width. Of course this will require some refactoring.

@nitinprakash96
Copy link
Collaborator Author

Let me sleep over it and revisit tomorrow. Probably there is a better solution that this. Even I'm a little uncomfortable with Unbounded page width.

@nitinprakash96
Copy link
Collaborator Author

nitinprakash96 commented Jun 4, 2024

There's also this usage of hardline:

multiLine l r = l <+> "->" <> hardline <> r

which causes the newline break right after -> in type signatures. I think this is where we will need to refactor things otherwise it won't really matter if we query for widget size because we will get a line break anyway.
Alternative would be to use softline' which takes page width into account.

@nitinprakash96
Copy link
Collaborator Author

I made a few changes in my latest commit:

  • Introduce softline' for multiline comment pretty printing instead of hardline. This ensure that line breaks happen
    only when we run out of allowed characters per line (allowed width).
  • Introduce prettyTextWidth which takes and additional Int implying the number of allowed characters before introducing a line break.
  • Make the commands being rendered in bottom left widget width-aware. This ensure they take up available space.

This seems like a better solution imo than Unbounded width in my previous commit.

As far as code blocks are rendered, I don't think there is way to make that widget size aware? I'm talking about this:

LeafCodeBlock _i t -> [CodeNode (prettyText t)]

It just allows the width from defaultLayoutOptions to take effect as part of prettyText.

[ hBox
[ padRight (Pad 1) (txt . syntax $ constInfo c)
, padRight (Pad 1) (txt ":")
, withAttr magentaAttr . txt $ prettyTextWidth (inferConst c) w
Copy link
Member

Choose a reason for hiding this comment

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

Using the width w of the widget to pretty-print the type signature here is not quite right, because w is the width of the entire widget, but the type signature is not going to be printed starting in the first column of the widget; there is a command name and colon that come before it. You can see the issue in the following screenshots. In the first screenshot, you can see that the type of installKeyHandler is cut off, because the pretty-printer thought it had the entire width of the widget to work with, so it laid out the type on a single line:

prettynowrap

In the next screenshot, you can see that in a much narrower window, the pretty-printer does actually insert some newlines into the type, but it still gets cut off on the right:

prettywrap

I can imagine two things to do here.

  • The obvious thing would just be to subtract the width of the constant name + colon (+ two spaces) from w before passing it to prettyTextWidth.
  • The nicer, slightly more complex approach would be:
    1. first try printing the type on a single line with prettyTextLine and see whether the command name, colon, + pretty-printed type will fit within w; if so, great.
    2. If not, instead use prettyTextWidth to print the type with a width of w - 2 and then print the type indented by 2 spaces on a new line(s) after the command + colon.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Update on this:
I managed to get rendering a little bit better but need to test the implemented a bit more. Here's how the above screenshot renders now:
CleanShot 2024-06-14 at 00 46 32@2x

Hopefully this is what you had in mind @byorgey ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that looks nice!

@xsebek
Copy link
Member

xsebek commented Jun 9, 2024

@nitinprakash96, thanks for taking on this issue. 👍

I remember getting the layout right in #1464 was tricky and the layout issues persisted.
Apparently, I was lazy and did not write any unit tests - if you could fix my shortcomings and write some
that lay out the command description on short and long lines, that would be great.

@nitinprakash96
Copy link
Collaborator Author

@xsebek yeah I agree that getting layout right is a bit tricky 😅 still trying to figure out how this can be done nicely. But I think I should be able to get this done given I was able to figure out a few things in my last commit. I'll see if I can implement what Brent suggested in one of his reviews.

@nitinprakash96 nitinprakash96 requested a review from byorgey June 16, 2024 03:31
@nitinprakash96
Copy link
Collaborator Author

I think this should now be ready ( - unit tests).

Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

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

Looks great! I ran it and resized a terminal window continuously while looking at a complex type in the info box --- it flawlessly adapted to the width.

I don't really know how to make unit tests for this (in general it is hard to make tests for TUI things). If you can think of a way, go for it, but I think we can merge this as is!

let w = ctx ^. availWidthL
constType = inferConst c
info = constInfo c
requiredWidthForTypes = textWidth (syntax info <> " : " <> prettyTextLine constType)
Copy link
Member

Choose a reason for hiding this comment

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

Today I learned about textWidth! I didn't know about this before. There are probably some other places in the codebase where we ought to be using it instead of length! https://hackage.haskell.org/package/brick-2.3.1/docs/Brick-Widgets-Core.html#t:TextWidth , for anyone else reading this.

Copy link
Member

Choose a reason for hiding this comment

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

@byorgey it will be more noticeable with Unicode and different terminals; see:
https://github.com/jtdaugherty/vty?tab=readme-ov-file#multi-column-character-support

@nitinprakash96 nitinprakash96 added the merge me Trigger the merge process of the Pull request. label Jun 16, 2024
@mergify mergify bot merged commit e94dd54 into main Jun 16, 2024
11 checks passed
@mergify mergify bot deleted the nitin/pretty branch June 16, 2024 04:18
@byorgey byorgey added the CHANGELOG Once merged, this PR should be highlighted in the changelog for the next release. label Jun 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CHANGELOG Once merged, this PR should be highlighted in the changelog for the next release. merge me Trigger the merge process of the Pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pretty-printing that takes brick widget size into account
3 participants