Skip to content

Draw completion proposals always as focused #2793

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

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

Conversation

Christopher-Hermann
Copy link
Contributor

Problem description

When opening the completion proposals via the keyboard, the focus will stay in the editor to be able to accept further user input. This is causing the completion proposal to be drawn in non focus colors. This colors can lead to UX problems, especially in dark theme. With this fix, the completion proposals are always drawn in focused colors.

Before the fix in dark theme, it was hard to sport the selected proposal:
image

With the fix, the proposal is colored in the focus color:
image

How to retest

  1. Open a text editor and run the code completion (ctrl+space)
  2. Check that the selected completion proposal is colored in the focused color

Fixes #1688

@Christopher-Hermann Christopher-Hermann added the enhancement New feature or request label Feb 12, 2025
@Christopher-Hermann Christopher-Hermann self-assigned this Feb 12, 2025
@Christopher-Hermann
Copy link
Contributor Author

@iloveeclipse I had fixed this issue in the past, but we had to revert the changes since you found some issues on Linux: #1690
Could you please check if this more local fix is working on Linux?

On Mac and Windows it looks good.

Copy link
Contributor

github-actions bot commented Feb 12, 2025

Test Results

 1 824 files  ±0   1 824 suites  ±0   1h 33m 17s ⏱️ +8s
 7 918 tests ±0   7 690 ✅ ±0  228 💤 ±0  0 ❌ ±0 
23 841 runs  ±0  23 093 ✅ ±0  748 💤 ±0  0 ❌ ±0 

Results for commit 8bb592a. ± Comparison against base commit 1e6e9a9.

♻️ This comment has been updated with latest results.

@iloveeclipse
Copy link
Member

I will test later, but in general I would assume this is not for this release, M3 is planned for tomorrow and the change affects everyone using content assist.

@iloveeclipse
Copy link
Member

Could you please check if this more local fix is working on Linux?

On Mac and Windows it looks good.

I see errors on every popup close:

!ENTRY org.eclipse.ui 4 0 2025-02-12 16:51:52.599
!MESSAGE Unhandled event loop exception
!STACK 0
java.lang.NullPointerException: Cannot invoke "org.eclipse.swt.widgets.Widget.getDisplay()" because "event.item" is null
	at org.eclipse.jface.contentassist.CompletionProposalDrawSupport.handleEvent(CompletionProposalDrawSupport.java:54)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:91)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:5862)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1652)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1678)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1657)
	at org.eclipse.swt.widgets.Widget.release(Widget.java:1396)
	at org.eclipse.swt.widgets.Control.release(Control.java:4753)
	at org.eclipse.swt.widgets.Composite.releaseChildren(Composite.java:1576)
	at org.eclipse.swt.widgets.Canvas.releaseChildren(Canvas.java:279)
	at org.eclipse.swt.widgets.Decorations.releaseChildren(Decorations.java:503)
	at org.eclipse.swt.widgets.Shell.releaseChildren(Shell.java:3437)
	at org.eclipse.swt.widgets.Widget.release(Widget.java:1403)
	at org.eclipse.swt.widgets.Control.release(Control.java:4753)
	at org.eclipse.swt.widgets.Widget.dispose(Widget.java:576)
	at org.eclipse.swt.widgets.Shell.dispose(Shell.java:3354)
	at org.eclipse.jface.text.contentassist.CompletionProposalPopup.hide(CompletionProposalPopup.java:1094)
	at org.eclipse.jface.text.contentassist.AsyncCompletionProposalPopup.hide(AsyncCompletionProposalPopup.java:359)
	at org.eclipse.jface.text.contentassist.ContentAssistant.hide(ContentAssistant.java:2268)
	at org.eclipse.jface.text.contentassist.ContentAssistant$Closer.mouseDown(ContentAssistant.java:221)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:230)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:91)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:5862)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1652)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:5072)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4524)

@Christopher-Hermann Christopher-Hermann force-pushed the completionColoring branch 2 times, most recently from 773fb5f to 6ffbfb8 Compare February 12, 2025 16:06
@Christopher-Hermann
Copy link
Contributor Author

I see errors on every popup close:

Yes I need to check this, there are also some "SWT Resource was not properly disposed" errors

@iloveeclipse
Copy link
Member

there are also some "SWT Resource was not properly disposed" errors

These are most likely follow ups of original error

@Christopher-Hermann Christopher-Hermann force-pushed the completionColoring branch 3 times, most recently from 51a7e93 to a4fdbaf Compare February 13, 2025 18:45
@Christopher-Hermann
Copy link
Contributor Author

I see errors on every popup close:

Should be fixed.

Is the coloring working on Linux?

@Christopher-Hermann Christopher-Hermann force-pushed the completionColoring branch 3 times, most recently from 233e2c8 to c6a0e42 Compare February 13, 2025 18:56
@Christopher-Hermann
Copy link
Contributor Author

@iloveeclipse can you confirm that the coloring is working on linux like expected?

MacOS and Windows is working. Then I would merge this change.

@iloveeclipse
Copy link
Member

Then I would merge this change.

You can't merge the change, we are in the RC phase.

@iloveeclipse
Copy link
Member

can you confirm that the coloring is working on linux like expected?

I can't observe the original problem on Linux. The proposal table without this change looks exactly as with the change (blue selection on dark background).

image

@Christopher-Hermann
Copy link
Contributor Author

If there are no objections I would merge the PR

@szarnekow
Copy link
Contributor

If there are no objections I would merge the PR

Personally I find the focus color to distracting - it grabs too much of my attention while typing.

@Christopher-Hermann
Copy link
Contributor Author

Personally I find the focus color to distracting - it grabs too much of my attention while typing.

Yes, good point. I will check if we can just change the non focus color to something more visible. The problem is, as far as I understood, that on Linux the code completion is always rendered with focus back ground color. So when changing the color, it will most probably change the behavior on Linux.

When opening the completion proposals via the keyboard, the focus will stay in the editor to be able to accept further user input. This is causing the completion proposal to be drawn in non focus colors. This colors can lead to UX problems, especially in dark theme.
With this fix, the completion proposals are always drawn in focused colors.

Fixes eclipse-platform#1688
@Christopher-Hermann
Copy link
Contributor Author

I tried using SWT.COLOR_TITLE_INACTIVE_BACKGROUND if the control is not focused, but this is even more worse (at least on MacOS).

image

So I see two ways to solve the issue:

  1. Go with the approach as it is suggested in this PR: Just show the code completion as focus in any case
  2. Introduce color definitions for table selections, as it was done in Highlighting problem when using the dark theme on Windows eclipse-platform/eclipse.platform.swt#811 #1690. Later on, when work on Highlighting problem when using the dark theme on Windows eclipse-platform/eclipse.platform.swt#811 #1690 is continued, we can reuse the already existing color definitions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dark theme: code completion selection does not have enough contrast -> align with other popular dark themes
4 participants