search

eclipse-platform / eclipse.platform.text

8 stars 45 forks source link

Fix spurious content-assist popup on MacOS #150

Closed ahmedneilhussain closed 1 year ago

ahmedneilhussain commented 1 year ago

I think this was introduced by https://github.com/eclipse-platform/eclipse.platform.text/commit/78fc9f6f6566323c691f1fee1eb14de2f27e7d34

Have observed with LSP4e-based editors in Eclipse 2022-12, but presumably would affect everything that uses the jface text editor and has a content assistant installed.

To reproduce (on MacOS):

  1. E.g. open a Rust file (with Eclipse Corrosion plugin installed)
  2. Position the cursor on some whitespace on a non comment line
  3. Having made sure the file is already saved, press cmd-S to 'save' again.

The content assistant popup will be shown. This also seems to occur if you press cmd-? where ? is any letter not bound to a command, or bound to a command that doesn't succeed and mark the event as consumed. E.g. cmd-C with no text highlighted will also provoke the popup.

I guess this isn't an issue on Windows/Linux because ctrl is used as the primary modifier key, and that gets encoded as part of the character code point and gets filtered out by Character.isLetter(c) || Character.isDigit(c) on L1242.

iloveeclipse commented 1 year ago

@gayanper, @vogella: could you please review?

mickaelistria commented 1 year ago

@lshanmug @kthoms IIRC you're using a mac. Can you please verify this one? Or maybe suggest some other committer using a mac and who could test it?

lshanmug commented 1 year ago

@elsazac Can you please verify this and update here?

elsazac commented 1 year ago

I tried and verified this PR in MacOS.The PR fixes the problem that was mentioned. Build information is listed below:

Eclipse SDK Version: 2023-03 (4.27) Build id: I20230222-1800 OS: Mac OS X, v.13.2.1, aarch64 / cocoa Java version: 19.0.1

laeubi commented 1 year ago

Interestingly there is also a test regarding the popup that is currently disabled on macos:

ContextInformationTest#testContextInfo_hide_focusOut

would be good to probably check if this can fix the test as well?

vogella commented 1 year ago

@elsazac sorry but help me, who are you? New team member in the IBM team or why is @lshanmug asking you to test?

ahmedneilhussain commented 1 year ago

@vogella was just getting chased by my boss about this. Is it likely to get merged in production Eclipse in the near future?

laeubi commented 1 year ago

This repository was merged with https://github.com/eclipse-platform/eclipse.platform.ui if your pull-request is still relevant please rebase the code and push it to the new repository.

elsazac commented 8 months ago

@elsazac sorry but help me, who are you? New team member in the IBM team or why is @lshanmug asking you to test?

@vogella Apologies I missed this question. Yes I work in IBM with @lshanmug