You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
format documents when hover like in android studio #3357
@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:
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?
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:
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.)
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)
Should display strings always be formatted this way, or opt-in so callers of getDisplayString() can explicitly choose to accept multiline displays?
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.
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).
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).
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.
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.
@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).
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!
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?
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.
Activity
DanTup commentedon May 24, 2021
@bwilkerson this string comes from
Hover.elementDescriptionwhich useselement.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?
bwilkerson commentedon May 24, 2021
I wouldn't mind having the logic in
ElementDisplayStringBuilderand 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:
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 commentedon Jun 2, 2021
Makes sense to me. Some questions:
(another thought I had was to use the underlying source, though I'm not sure if that's accessible or makes sense either)
getDisplayString()can explicitly choose to accept multiline displays?bwilkerson commentedon Jun 2, 2021
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.
I don't know. I'd have to look at where
getDisplayStringis 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 commentedon Jun 2, 2021
Yeah, good point.
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 commentedon Jun 2, 2021
SGTM
DanTup commentedon 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):
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 commentedon 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 commentedon Jun 9, 2021
@alexander-doroshko @jwren Any interest in having this text formatted on server for IntelliJ?
alexander-doroshko commentedon Jun 9, 2021
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 commentedon 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 commentedon Jun 9, 2021
I don't remember other places. Probably only hovers.
[analyzer] Format signatures in LSP hovers over multiple lines
DanTup commentedon 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 commentedon Jun 10, 2021
Thanks! I'll check and answer later today.
alexander-doroshko commentedon Jun 10, 2021
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?
Dart Analysis Server returns formatted HoverInformation.getElementDes…
Dart Analysis Server returns formatted HoverInformation.getElementDes…
DanTup commentedon Jun 13, 2021
@alexander-doroshko
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 commentedon 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).
Dart Analysis Server returns formatted HoverInformation.getElementDes…
Dart Analysis Server returns formatted HoverInformation.getElementDes…