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

Context assist on type experiment #111

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Context assist on type experiment #111

wants to merge 1 commit into from
+76 −17

Conversation

gayanper
Copy link
Contributor

@gayanper gayanper commented Jun 20, 2022 •
edited

What it does

The changes enables content assist on type by

  • Disabling the auto insert when content assist trigger on type
  • Move keywords over template proposals
  • Hide empty completion list for the first iteration and enable it when user repeat
  • Expose how the content assist was invoked (Auto or KeyBinding) to CompletionContext so computers can make use of it to decide how they should spend time on calculating proposals.
  • Content assist trigger from key binding work as before without any change.

How to test

Run eclipse with -Dorg.eclipse.jdt.assist.activateOnType=true and typing alpha numerics should trigger the content assist, existing auto activation chars and delay is also reused.

Author checklist

@gayanper
Copy link
Contributor Author

@mickaelistria @noopur2507 Please suggest me few reviewers to get feedback and provide your feedback as well.

Adding for feedback:
@stephan-herrmann
@iloveeclipse
@mpalat

@gayanper
Copy link
Contributor Author

I will search and report a issue for this if one doesn't exist.

@mickaelistria
Copy link
Contributor

I think it's https://bugs.eclipse.org/bugs/show_bug.cgi?id=101420 . I hope to look at this proposal soon.

@gayanper
Copy link
Contributor Author

I think it's https://bugs.eclipse.org/bugs/show_bug.cgi?id=101420 . I hope to look at this proposal soon.

@mickaelistria Found an issue in Windows, even though the it is configured not to show empty list, the empty list is shown in windows, but not in macos.

@gayanper
Copy link
Contributor Author

I think it's https://bugs.eclipse.org/bugs/show_bug.cgi?id=101420 . I hope to look at this proposal soon.

@mickaelistria Found an issue in Windows, even though the it is configured not to show empty list, the empty list is shown in windows, but not in macos.

The problem persists when the computations are executed asynchronously. I think when computation are fast enough the async computation is not triggered.

What I observed if I don't see the progress item in the completion list, things work fine in both OS.

@gayanper
Copy link
Contributor Author

I think it's https://bugs.eclipse.org/bugs/show_bug.cgi?id=101420 . I hope to look at this proposal soon.

If you are running I-Builds of Eclipse, you can install this change and related jface.text mods using this updatesite patch-site - https://gayanper.github.io/org.gap.eclipse.jdt.eap/p2/i-builds

@humphreygao
Copy link

You type something in one line, and find that your input is totally wrong, so you decide to keep pressing backspace. When there are only leading whitespaces in one line, the content assist menu should not pop up, but it pops up every time you press backspace.

20220627_222944.mp4

Was it possible to disable content assist when you press backspace but there are only whitespaces(or nothing) in one line?

@gayanper
Copy link
Contributor Author

I think i can see how to solve this issue. Thanks for testing and catching this scenario. Did you install the changes from the update site i shared which container jface patch as well ?

@gayanper
Copy link
Contributor Author

I think this behavior is by default in assist popup. But i think we can improve it. There is no point in keeping the popup after you erased your completion token.

@humphreygao
Copy link

humphreygao commented Jun 28, 2022 •
edited

Did you install the changes from the update site i shared which container jface patch as well ?

yes

mickaelistria
mickaelistria reviewed Jun 29, 2022
View reviewed changes
Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

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

I like the progress being made, however I believe that it shouldn't be the responsibility of the processor to decide whether to auto activate on typing. This is more a responsibility of the popup as it affects all processors in the same way.

org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/java/ContentAssistProcessor.java Outdated
fAutoActivateOnType= autoActivateOnType;
}

public final boolean isAutoActivateOnType() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see it used anywhere. Is it expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this is just the getter that is added to comply with bean pattern, But i think we can remove these properties since it make sense to have it in the content assist it self rather than the processors to be beneficial in other places.

@gayanper
Copy link
Contributor Author

gayanper commented Jul 3, 2022

I like the progress being made, however I believe that it shouldn't be the responsibility of the processor to decide whether to auto activate on typing. This is more a responsibility of the popup as it affects all processors in the same way.

@mickaelistria check the latest patches, I have moved auto activation into jface contentassist.

@gayanper gayanper force-pushed the complete_on_type branch 2 times, most recently from 29df495 to 1e4b27d Compare August 13, 2022 18:50
@gayanper gayanper marked this pull request as ready for review August 13, 2022 18:51
@gayanper gayanper requested a review from jjohnstn August 13, 2022 18:51
@gayanper
Copy link
Contributor Author

@mickaelistria and @jjohnstn please have a look at the changes, We can try to merge this for next release (4.26) M1.

@gayanper gayanper force-pushed the complete_on_type branch 4 times, most recently from ad06c79 to f9f7789 Compare August 14, 2022 17:36
@gayanper gayanper changed the title WIP context assist on type experiment Context assist on type experiment Aug 17, 2022
@gayanper gayanper force-pushed the complete_on_type branch 2 times, most recently from 761e92d to 529b36f Compare September 15, 2022 17:43
@gayanper
Add support for async completions on type eclipse-jdt#215
f999c9e 
The changes use the new platform.text ContentAssist mode which act of
key type. Some additional attributes are added to
JavaContentAssistInvocationContext to figure out if the request was made
by action or on-type.
@gayanper
Copy link
Contributor Author

@mickaelistria @noopur2507 should we merge this and try this out with M1 ?

@mickaelistria
Copy link
Contributor

@gayanper Sorry, I cannot decide nor act here, I'm not a committer on JDT.

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

@mickaelistria mickaelistria mickaelistria left review comments

@jjohnstn jjohnstn Awaiting requested review from jjohnstn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gayanper @mickaelistria @humphreygao