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

Error hover semantic information should be rendered separate of error message #62370

Closed
joaomoreno opened this Issue Nov 1, 2018 · 36 comments

Comments

@joaomoreno
Copy link
Member

joaomoreno commented Nov 1, 2018

From @octref in #62159

This is a regression on the diagnostics display.

1.28:

image

Insiders:

image

In both case, the diagnostics returned are:

const diagnostics = {
  code: "unknownProperties",
  source: "css.lint.unknownProperties",
  message: "Unknown property: 'foo1'"
}

If we have access to semantic information we should render it much better. The message should come first. Then the source on a separate line, with a Source: label preceding it and in non-monospace font (since source should be a user-readable string like CSS Lint). Then, the code with the same style, but with monospace, since code is often a library/OS error code. Something like:

Unknown property: 'foo1'

Source: CSS Lint
Code: unknownProperties

cc @sandy081 @ramya-rao-a @misolori

@octref

This comment has been minimized.

Copy link
Member

octref commented Nov 2, 2018

Another problem: changing source back to css is not nice in problems view. Previously I get:

image

Now I get:

image

Good things I miss from previous one:

  • source and code displayed together in the left, the most prominent & easily-scannable place
  • I'm able to see the code even in half screen, whereas if the message is long, I lose the code at the end
@jrieken

This comment has been minimized.

Copy link
Member

jrieken commented Nov 5, 2018

source and code displayed together in the left, the most prominent & easily-scannable place

While that is true, isn't the real question what value the code attribute has at all? Usually this is a number and for css its seems to be a close variant of the message. Are people reading/understanding diagnostics like: "Oh error TS33165, I better not assign a boolean to a string" or do they understand the message "Type boolean cannot be assigned to type string" better/faster? For me the code is something I would use when googling a problem I don't understand by reading the message and therefore the least important piece of information.

@octref

This comment has been minimized.

Copy link
Member

octref commented Nov 5, 2018

@jrieken
I see your point, but there are also cases when, say, a user is enforcing some new linter rules throughout a codebase. He would want to see which errors/warnings are caused by exactly these rules. He might also add pragmas to disable certain rules for some lines, when the rulename is useful.

From an extension author point of view, he doesn't care about passing in semantically correct info. He only knows the messages displayed on hover and problems are ${code} ${message} ${source} ${range}, and he tries to make best use of that format. We see that with HTML all the time, don't we...

@jrieken

This comment has been minimized.

Copy link
Member

jrieken commented Nov 5, 2018

which errors/warnings are caused by exactly these rules.

That sounds a little artificial but if that is the case, then I would probably search for that specific error code using the filter box on the upper right. (actually I wouldn't use the UI at all but I'd use the command line)

@kieferrm kieferrm referenced this issue Nov 12, 2018

Closed

Iteration Plan for November 2018 #62876

35 of 45 tasks complete
@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Nov 13, 2018

Inline widget renders the source and code similar to Problems view.
Hover renders the source and code as suggested in description

Example 1:

image

Example 2:

image

@jrieken @joaomoreno @misolori Feedback is welcome.

@jrieken jrieken removed their assignment Nov 13, 2018

@jrieken

This comment has been minimized.

Copy link
Member

jrieken commented Nov 13, 2018

I think having source and code on separate lines gives them way more attention than they should have. And to my eyes it also looks ugly. What again is wrong about the inline rendering and why is it only wrong in the hover?

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Nov 13, 2018

Since it is a suggestion from @joaomoreno I would request him to reply for above comment.

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Nov 13, 2018

Pushed changes in inline view - made source and code less prominent just like in problems view.

sandy081 added a commit that referenced this issue Nov 13, 2018

@octref

This comment has been minimized.

Copy link
Member

octref commented Nov 13, 2018

Error Code than Code would be more explicit and easy to understand in the inline hover. The [2551] (417, 98) in the problems view is mystic and hard to understand. Just my 2 cents.

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Nov 15, 2018

Made Hover consistent with Inline and Problems view

image

This is the best can be done before introducing more UI changes. I would not invest more as the current approach is OKish and do not see complains from users.

@sandy081 sandy081 closed this in aab56e9 Nov 15, 2018

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Nov 20, 2018

Since some of us does not like how the current metadata is shown in the hover I am proposing following options. Each option shows hover for single line and multi line errors.

  • Option 1 (Current)

screenshot 2018-11-20 at 16 38 28

screenshot 2018-11-20 at 16 38 42


  • Option 2 (As suggested in description)

screenshot 2018-11-20 at 16 37 29

screenshot 2018-11-20 at 16 37 51


  • Option 3 (Do not show metadata)

screenshot 2018-11-20 at 16 21 02

screenshot 2018-11-20 at 16 22 02

@Microsoft/vscode Please give the option you like

@sandy081 sandy081 reopened this Nov 20, 2018

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Nov 20, 2018

I vote for Option 3. Imho a user is not interested in the source and of course not the code.
I would always render source and code in the problems panel such that users who are interested in this can go to the view that provides more details. Similar analogy is debug hover, gives you minimalistic information, if you want more please open the debug viewlet.

If we decide to go with option 2 I suggest to right align it so it is less noticable and less space between it and the actual error.

@octref

This comment has been minimized.

Copy link
Member

octref commented Nov 20, 2018

If we decide to go with option 2 I suggest to right align it so it is less noticable and less space between it and the actual error.

Yeah, if we have

  • Minimal width of the diagnostics
  • Right align the source/code
  • Give source/code a smaller font size (maybe with a fading grey color too)

This would look great to me.

Option 3 would mean "I should not follow the semantics of the Diagnostic interface", because I need the source and code presented to users. Look at issue like vuejs/vetur#261.

I also hope code can be a link for use cases like vuejs/vetur#849. Otherwise the only way I can do it is to append code to message as a MD link.

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Nov 20, 2018

I think first we have to decide whether the metadata should be shown at all, or whether it should be configurable to be hidden, etc. Then, if it's shown, how it should be displayed.

I think it should be shown because if I have multiple extensions contributing diagnostics, I want to know where a message is coming from. The error code is also useful, e.g. if you want to know which rule in a tslint.json is associated with an error message. I prefer option 1.

@misolori

This comment has been minimized.

Copy link
Member

misolori commented Nov 20, 2018

Another thought is a hybrid of option 1 & 3 where we have some way for a user to expand the additional information but it's hidden by default (similar to the suggest widget info box).

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Nov 21, 2018

@roblourens @octref Source and code information are hidden only in hover. If you want you can find them in Problems view. Is not that enough and helpful?

@octref

This comment has been minimized.

Copy link
Member

octref commented Nov 21, 2018

Source and code information are hidden only in hover.

That's what I currently resort to:

image

If you want you can find them in Problems view. Is not that enough and helpful?

I need to open panel, find the matching message (hard if I have many errors) and read the error rule id. It's not my workflow.

Plus, this makes it much harder for people to write inline pragmas such as /* eslint-disable [ruleId] */.

@octref

This comment has been minimized.

Copy link
Member

octref commented Nov 21, 2018

This is what I meant:

image

  • Code/Source are displayed but do not command too much attention
  • code, often the ruleId, should be linkable. That's #11847 but not #54272.
@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Nov 23, 2018

This is how it looks when I align it to right. Not sure right alignment looks good for all. May be it is better to go with left alignment?

image

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Nov 23, 2018

I would decrease the font and make it more opaque, @misolori might have more ideas
I am still not convinced that we need to show them. Why don't we discuss this at the next UX meeting?

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Nov 23, 2018

Another idea from (@glen-84) is to have everything in single line and left aligned. Some realtime examples:

screenshot 2018-11-23 at 15 35 36

image

image

Edit: Credits to @glen-84 :)

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Nov 23, 2018

I would prefer either show or not show them but not configurable (through setting or action) as it is not worth.

I personally think above approach is the best out of all where metadata is not prominent and rendered nicely.

Sure. Let's discuss in the next UX meeting.

sandy081 added a commit that referenced this issue Nov 23, 2018

@glen-84

This comment has been minimized.

Copy link

glen-84 commented Nov 23, 2018

Another idea is to have everything in single line

I feel so ignored. 😄

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Nov 28, 2018

Another suggestion from @egamma is to inline the source and code with the message without labels

image

image

image

@sandy081 sandy081 closed this in c8d9db0 Nov 28, 2018

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Nov 28, 2018

Closing this with above inline approach

@octref

This comment has been minimized.

Copy link
Member

octref commented Nov 28, 2018

There's no consistency.
Problems View:

  • Square bracket for source
  • Paren for range

Inline View:

  • Source has no bracket
  • Paren for code

I also don't like the code/source appended to the last line's message. If not on a new line, I hope they can be right-aligned.

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Nov 29, 2018

@octref I do not like to make it right aligned because it looks ugly as I showed it already.

Regarding consistency, I think its not a big issue. (like status bar and problems view are inconsistent). If you strongly feel so, I would change the line and column rendering similar to in status bar.

@usernamehw

This comment has been minimized.

Copy link
Contributor

usernamehw commented Nov 29, 2018

How about always using top right corner?

Sick paint skills:
49151188-866ab800-f30f-11e8-8b03-910450ca69da

49151203-92567a00-f30f-11e8-9aa0-8341e992b3eb

@sandy081

This comment has been minimized.

Copy link
Member

sandy081 commented Nov 29, 2018

There have been too many iterations done on this and thanks to all for the suggestions. Since it is highly individual opinionated, I would collect more feedback on the current approach before changing and going in circles.

@octref

This comment has been minimized.

Copy link
Member

octref commented Nov 29, 2018

Yes, the comment 6 days ago #62370 (comment) got most upvotes and everyone seems to like it. That looked great:

image

but suddenly you changed it to the current form.

image

I don't know what's the reason we can't have one extra line for the source/code. It's easy for user to locate and read, and it's always aligned. Vertical space is not limited for diagnostics hovers, are they?

And the new design generates inconsistencies. User now will be confused for "what's inside a parenthesis" and "what's inside a bracket". This goes against our effort to "make errors/hovers consistent". /cc @joaomoreno @misolori for input.

@misolori

This comment has been minimized.

Copy link
Member

misolori commented Nov 29, 2018

In the UX call yesterday we discussed both of those options and even though we all liked both versions, we all agreed that we liked the cleaner look of last one (the one that's now on insiders).

@mattacosta

This comment has been minimized.

Copy link
Contributor

mattacosta commented Nov 29, 2018

@octref To be fair, the option to not show any additional information is actually the one with the most likes and it is what Visual Studio does too.

#62370 (comment)

vs_tooltip

@roblourens #62370 (comment)

I think it should be shown because if I have multiple extensions contributing diagnostics, I want to know where a message is coming from. The error code is also useful, e.g. if you want to know which rule in a tslint.json is associated with an error message. I prefer option 1.

Note that the hovers in vscode are clickable. If said information is not shown, what about clicking the hover and having it shift focus to the full diagnostic in the problems view? Personally, I think it would make sense as most people eventually get an idea of where a diagnostic is coming from an no longer need that data.

@Xanewok

This comment has been minimized.

Copy link
Contributor

Xanewok commented Dec 31, 2018

Fixes for his have caused regression for https://github.com/rust-lang/rls-vscode. We used backtick-enclosed strings for types and since everything is escaped now to render a custom hover window, the error messages are illegible now, for example:

current output

note: expected type std::option::Option<std::string::String> found type std::option::Option<&std::string::String>

previous output

note: expected type std::option::Option<std::string::String> found type std::option::Option<&std::string::String>

See rust-lang/rls-vscode#479 for more details.

@nwolverson

This comment has been minimized.

Copy link

nwolverson commented Jan 2, 2019

Suffering the same issue as @Xanewok in nwolverson/vscode-ide-purescript#115 - diagnostics are rendering fine in problems and tooltip but replaced by displayed HTML entities in the hover

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment