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

format documents when hover like in android studio #3357

Closed
tbm98 opened this issue May 22, 2021 · 17 comments
Closed

format documents when hover like in android studio #3357

tbm98 opened this issue May 22, 2021 · 17 comments
Labels
in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server is enhancement
Milestone

Comments

@tbm98
Copy link

tbm98 commented May 22, 2021

in android studio
Screen Shot 2021-05-22 at 21 14 06

in vscode
Screen Shot 2021-05-22 at 21 13 42

@DanTup
Copy link
Member

DanTup commented May 24, 2021

@bwilkerson this string comes from Hover.elementDescription which uses element.getDisplayString(). Android Studio seems to split this string up and insert the newlines/indenting here:

https://github.com/JetBrains/intellij-plugins/blob/0f07ca63355d5530b441ca566c98f17c560e77f8/Dart/src/com/jetbrains/lang/dart/ide/documentation/DartDocUtil.java#L76

LSP could do similar (in handler_hover.dart) though I wonder if it may be better if ElementDisplayStringBuilder handled this and was passed some flag to tell it to prefer formatting over multiple lines (which initially would just be passed through from the LSP handler) - any thoughts?

@DanTup DanTup added this to the v3.24.0 milestone May 24, 2021
@DanTup DanTup added in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server labels May 24, 2021
@bwilkerson
Copy link

I wouldn't mind having the logic in ElementDisplayStringBuilder and eventually move IntelliJ over to using the version from server.

If we're going to do this, it would be nice to improve on the display in IntelliJ. It doesn't always produce nice results, as evidenced by this screenshot:

image

I'd propose that if the line is too long and needs to be split up that we break before the every parameter (including the first) and that we indent a fixed number of spaces, more like what the formatter produces. (Bonus points if we can find a way to use the formatter so that it stays consistent over time.)

@DanTup
Copy link
Member

DanTup commented Jun 2, 2021

Makes sense to me. Some questions:

  1. Should it include trailing commas? If it always included them, then short methods would also be wrapped. If not, the last arg would go on the line with the paren. We could conditionally include them (eg. if string is over a certain length) but that feels a bit weird if the goal is to let the formatter do its thing.
    (another thought I had was to use the underlying source, though I'm not sure if that's accessible or makes sense either)
  2. Should display strings always be formatted this way, or opt-in so callers of getDisplayString() can explicitly choose to accept multiline displays?

@bwilkerson
Copy link

Should it include trailing commas?

Yes, the underlying source is available. I don't know whether we want to use it though because there could be additional text in it that we don't want to include in the output (comments, annotations on parameters, and default values come to mind).

As for using the formatter, I'm not actually certain that that's what we want to do. It was just a brainstorming idea. If we're composing the text to be formatted then it might be better to just control the format directly. We can't, for example, include the "(new) " or "(const) " prefix text when passing it to the formatter, so that might throw off the results. Using the formatter is probably more work than it's worth.

Should display strings always be formatted this way, or opt-in ...

I don't know. I'd have to look at where getDisplayString is being called to know what impact that decision would have. For example, if we're using it to compose diagnostic messages, then I think we need to preserve an unformatted version because some clients probably can't display multi-line messages.

@DanTup
Copy link
Member

DanTup commented Jun 2, 2021

We can't, for example, include the "(new) " or "(const) " prefix text when passing it to the formatter, so that might throw off the results. Using the formatter is probably more work than it's worth.

Yeah, good point.

if we're using it to compose diagnostic messages, then I think we need to preserve an unformatted version because some clients probably can't display multi-line messages

I'm not sure about diagnostics, but it is called in quite a few places - some of which I'm not certain where they'd be used (for example it's included in some of the protocol messages back to the client). I think it might be better to keep opt-in and just enable in specific places we know can handle it (and if over time this becomes all of them, the option could always be removed).

@bwilkerson
Copy link

I think it might be better to keep opt-in and just enable in specific places ...

SGTM

@DanTup
Copy link
Member

DanTup commented Jun 9, 2021

I've got a change at https://dart-review.googlesource.com/c/sdk/+/202962/, here are some examples of how it looks (including the bad IntelliJ example from above):

Screenshot 2021-06-09 at 15 32 38

Screenshot 2021-06-09 at 15 33 17

Screenshot 2021-06-09 at 15 33 40

Right now it's enabled only for Hovers in LSP, and only for the main executable elements (function types within are kept on a single line.. I did initially wrap them and further indent, but it didn't seem like an improvement having multiple indent levels).

@tbm98
Copy link
Author

tbm98 commented Jun 9, 2021

It looks good and easier to read. Thank you :D
I think function types within are kept on a single line is better separate them into multiple lines, and almost case they are not too long.

@bwilkerson
Copy link

@alexander-doroshko @jwren Any interest in having this text formatted on server for IntelliJ?

@alexander-doroshko
Copy link

Any interest in having this text formatted on server for IntelliJ?

Sure. I'm not aware of any reasons not to be interested in this :). I'm ready to add a switch to the Dart plugin to tweak behavior based on the SDK version.

@DanTup
Copy link
Member

DanTup commented Jun 9, 2021

@alexander-doroshko is there anywhere other than hovers that you also do this formatting? If so, it may be worth enabling them in those places at the same time (so far for LSP I only enabled it for hovers).

@alexander-doroshko
Copy link

I don't remember other places. Probably only hovers.

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Jun 9, 2021
Fixes Dart-Code/Dart-Code#3357.

Change-Id: I999ca8d3ffb7311ea4f60586e75160ae87292a77
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/202962
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
@DanTup
Copy link
Member

DanTup commented Jun 10, 2021

@alexander-doroshko I have a change to enable this for non-LSP here:

https://dart-review.googlesource.com/c/sdk/+/202971/

I've also increased the protocol version (to 1.32.7) as a convenient way to detect this. Do you need to ship a change before this lands or will the current code still behave reasonably in the meantime? (If so we might want to delay landing this until you've shipped). Thanks!

@alexander-doroshko
Copy link

Thanks! I'll check and answer later today.

@alexander-doroshko
Copy link

Go ahead and land it! I'll publish the plugin updates shortly. There won't be dramatic problems until people update the plugin.
This will go into Dart SDK 2.14, won't it?

SergeyZh pushed a commit to JetBrains/intellij-plugins that referenced this issue Jun 11, 2021
…cription() starting from SDK 2.14

Dart-Code/Dart-Code#3357

GitOrigin-RevId: 878d64731513d0677aa8d69dc29bb023475fc8de
SergeyZh pushed a commit to JetBrains/intellij-plugins that referenced this issue Jun 11, 2021
…cription() starting from SDK 2.14

Dart-Code/Dart-Code#3357
(cherry picked from commit 878d64731513d0677aa8d69dc29bb023475fc8de)

IJ-CR-10012

GitOrigin-RevId: 744917b9b8ad262c800886958df7cd5281333cd5
@DanTup
Copy link
Member

DanTup commented Jun 13, 2021

@alexander-doroshko

This will go into Dart SDK 2.14, won't it?

I would expect so. It would likely be included in some 2.14 pre-release builds (those made after it lands), but not in others (those that have already shipped). I don't know if that's important to you though. Using the protocol version would be most reliable, but I don't know if that's as simple for you.

I'll let you know/close this issue once it's merged.

@DanTup
Copy link
Member

DanTup commented Jun 13, 2021

This has landed in the SDK in dart-lang/sdk@d75becf :-)

It'll show up automatically in VS Code when using an SDK that includes that change (it doesn't need any changes here in the Dart extension).

@DanTup DanTup closed this as completed Jun 13, 2021
SergeyZh pushed a commit to JetBrains/intellij-plugins that referenced this issue Jun 14, 2021
…cription() starting from SDK 2.14

Dart-Code/Dart-Code#3357
(cherry picked from commit 878d64731513d0677aa8d69dc29bb023475fc8de)

IJ-CR-10012

GitOrigin-RevId: 61c8962331240a964e317899b4e192dcbf39552d
SergeyZh pushed a commit to JetBrains/intellij-plugins that referenced this issue Jun 14, 2021
…cription() starting from SDK 2.14

Dart-Code/Dart-Code#3357
(cherry picked from commit 878d64731513d0677aa8d69dc29bb023475fc8de)

IJ-CR-10012

GitOrigin-RevId: fd64c39676d31ef851f3d5477ff0749274a4fcd9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server is enhancement
Projects
None yet
Development

No branches or pull requests

4 participants