Open DanTup opened 10 months ago
aiday-mar
commented
10 months ago Hi @DanTup thank you for posting this issue. I see how the sizing that is rendered can be confusing. So far we have decided to use the strategy I described in https://github.com/microsoft/vscode/issues/178670, but we may decide to implement a way to remember size per token location.
VSCodeTriageBot
commented
10 months ago This feature request is now a candidate for our backlog. The community has 60 days to upvote the issue. If it receives 20 upvotes we will move it to our backlog. If not, we will close it. To learn more about how we handle feature requests, please see our documentation.
Happy Coding!
DanTup
commented
10 months ago @aiday-mar I'm still not sure I understand. Even if it can't persist, why can't it have a smaller default size?
aiday-mar
commented
10 months ago Hi like I mentioned in the other issue. The size of the hover is determined by the following equations:
const currentMaxHeight = Math.max(this._editor.getLayoutInfo().height / 4, 250, lastHeight);
const currentMaxWidth = Math.max(this._editor.getLayoutInfo().width * 0.66, 500, lastWidth);By looking at the GIF you sent, it appears there may be a bug in the rendering. The color picker does look wider than it should be. This requires some investigation.
DanTup
commented
10 months ago const currentMaxWidth = Math.max(this._editor.getLayoutInfo().width * 0.66, 500, lastWidth);This line looks like it will always take at least 66% of the width of the editor. Is that intended? This seems kinda silly if you only have a single file open and are on an ultrawide monitor (it would be even worse on a super-ultrawide).
aiday-mar
commented
10 months ago Yes that is a good point, we'll look into it when there is time
aiday-mar
commented
10 months ago Hi I am trying to reproduce the error, but when I make the color picker appear it appears normal sized. Could you please post the example code in the GIF here? What type of language is this? Are you perhaps using some extensions related to this language that could be affecting the color picker?
DanTup
commented
10 months ago Interesting - it does seem to depend on the language, it's much smaller for CSS:
I'm using Dart/Flutter. It reproduces in a new Dart file with just the following:
import 'package:flutter/material.dart';
const a = Colors.red;You'd likely need a Flutter SDK set up to trigger this, but I've included the LSP traffic below. While pasting it in I realised that the hover is also being requested, and is visible if I made the colour picker taller. However, if I disable the colour picker and hover the same symbol, the hover window is still far smaller so it still doesn't explain everything:
[4:22:31 PM] [Analyzer] [Info] ==> {"jsonrpc":"2.0","id":93,"method":"textDocument/colorPresentation","params":{"color":{"red":0.9568627450980393,"green":0.2627450980392157,"blue":0.21176470588235294,"alpha":1},"textDocument":{"uri":"file:///C%3A/Dev/Test%20Projects/flutter_counter/lib/color.dart"},"range":{"start":{"line":2,"character":10},"end":{"line":2,"character":20}}},"clientRequestTime":1701879751209}
[4:22:31 PM] [Analyzer] [Info] ==> {"jsonrpc":"2.0","id":94,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///C%3A/Dev/Test%20Projects/flutter_counter/lib/color.dart"},"position":{"line":2,"character":10}},"clientRequestTime":1701879751210}
[4:22:31 PM] [Analyzer] [Info] <== {"id":94,"jsonrpc":"2.0","result":{"contents":{"kind":"markdown","value":"```dart\nabstract final class Colors\n```\n*package:flutter/src/material/colors.dart*\n\n---\n[Color] and [ColorSwatch] constants which represent Material design's\n[color palette](https://material.io/design/color/).\n\nInstead of using an absolute color from these palettes, consider using\n[Theme.of] to obtain the local [ThemeData.colorScheme], which defines\nthe colors that most of the Material components use by default.\n\n\nMost swatches have colors from 100 to 900 in increments of one hundred, plus\nthe color 50. The smaller the number, the more pale the color. The greater\nthe number, the darker the color. The accent swatches (e.g. [redAccent]) only\nhave the values 100, 200, 400, and 700.\n\nIn addition, a series of blacks and whites with common opacities are\navailable. For example, [black54] is a pure black with 54% opacity.\n\n\nTo select a specific color from one of the swatches, index into the swatch\nusing an integer for the specific color desired, as follows:\n\n```dart\nColor selection = Colors.green[400]!; // Selects a mid-range green.\n```\n\nEach [ColorSwatch] constant is a color and can used directly. For example:\n\n```dart\nContainer(\n color: Colors.blue, // same as Colors.blue[500] or Colors.blue.shade500\n)\n```\n\n## Color palettes\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n## Blacks and whites\n\nThese colors are identified by their transparency. The low transparency\nlevels (e.g. [Colors.white12] and [Colors.white10]) are very hard to see and\nshould be avoided in general. They are intended for very subtle effects.\n\n\n\n\nThe [Colors.transparent] color isn't shown here because it is entirely\ninvisible!\n\nSee also:\n\n * Cookbook: [Use themes to share colors and font styles](https://flutter.dev/docs/cookbook/design/themes)"},"range":{"end":{"character":16,"line":2},"start":{"character":10,"line":2}}}}
[4:22:31 PM] [Analyzer] [Info] <== {"id":93,"jsonrpc":"2.0","result":[{"label":"Color.fromARGB(255, 244, 67, 54)","textEdit":{"newText":"Color.fromARGB(255, 244, 67, 54)","range":{"end":{"character":20,"line":2},"start":{"character":10,"line":2}}}},{"label":"Color.fromRGBO(244, 67, 54, 1)","textEdit":{"newText":"Color.fromRGBO(244, 67, 54, 1)","range":{"end":{"character":20,"line":2},"start":{"character":10,"line":2}}}},{"label":"Color(0xFFF44336)","textEdit":{"newText":"Color(0xFFF44336)","range":{"end":{"character":20,"line":2},"start":{"character":10,"line":2}}}}]}Hi @DanTup I am trying to reproduce this issue. I believe this should be reproducable with the right extensions installed (since they provide the colors). With the main Flutter and Dart extensions, I am currently not able to reproduce this issue. Could you share please the extensions you are using?
DanTup
commented
10 months ago I don't think it's caused by other extensions, but here's the list:
PS C:\Dev> code --list-extensions
amodio.tsl-problem-matcher
Dart-Code.dart-code
Dart-Code.flutter
dbaeumer.vscode-eslint
dcmdev.dcm-vscode-extension
esbenp.prettier-vscode
GitHub.copilot
GitHub.copilot-chat
googlecloudtools.cloudcode
ms-vscode-remote.remote-containers
ms-vscode-remote.remote-wsl
ms-vscode.powershell
ms-vsliveshare.vsliveshareCan you provide a screenshot of what you see, and confirm which version of the Flutter SDK you're using? The SDK version is shown in the language status area:
(the colours won't show up without a Flutter SDK because they're coming from the Dart LSP server).
aiday-mar
commented
10 months ago Hi @DanTup thank you for the explanation. I was finally able to reproduce the error by downloading the flutter sdk. I will look into it.
aiday-mar
commented
10 months ago I have looked at the code and it appears this happens in the case when in the hover there is a color picker and there is markdown content. When there is markdown content, the hover will assume the natural width of the markdown content thereby stretching the color picker. A fix here would be to limit the width of the hover when a color picker is present. This however means that the markdown content below the color picker will be squeezed into a smaller space.
DanTup
commented
10 months ago @aiday-mar I don't understand why it's wider than the markdown hover on its own. In my comment above (https://github.com/microsoft/vscode/issues/199944#issuecomment-1843241066) the last screenshot shows the markdown/hover content without the colour picker, and it's much narrower. If the colour picker was only the same width as that markdown hover that would be fine, but for some reason the colour picker (with the markdown content) is over 2x the width of when only the markdown content is shown.
aiday-mar
commented
10 months ago There appears to be another bug in general with your content hover. In the second image, the hover should not be so wide. It looks very wide and should be the size of the content inside of it.
In the screenshots you have provided, is there content below the color picker that is not visible in the image? Is that content as wide as the current hover width?
DanTup
commented
10 months ago Aaah, I see the difference. When I compared above, I was hovering over "Red" but didn't realise that hovering over the color picker is essentially hovering over "Color". So the documentation shown in each screenshot above was not the same.
If I disable the color picker and instead hover over Color, then I see the same width:
So the issue is unrelated to the color picker and is something related to how the width is being computed for that markdown content. Scrolling down, I don't see anything that is anywhere near as wide as the hover. I've included the full markdown here:
I'll have a quick look if I can figure out which part of the content is causing it.
DanTup
commented
10 months ago I reduced the repro to just this:
/// These colors are identified by their transparency. The low transparency
/// levels (e.g. [Colors.white12] and [Colors.white10]) are very hard to see and
/// should be avoided in general. They are intended for very subtle effects.
class Aaaaaaaaa {}
void f() {
Aaaaaaaaa? a;
}For some reason the hover does not want to wrap that paragraph, and makes the hover really wide. The LSP response for the hover looks like this:
{"id":551,"jsonrpc":"2.0","result":{"contents":{"kind":"markdown","value":"```dart\nclass Aaaaaaaaa\n```\n*package:flutter_counter/color.dart*\n\n---\nThese colors are identified by their transparency. The low transparency\nlevels (e.g. [Colors.white12] and [Colors.white10]) are very hard to see and\nshould be avoided in general. They are intended for very subtle effects."},"range":{"end":{"character":11,"line":12},"start":{"character":2,"line":12}}}}So according to the equation in https://github.com/microsoft/vscode/issues/199944#issuecomment-1838645790, the initial maximum width can be up to the editor width times 0.66. Would you say that this roughly corresponds to what you are seeing? Is the hover width indeed 2/3 of the editor width?
We can consider making that fraction smaller in order to avoid cases like yours.
Your editor looks very wide, so the hovers are very wide too think.
DanTup
commented
10 months ago When I hover over "Color" (without a color picker), the total editor width was 2913 and the hover was 1690 (so around 58%). So it's certainly smaller than the maximum, but the maximum being 66% of the editor seems a bit excessive to me if you consider that I might have my editor full-screen on an ultrawide monitor and I might have only opened one file. Here's a fullscreen screenshot for reference:
I don't know what a reasonable maximum size is though - maybe if my docs had a 1600px wide image in it, I would want it this wide. But generally reading a hover at that width with really long lines of text doesn't seem ideal.
DanTup
commented
10 months ago Actually, to be clear - the real issue here isn't so much the maximum size - it's that if I resize it to be 1/3 of the current size, then it does not persist that.
I could live with the defaults being bad if it at least saved when I tried to resize it smaller. The issue here is that I have no way to fix it, they'll always expand to this size.
aiday-mar
commented
10 months ago Hi thanks for the clarification. Yes those are all good points. IMO we should make the hover persist on a per token basis like you mentioned, which would be more intuitive. There have been some discussions in the team, and overall the consensus was to keep the current persistance mechanism as is. I will work on a PR to make the defaults smaller than what you showed, this should at least improve the experience.
DanTup
commented
10 months ago IMO we should make the hover persist on a per token basis like you mentioned, which would be more intuitive.
Depending on what you mean by "per token", that might not be what I was asking for. I don't want to see this same overly-wide hover/color picker as I move around the code. I'd prefer there was just a single size for a) all hovers and b) all colour pickers, and when I resized one, the next one was that size regardless of where it was.
However, if the defaults were more reasonable, I'd probably care less about that. It's just really annoying to have to move my mouse so far 😄
That said - I'm now wondering why the hover content affects the width of the colour picker anyway. Why can't the hover by wider (for the documentation) without causing the colour picker to stretch? I don't think anyone would really expect the aspect ratio of the colour picker to vary depending on the documentation comments of the class next to the colour picker? 🤔
aiday-mar
commented
10 months ago Yes we have considered making the content hover the same size for all hovers but that would not look good because content hovers are of different sizes. Consider the two following hovers, we can not equate them:
I think we can make the color picker have a fixed size and always center it in the middle, but I think that some users explicitly want to increase the size of the color picker in order to better see the colors. I am not sure if we should make it fixed size.
Like you said however the color picker size currently varies with the documentation comments below it, so we can decide to force a smaller rendering width when a color picker is present in the hover. If there is a color picker, the content hover will be smaller size.
Additionally we can decrease the defaults
DanTup
commented
10 months ago Yes we have considered making the content hover the same size for all hovers but that would not look good because content hovers are of different sizes. Consider the two following hovers, we can not equate them:
Can't you have a default/user-persisted hover size and then just shrink it if the content is small (but do not overwrite the user-persisted preference when you do this)? If I resize a hover, I would really not expect it to persist that size for that one very specific token in that file. Almost every time I invoke a hover it will be in a different location, so it seems like my "preference" would almost never be used?
I think we can make the color picker have a fixed size and always center it in the middle
Why center it? My concern here is how far I have to move my mouse so far from where I hovered to select a color and centering it won't solve that problem.
but I think that some users explicitly want to increase the size of the color picker in order to better see the colors
Hmm, that's fair. Maybe the real issue here then is that the colour picker and hover documentation are being lumped together... Is there a great benefit in showing the doc comments hidden below the colour picker? If they weren't there, the width of the colour picker and the width of documentation hovers would be less tied together.
Although again, my real issue here is with the incredibly wide default rendering.. I suspect all these other gripes would be less of a problem if the default was better :)
aiday-mar
commented
10 months ago I see yes that is an idea. Currently when one changes the size, it persists the current size as the new upper bounded size. If the next hover is smaller it assumes a smaller size, if it is bigger, it is limited by the previous upper bounded size. I think you are suggesting to have maybe some sort of setting of type maximum natural width and height of hover which would take two entries for the width and height, and these could be used instead? I opened a feature request here to track this https://github.com/microsoft/vscode/issues/200356
I don't think we are currently planning on separating the color picker from the markdown content in the hover. If you feel this is a feature you would really like to see in VS Code, you may open an issue and we can discuss this there.
I opened a PR here https://github.com/microsoft/vscode/pull/200355 in order to change how the default maximum natural width and height are calculated. The calculation is as follows:
const height = Math.max(Math.min(this._editor.getLayoutInfo().height / 4, 400), ContentHoverWidget._lastDimensions.height);
const width = Math.max(Math.min(this._editor.getLayoutInfo().width * 0.66, 800), ContentHoverWidget._lastDimensions.width);In particular this means that the width and height will never be bigger initially than 800 pixels and 400 pixels respectively, even if you have a very wide editor.
This calculation can be combined with the setting I mentioned earlier, so one could tweak the values 400 and 800 through the settings.
aiday-mar
commented
9 months ago Hi @DanTup after some discussion with the team, we decided to limit the width of the color picker to a certain maximum width, even if the hover is larger. We will be going forward with this decision.
DanTup
commented
9 months ago we decided to limit the width of the color picker to a certain maximum width
Do you mean an absolute value? And not "some % of the editor width"? If so (and it's not a huge value), that sounds good to me :-)
aiday-mar
commented
9 months ago So I think we will set an absolute maximum value on the color picker
VSCodeTriageBot
commented
9 months ago This issue has been closed automatically because it needs more information and has not had recent activity. See also our issue reporting guidelines.
Happy Coding!
This was discussed a little in #178670, but I'm not sure I totally understand the logic behind it sometimes not saving my size. Additionally, it now seems even wider than it was back then.
https://github.com/microsoft/vscode/assets/1078012/e4ed2947-e391-4834-a899-c0f13559cc12