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.

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

Implement "Try new editing experience" to promote CDT LSP #421

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

jonahgraham
Copy link
Member

@jonahgraham jonahgraham commented Feb 17, 2025

Contribute a banner to the CEditor and CLspEditor to promote the new C/C++ editing experience based on LSP. The banner in CEditor enables a quick switch to the LSP editor:

image

And in the CLspEditor it enables a quick switch back to the traditional CEditor.

image

If there is no project specific setting for prefer_lsp a quick confirmation dialog is displayed with options for help and feedback:

image

and the other way:

image

If the editor is open on a specific project and that project has project specific preferences for prefer_lsp, instead of automatically changing the setting, the property page is opened:

image

A notification is also displayed to the user for a short while after the editors are re-opening offering more information and access to the preferences.

image

A notification is also displayed when closing the banner allowing the user easy access to disable opening the banner on new editors

image

Finally a new preference is available:

image

Fixes eclipse-cdt/cdt#968

@jonahgraham
Copy link
Member Author

@ghentschke you can have an early look at this - I really want to get this into M3, which means latest Wednesday. This is still in draft mode and requires the corresponding CDT change eclipse-cdt/cdt#1088 so that we can hook into CEditor.

@ghentschke
Copy link
Contributor

Yes, I'll take look at it.

@ghentschke
Copy link
Contributor

Add preference to disable the banners in the editors and connect the close banner to it.

I think this is important to give vendors the possibility to disable the banners.

@jonahgraham
Copy link
Member Author

@ghentschke / other interested parties

Please have a look - I updated the original description with updated and additional screenshots. eclipse-cdt/cdt#968 also has some of the older dark mode screenshots.

* SPDX-License-Identifier: EPL-2.0
*******************************************************************************/

package org.eclipse.cdt.lsp.internal.switchtolsp;
Copy link
Contributor

Choose a reason for hiding this comment

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

I still cannot run/compile this change, because of this missing dependency which comes only with the CDT PR 1088
image

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 know how we could avoid this break.

Copy link
Contributor

@ghentschke ghentschke Feb 19, 2025

Choose a reason for hiding this comment

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

I also retested by building this patch with mvn verify and installing it into 2024-12 C/C++. The banner appears on CDT LSP editor, and not on CEditor.

Okay, the compiled jar file is executable in conjunction with CDT version < 12?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Cdt 12 is required for build, older cdt works at runtime.

If it is a hard requirement to build with cdt 11 we can ditch osgi for just that interface, and use a Function instead as the interface between CEditor and lsp

Lmk

Copy link
Contributor

@ghentschke ghentschke Feb 19, 2025

Choose a reason for hiding this comment

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

No, its just important for us at run time because we cannot update our IDE at the moment and I still want to use the cdt-lsp latest builds.

Copy link
Contributor

@ghentschke ghentschke left a comment

Choose a reason for hiding this comment

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

LGTM

@ghentschke
Copy link
Contributor

ghentschke commented Feb 19, 2025

On windows 11 I see a frame around the banner:
image

image

Is this intended?

@jonahgraham
Copy link
Member Author

Not intended. I will spin it up on Windows to see what to do about it.

@jonahgraham
Copy link
Member Author

Is this intended?

Now fixed.

Sadly SWT's emoji handling on Windows makes the sparkles less sparkly. Its not something I can fix. LMK if the sparkle emoji use should be removed on Windows, or even on all platforms.

@jonahgraham
Copy link
Member Author

I tested on macOS and it all looks good there too.

@ghentschke
Copy link
Contributor

I think the less sparkling banner is better than nothing. @jonahgraham thank you!

Copy link
Member Author

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

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

I have squashed and rebased on main. Once the build reports back this will be merged and I will contribute a build to SimRel later today for 2025-03 M3

*
* @since 3.0
*/
List<PreferenceMetadata<?>> defaults = List.of(//
Copy link
Member Author

Choose a reason for hiding this comment

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

@ruspl-afed to take advantage of #425 I added the defaults here too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wrong link - should have been #431

Copy link
Member

Choose a reason for hiding this comment

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

oops. I just merged #432

Copy link
Member

Choose a reason for hiding this comment

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

sorry, I wasn't aware that you are planning change exactly in the same area @jonahgraham

@ghentschke
Copy link
Contributor

I think it would be nice to have #425 in that build as well. What about #410 @ruspl-afed ?

@jonahgraham
Copy link
Member Author

I think it would be nice to have #425 in that build as well. What about #410 @ruspl-afed ?

+1.

Also now that #432 is merged, I'll rebase this change.

Contribute a banner to the CEditor and CLspEditor to promote
the new C/C++ editing experience based on LSP. The banner in
CEditor enables a quick switch to the LSP editor:

![image](https://github.com/user-attachments/assets/67a3c8d7-7ea5-4fa4-8384-debebc893233)

And in the CLspEditor it enables a quick switch back to the
traditional CEditor.

![image](https://github.com/user-attachments/assets/e508a726-f892-47da-92bd-a195bedc0dda)

If there is no project specific setting for `prefer_lsp` a
quick confirmation dialog is displayed with options for help
and feedback:

![image](https://github.com/user-attachments/assets/7c33d4fe-69a8-4cf8-b1fb-7ca80e07020c)

and the other way:

![image](https://github.com/user-attachments/assets/a69a87d8-4f32-4e58-a651-2583db50183e)

If the editor is open on a specific project and that project
has project specific preferences for `prefer_lsp`, instead of
automatically changing the setting, the property page is opened:

![image](https://github.com/user-attachments/assets/16c2ce3b-5b62-4199-8392-5d6800f621c7)

A notification is also displayed to the user for a short while after the editors are
re-opening offering more information and access to the preferences.

![image](https://github.com/user-attachments/assets/e8d069de-0939-4503-8755-a130595570d0)

A notification is also displayed when closing the banner allowing
the user easy access to disable opening the banner on new editors

![image](https://github.com/user-attachments/assets/9f9a00ef-8957-4a8a-8a3d-9c804eee5b5e)

Finally a new preference is available:

![image](https://github.com/user-attachments/assets/74d2d461-d219-465b-9276-e93f6720a9ed)

Fixes eclipse-cdt/cdt#968
@jonahgraham
Copy link
Member Author

I got a green tick - so I am going for the merge.

@jonahgraham jonahgraham merged commit dfe1635 into eclipse-cdt:main Feb 19, 2025
3 checks passed
@jonahgraham jonahgraham deleted the try-lsp branch February 19, 2025 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make CDT LSP editor more visible and easier to onboard in CDT 12
3 participants