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.

Sign up for GitHub

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

Jump to bottom

Show right doc on hover over constructor #7654

Merged
merged 1 commit into from
Aug 30, 2024
Merged

Show right doc on hover over constructor #7654

merged 1 commit into from
Aug 30, 2024

Conversation

subhash-arabhi
Copy link
Contributor

@subhash-arabhi subhash-arabhi commented Aug 9, 2024 •
edited
Loading

Fixes the issue #7653
Changes:

  1. Added the FindNewClassForConstructor function to skip to the parent if the parent is a new class type.
  2. Added numbers and characters to the list where the offset needs to be incremented.
  3. Added tests for these changes.

@mbien mbien added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) JavaDoc [ci] enable java/javadoc tests and build-javadoc target ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Aug 9, 2024
@mbien mbien added this to the NB24 milestone Aug 9, 2024
@mbien mbien linked an issue Aug 9, 2024 that may be closed by this pull request
Hovering over constructor and it's integer arguments showing wrong signature #7653
Closed
@mbien mbien added the LSP [ci] enable Language Server Protocol tests label Aug 9, 2024
@apache apache locked and limited conversation to collaborators Aug 9, 2024
@apache apache unlocked this conversation Aug 9, 2024
@mbien mbien added the VSCode Extension [ci] enable VSCode Extension tests label Aug 9, 2024
@lahodaj
Copy link
Contributor

lahodaj commented Aug 12, 2024

I would suggest to add testcases for the number and character literals also to JavaDocumentationTaskTest.java, as that will be the test other developers will likely run when adjusting code completion. Tests in java/java.lsp.server are mostly meant for the gluing code between the LSP and the real backend, and do not generally cover all possible problems/combinations.

dbalek
dbalek approved these changes Aug 12, 2024
View reviewed changes
Copy link
Contributor

@dbalek dbalek left a comment

Choose a reason for hiding this comment

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

Looks fine.

lahodaj
lahodaj approved these changes Aug 12, 2024
View reviewed changes
Copy link
Contributor

@lahodaj lahodaj left a comment

Choose a reason for hiding this comment

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

Looks good.

@Achal1607 Achal1607 mentioned this pull request Aug 13, 2024
Closed
@subhash-arabhi subhash-arabhi mentioned this pull request Aug 14, 2024
Merged
@mbien
Copy link
Member

mbien commented Aug 28, 2024

would need squashing before merge

@mbien mbien changed the title Constructor hover bug Show right doc on hover over constructor Aug 28, 2024
@subhash-arabhi
Fix for constructor hover signature issue
c08d1dd 
Added tests for hover bug code changes

Signed-off-by: Subhash Arabhi <subhashsubbu636@gmail.com>

Added test for class hover

Signed-off-by: Subhash Arabhi <subhashsubbu636@gmail.com>

Added integer and character tests in JavaDocumentationTests

Signed-off-by: Subhash Arabhi <subhashsubbu636@gmail.com>
@subhash-arabhi
Copy link
Contributor Author

@mbien I squashed the commits, Could you merge the PR

@mbien
Copy link
Member

mbien commented Aug 29, 2024

@subhash-arabhi I unlocked CI here. But merging should be done by someone else (assuming tests are green). This is VSCode specific and doesn't influence NetBeans behavior if I understand it correctly due to UX differences.

NetBeans shows the overlay information of the parameter if the mouse is over the parameter, not the constructor doc. For the constructor doc, the user has to hover over the constructor identifier. For the class doc you have to move the mouse over the type - all very intuitive. (#7653 (comment))

The bugfix to me would be to show the type info of the param, not the constructor doc IMO.

But I am not going to veto this since a quick manual test showed that this patch does not seem to influence NB. cc @lahodaj

@lahodaj
Copy link
Contributor

lahodaj commented Aug 29, 2024

Unless there are objections, I'll merge tomorrow.

@lahodaj lahodaj merged commit 5c4660a into apache:master Aug 30, 2024
37 checks passed
Achal1607 pushed a commit to Achal1607/netbeans that referenced this pull request Feb 21, 2025
@subhash-arabhi @sid-srini
Patch for PR apache#7654 (apache#238)
8693aa0 
Signed-off-by: Subhash Arabhi <subhashsubbu636@gmail.com>
Co-authored-by: Siddharth Srinivasan <siddharth.srinivasan@oracle.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbalek dbalek dbalek approved these changes

@lahodaj lahodaj lahodaj approved these changes

Assignees
No one assigned
Labels
ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) JavaDoc [ci] enable java/javadoc tests and build-javadoc target LSP [ci] enable Language Server Protocol tests VSCode Extension [ci] enable VSCode Extension tests
Projects
None yet
Milestone
NB24
Development

Successfully merging this pull request may close these issues.

Hovering over constructor and it's integer arguments showing wrong signature
4 participants
@subhash-arabhi @lahodaj @mbien @dbalek