Skip to content

Better tooltips. #1504

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

Merged
merged 2 commits into from
Oct 7, 2022
Merged

Better tooltips. #1504

merged 2 commits into from
Oct 7, 2022

Conversation

dankeboy36
Copy link
Contributor

Better tooltips.

tooltipfix.mp4

Motivation

Change description

Other information

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

fixes arduino#1503

Signed-off-by: dankeboy36 <[email protected]>
@CLAassistant
Copy link

CLAassistant commented Sep 27, 2022

CLA assistant check
All committers have signed the CLA.

@kittaakos kittaakos requested a review from AlbyIanna September 28, 2022 06:01
@Edivad99
Copy link

Nice, but I would fix having 2 identical pieces of information when holding Command. I would combine the last and penultimate lines since they report the same information

@kittaakos
Copy link
Contributor

@Edivad99, if you are referring to this:

  • The fourth rectangle is a duplication of the information I have in the third rectangle

Then you misunderstood the feature. In the monaco editor, if you press Ctrl/⌘ while hovering over a symbol in the editor (see textDocument/docuemtnSymbol), monaco will show the actual code as a preview.

From the VS Code docs:

If you press Ctrl and hover over a symbol, a preview of the declaration will appear:

I do not think we will spend time removing an available feature, but I am positive there is a preference to disable it if required.

Here is an .ino file you can copy paste and try out locally:

void setup() {
  sum(1, 2);
}
void loop() {}

/**
 * This is an example doc.
 *
 * @param left your number
 * @param right another number
 * @returns the sum of the args
 */
int sum(int left, int right) {
  int i = 0;
  // some comment
  int j = i;
  return left + right;
}

In action:

peek.mp4

Here is another example. It's coming from Arduino.h:

peek_definition_other_example.mp4

For completeness, people are complaining about too slow and fast popups. It's possible to adjust it via the advanced settings.

To disable the popup, add the following to the settings.json file:

{
  "editor.hover.enabled": false
}

If you want to tune the popup delay, add this to the settings file:

{
  "editor.hover.delay": 300
}

The default value is 300, the minimum allowed value is 0, and the maximum is 10000 in milliseconds.

In action:

hover_settings.mp4

@Edivad99
Copy link

Edivad99 commented Sep 28, 2022

I don't want to delete the feature, but it can be set that when I press command the penultimate rectangle disappears and only the last one remains visible so you don't have 2 lines showing the method signature

@kittaakos
Copy link
Contributor

but it can be set that when I press command the penultimate rectangle disappears and only the last one remains visible so you don't have 2 lines showing the method signature

If you want to tune how your editor works locally, you might find the corresponding settings here to achieve it. Let us know how it went. Or maybe you can even open a PR with your proposed default settings.

@per1234
Copy link
Contributor

per1234 commented Sep 28, 2022

If you want to tune the popup delay, add this to the settings file:

To save any interested parties from confusion, I'll add that this setting is not working correctly: #571

@dankeboy36
Copy link
Contributor Author

CLA assistant check
All committers have signed the CLA.

Hi, there is a mistake in the CLA:

Agreement by signin in with GitHub.

It should be:

Agreement by signing in with GitHub.

@per1234
Copy link
Contributor

per1234 commented Sep 30, 2022

Thanks so much for your pull request and also for pointing out this error in the CLA text @dankeboy36!

@alranel can you make the suggested correction to the CLA text?

@alranel
Copy link
Contributor

alranel commented Sep 30, 2022

I'm unable to do it - the CLA is owned by the GitHub ArduinoBot user. Who has access to it? This is the gist to edit:
https://gist.github.com/ArduinoBot/69c5b68e989a47174b83b2bad2f4ecbf

@kittaakos kittaakos added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project topic: theia Related to the Theia IDE framework labels Oct 4, 2022
@AlbyIanna AlbyIanna requested review from kittaakos and per1234 and removed request for AlbyIanna October 4, 2022 13:37
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

Works for me but I do not know why we need this additional styling. Maybe it's a Theia problem. If others are also OK, we can merge the PR.

@AlbyIanna, could you please also take a look? Thank you!

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

This fixes the problem of the incorrect spacing of the horizontal rules in the editor hover:

Using this test code (written as pure C++ so it can be compared to the rendering in VS Code):

// Doc comment for foo with `inline code` markup
int foo(int bar);
int main() {
  foo(42);
  return 0;
}
int foo(int bar) {}

This is how the plain hover looks using the latest build from the main branch:

image

The return type and signature lines are unpleasantly close to the horizontal rule.

This is how the plain hover looks using the build from this PR:

image

The spacing after the horizontal rules is nice looking.


I did notice one small regression:

This is how the Ctrl+hover looks using the latest build from the main branch:

image

In the case of the definition section that is added at the bottom of the hover, the spacing after the horizontal rule is already correct.

This is how the Ctrl+hover looks using the build from this PR:

image

Now the spacing after the horizontal rule in the definition section is a little larger than expected.


I think the significant improvement in formatting outweighs the minor regression, especially since Ctrl+hover is probably rarely used by the Arduino IDE users (I didn't know about it before now), so I think the PR is beneficial overall, but if it is easy to fix that regression then I think it would be good to do.

@per1234
Copy link
Contributor

per1234 commented Oct 6, 2022

Maybe it's a Theia problem.

Theia Blueprint 1.30.0 + clangd extension has the same formatting problems as the Arduino IDE hover:

image

VS Code + clangd extension has the problem with the signature line, but not with the return type line:

image

@per1234 per1234 linked an issue Oct 6, 2022 that may be closed by this pull request
3 tasks
@dankeboy36
Copy link
Contributor Author

OK

Screen Shot 2022-10-06 at 20 53 54

Copy link
Contributor

@AlbyIanna AlbyIanna left a comment

Choose a reason for hiding this comment

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

I tested it and it works for me. Thank you @dankeboy36 !

If @per1234 is good with the changes too, I would merge this.

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

It looks great. Thanks @dankeboy36!

@per1234 per1234 merged commit bc264d1 into arduino:main Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself topic: theia Related to the Theia IDE framework type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tooltip too squashed
7 participants