API to Track TextViewer install/uninstall #107

angelozerr · 2022-10-27T17:48:06Z

API to Track TextViewer install/uninstall

My motivation to provide this API is

to fix Register IReconcilingStrategy with GenericEditor extension point. #85
I need this API to support with a clean mean Provide .editorconfig support eclipse.platform.ui#877 to track the install/uninstall of ITextViewer.

laeubi · 2022-10-28T04:30:30Z

@angelozerr you need to fix the API errors:

2 API ERRORS
[API ERROR] File MANIFEST.MF at line 5: The minor version should be incremented in version 3.21.100, since new APIs have been added since version 3.21.0 - The superinterfaces set has been expanded (org.eclipse.jface.text.ITextViewerLifecycle) for class org.eclipse.jface.text.reconciler.Reconciler
- The superinterfaces set has been expanded (org.eclipse.jface.text.ITextViewerLifecycle) for interface org.eclipse.jface.text.contentassist.IContentAssistant
- The superinterfaces set has been expanded (org.eclipse.jface.text.ITextViewerLifecycle) for interface org.eclipse.jface.text.information.IInformationPresenter
- The superinterfaces set has been expanded (org.eclipse.jface.text.ITextViewerLifecycle) for class org.eclipse.jface.text.contentassist.ContentAssistant
- The superinterfaces set has been expanded (org.eclipse.jface.text.ITextViewerLifecycle) for interface org.eclipse.jface.contentassist.ISubjectControlContentAssistant
- The superinterfaces set has been expanded (org.eclipse.jface.text.ITextViewerLifecycle) for class org.eclipse.jface.text.reconciler.AbstractReconciler
- The superinterfaces set has been expanded (org.eclipse.jface.text.ITextViewerLifecycle) for class org.eclipse.jface.text.information.InformationPresenter
- The superinterfaces set has been expanded (org.eclipse.jface.text.ITextViewerLifecycle) for class org.eclipse.jface.text.codemining.CodeMiningReconciler
- The superinterfaces set has been expanded (org.eclipse.jface.text.ITextViewerLifecycle) for class org.eclipse.jface.text.presentation.PresentationReconciler
- The superinterfaces set has been expanded (org.eclipse.jface.text.ITextViewerLifecycle) for interface org.eclipse.jface.text.presentation.IPresentationReconciler
- The superinterfaces set has been expanded (org.eclipse.jface.text.ITextViewerLifecycle) for interface org.eclipse.jface.text.hyperlink.IHyperlinkPresenter
- The type org.eclipse.jface.text.ITextViewerLifecycle has been added to org.eclipse.jface.text_3.21.100
- The superinterfaces set has been expanded (org.eclipse.jface.text.ITextViewerLifecycle) for interface org.eclipse.jface.text.reconciler.IReconciler
- The superinterfaces set has been expanded (org.eclipse.jface.text.ITextViewerLifecycle) for class org.eclipse.jface.contentassist.SubjectControlContentAssistant
- The superinterfaces set has been expanded (org.eclipse.jface.text.ITextViewerLifecycle) for class org.eclipse.jface.text.reconciler.MonoReconciler (location: /home/jenkins/agent/workspace/eclipse.platform.text_PR-107/org.eclipse.jface.text/META-INF/MANIFEST.MF)
[API ERROR] File ITextViewerLifecycle.java at line 20: Missing @since tag on org.eclipse.jface.text.ITextViewerLifecycle (location: /home/jenkins/agent/workspace/eclipse.platform.text_PR-107/org.eclipse.jface.text/src/org/eclipse/jface/text/ITextViewerLifecycle.java)

angelozerr · 2022-10-28T07:01:58Z

Thanks @laeubi for your feedback.

It should be fixed now.

laeubi

This looks good, but wait for @mickaelistria as he is our API-Guardian here :-)

angelozerr · 2022-10-28T07:05:36Z

I update the since to 3.21.0 instead of 3.21, is it correct?

This looks good, but wait for @mickaelistria as he is our API-Guardian here :-)

Yes sure! As .editorconfig support is a big PR, I try to create little PR to speed the merge.

angelozerr · 2022-10-28T07:53:23Z

@laeubi the build fails again, I'm a little lost, could you help me to fix it please. Thanks!

laeubi · 2022-10-28T08:54:42Z

@angelozerr in general I search the (full) Console log for "[API ERROR] "

https://ci.eclipse.org/platform/job/eclipse.platform.text/job/PR-107/3/consoleFull

If you use the Eclipse SDK you should also see API error in the IDE, if not @merks might can help / advice why those not sho up.

org.eclipse.jface.text/src/org/eclipse/jface/text/ITextViewerLifecycle.java

angelozerr · 2022-10-28T14:54:23Z

@szarnekow I updated my PR to use ITextViewerLifecycle with generic editor.

I created a PR on LSP4e side which consumes this API eclipse-lsp4e/lsp4e#272

mickaelistria · 2022-11-04T16:37:00Z

I actually likes the first version of the patch better, particularly the part that factors the ITextLifecycle in existing reconcilers or others as it already emphasizes a lot of benefit in this factorization.
But I agree with @szarnekow that this new API should be made most obviously useful in general if we want it part of the common text API. Isn't there some other API that could optionally implement the provided interface, for example IQuickAssistProcessor or something like that? If we can show in the existing layer that this API can provide value to existing code, then it can make it a good candidate to get directly into jface.text. I suspect that about every possible strategy in SourceViewer.configure() can benefit from this API.

angelozerr · 2022-11-04T18:09:00Z

@mickaelistria if I understand correctly your comment you would prefer that ITextViewerLifecycle belongs to jface text?

So it means that SourceViewer should store a list of ITextViewerLifecycle and we should provide a new Api like ITextViewerExtendionX which provide à method like addTextVieweLifecycle.

The existing reconcilier presentationReconciler will be added to the list to call install / uninstall methods instead of hard coding that. Is this idea that you would like to follow ?

mickaelistria · 2022-11-04T20:38:26Z

Just to be clear, what is the purpose of this API? Is it to allow more of existing contributions that support and install/uninstall pattern just like Reconcilier or ContentAssistProcessor already do, or to provide something new? If it's the former, I don't think you need more logic than adding if (x instanceof ITextViewerLifecycle lifecycle) lifecycle.install(this) here and there.

angelozerr · 2022-11-05T05:58:47Z

Is it to allow more of existing contributions that support and install/uninstall pattern just like Reconcilier or ContentAssistProcessor already do, or to provide something new?

The both, but for LSP4e usecase, I would say "provide something new"

angelozerr · 2022-11-05T09:48:36Z

Just to be clear, what is the purpose of this API?

My main goal is to track install / uninstall of ITextViewer for any contribution of genereic editor. I mean for IReconciler (which have already install / uninstall), but too for IAutoEditingStrategy (which have not install / uninstall methods).

When you write a custom TextEditor (without generic editor), you need sometimes ITextEditor or track install / unisntall of ITextViewer. It is easy to do that since you can override SourceViewer or another component.

With generic editor you cannot do that, it is the main goal of ITextViewerLifecycle and the another API ITextEditorAware #108

Now the question is where host those 2 APIs?

in generic editor, it means that generic editor requires to export those API (today generic editor uses existing API like IReconciler)
in an another plugin like JFace Text, I tried to use it with existing component but according this comment it seems that it is a bad idea. For ITextViewerLifecycle the main idea is to store all contribution which implements ITextViewerLifecycle where contribution is created, and when source view install / unistall the TextViewer, we need to loop for ITextViewerLifecycle. I tried to do that for existing component in

eclipse.platform.text/org.eclipse.jface.text/src/org/eclipse/jface/text/source/SourceViewer.java

Line 492 in d04589f

fPresentationReconciler= configuration.getPresentationReconciler(this);

but my fear is that I broke something, because there is some order of install and some updat eof boolean flag. I think it can be dangerous to do that.

In other words I think more and more that ITextViewerLifecycle and ITextEditorAware should be hosted in genereic editor plugin.

I'm waiting for your feedback to clean the both APIs.

mickaelistria · 2022-11-09T13:45:19Z

t according #107 (comment) it seems that it is a bad idea.

I don't think the comment says the idea is a bad one, but more that some documentation/usage of the new API is missing for it to feel complete. It's the same thing I tried to mention in my comment where I suggest we check for instanceof ITextViewerLifecycle on more objects of the SourceViewer.
The classes that can support this ITextViewerLifecycle to trigger the install/uninstall methods should receive a not in their documentation a bit like *Extension interfaces are documented.

In other words I think more and more that ITextViewerLifecycle and ITextEditorAware should be hosted in genereic editor plugin.

The generic editor isn't meant to provide new Text APIs, JFace Text is the right place for APIs; Generic Editor just allows to contribute some extra behavior to the generic editor. The description of the use-case can make sense in other editors, use-cases than the generic editor. Usually the workaround is to pass directly a reference to the viewer when creating such a contributor, but it's actually not best for reusability, the suggested interface would also allow to improve existing cases that are independant from the generic editor.

angelozerr · 2022-11-09T21:17:16Z

@mickaelistria is #107 (comment) will follow your idea?

mickaelistria · 2022-11-10T10:06:15Z

@mickaelistria is #107 (comment) will follow your idea?

I don't think we need a new API method in SourceViewer for the moment, but just checking for instanceof ITextViewerLifecycle on the various objects that are used by this viewer (usually returned by the SourceViewerConfiguration). Already having it would validate the ITextViewerLifecycle addition.
Then, later in another change, we can consider adding a API method to add plain ITextViewerLifecycle that wouldn't be just decorations to existing elements, but I see it as a 2nd step when the ITextViewerLifecycle interface is already in place.

szarnekow · 2022-11-10T14:41:45Z

org.eclipse.jface.text/src/org/eclipse/jface/text/ITextViewerLifecycle.java

+public interface ITextViewerLifecycle {
+
+	/**
+	 * Installs a text viewer.


Please clarify whether implementors should expect more than one install(..) call with / without uninstall invocations.

I explain when install / uninstall is called. Hope it will be enough. If no please give me some suggestion.

szarnekow

Yes, I like it. This is the kind of comment and handling of the new API that I expected to happen.

org.eclipse.jface.text/src/org/eclipse/jface/text/reconciler/MonoReconciler.java

org.eclipse.jface.text/src/org/eclipse/jface/text/source/SourceViewer.java

mickaelistria · 2022-11-10T15:47:02Z

@angelozerr I like it a lot as well in this current form. Please address @szarnekow 's comment and then we'll try to get this API addition merged ASAP (we're starting the RC phase, so we'll need some lobbying first but as it's backward compatible and the case is very well built with clear needs and various proposals were discussed to reach a consensus, I'm optimistic the development process and result are correct enough for a merge in RC).

angelozerr · 2022-11-10T17:23:44Z

It tells you to make https://github.com/eclipse-platform/eclipse.platform.text/blob/master/org.eclipse.jface.text/META-INF/MANIFEST.MF#L5 3.22.0.qualifier

I try, hope CI build will work.

angelozerr · 2022-11-10T17:26:12Z

@angelozerr I like it a lot as well in this current form.

Cool!

Please address @szarnekow 's comment and then we'll try to get this API addition merged ASAP

I tried to fix all comments.

mickaelistria · 2022-11-10T20:03:01Z

I don't get the API Tools issue here, it seems pretty wrong. Locally, I don't see this issue, either in the IDE nor in a Maven build with the right profiles triggered; the message gives me the impression an older version is used as baseline.

laeubi · 2022-11-11T05:14:32Z

I don't get the API Tools issue here, it seems pretty wrong. Locally, I don't see this issue, either in the IDE nor in a Maven build with the right profiles triggered; the message gives me the impression an older version is used as baseline.

@vik-chand can you help here?

Error is

[API ERROR] File LinkedModeUI.java at line 295: Invalid @SInCE 3.21 tag on doExit(LinkedModeModel, DocumentEvent); expecting @SInCE 3.22 (location: /home/jenkins/agent/workspace/eclipse.platform.text_PR-107/org.eclipse.jface.text/src/org/eclipse/jface/text/link/LinkedModeUI.java)

@mickaelistria can you explain why you think this is bogus?

laeubi · 2022-11-11T05:19:01Z

Just from the code, maybe when an interface extends the new interface, instead of @Override them maybe remove them from the interface?

vik-chand · 2022-11-11T17:01:44Z

vik-chand can you help here?

I fetched this pr on latest eclipse and I get
Description Resource Path Location Type
Invalid @SInCE 3.21 tag on doExit(LinkedModeModel, DocumentEvent); expecting @SInCE 3.22 LinkedModeUI.java /org.eclipse.jface.text/src/org/eclipse/jface/text/link line 295 @SInCE tag problem

This is because the manifest changed the version and few weeks ago, there was an API added with since 3.21. So that should be changed to since 3.22 ( assuming we want this version)

So I skipped this error locally and I couldn't get further API tools error.

The expanded interface warning comes when an API interface has another interface added somewhere in its hierarchy and now available to its clients. I remember also making a change where in if the added interface breaks the client, then it will require a major version change.

If a change should report a error but doesn't in IDE, I think we need to fix it in IDE.
If a change reports an error in IDE but shouldn't, we need to fix it.
If something is not an error but this PR job reports it, then I guess @mickaelistria may know whats going on. It is possible that some baseline/profile/config is wrong but I'm not much of an expert there. If not present in IDE, we can check in I build page ( which uses ant script) - https://download.eclipse.org/eclipse/downloads/drops4/I20221109-1850/apitools/analysis/html/index.html

mickaelistria · 2022-11-13T20:24:00Z

@vik-chand thanks. I was looking at the commit date and not the merge date and didn't realize that the addition of doExit was after the release; what surprises me is that it was not noticed later, but now I believe it can be because an older version of parent was used by CI build and the "Rebase" part in "Rebase and merge" can have hidden the issue.
Anyway, @angelozerr , I think it should be @since 3.22. If you can add a commit that also fixes addition of doExit to @since 3.22, that would be welcome.

angelozerr · 2022-11-14T08:20:36Z

@mickaelistria I'm little lost, I have @since 3.22 in ItextViewerLifecycle. What I need to do ?

mickaelistria · 2022-12-01T13:43:11Z

@angelozerr Can you please rebase on top of master and adapt the MANIFEST and @since tag to new 3.23 version? As soon as build is green, we can merge.

Signed-off-by: azerr <[email protected]>

mickaelistria · 2022-12-02T08:46:56Z

Thank you!

mickaelistria · 2022-12-02T08:52:05Z

Please remember about adding a note to the N&N doc some time later (no need to rush)

cdietrich · 2022-12-06T17:16:12Z

org.eclipse.jface.text/src/org/eclipse/jface/text/source/SourceViewer.java

@@ -422,7 +428,7 @@ public SourceViewer(Composite parent, IVerticalRuler verticalRuler, IOverviewRul
 		fIsVerticalRulerVisible= (verticalRuler != null);
 		fOverviewRuler= overviewRuler;
 		fIsOverviewRulerVisible= (showAnnotationsOverview && overviewRuler != null);
-
+		this.lifecycles= new HashSet<>();


this seems to cause a memory leak in our Xtext Content assist test:
as the reconciler will now be uninstalled after the content assistant.
and thus additional JobInfos will be created for reconciler job

see eclipse/xtext-eclipse#1959

cdietrich · 2022-12-08T05:49:24Z

org.eclipse.jface.text/src/org/eclipse/jface/text/source/SourceViewer.java

-			fReconciler= null;
-		}
+		uninstallTextViewer();
+		fPresentationReconciler= null;


@angelozerr @szarnekow my this nulling cause the problem we see in test?

I created a new PR #123 to fix this issue. This PR should keep the same behavior than before.

angelozerr mentioned this pull request Oct 27, 2022

Provide .editorconfig support eclipse-platform/eclipse.platform.ui#877

Open

laeubi requested a review from mickaelistria October 28, 2022 04:30

angelozerr force-pushed the ITextViewerLifecycle branch from 0678e5e to baa001c Compare October 28, 2022 07:00

laeubi approved these changes Oct 28, 2022

View reviewed changes

angelozerr force-pushed the ITextViewerLifecycle branch from baa001c to f1befb6 Compare October 28, 2022 07:03

angelozerr mentioned this pull request Oct 28, 2022

Preference store provider extension point #109

Closed

szarnekow reviewed Oct 28, 2022

View reviewed changes

org.eclipse.jface.text/src/org/eclipse/jface/text/ITextViewerLifecycle.java Show resolved Hide resolved

angelozerr force-pushed the ITextViewerLifecycle branch 2 times, most recently from 9238e62 to a5c5564 Compare October 28, 2022 14:38

angelozerr mentioned this pull request Oct 28, 2022

LSP folding with ITextViewerLifecycle eclipse-lsp4e/lsp4e#272

Merged

angelozerr force-pushed the ITextViewerLifecycle branch 2 times, most recently from 34bbf1b to 2b25171 Compare October 28, 2022 16:02

angelozerr mentioned this pull request Nov 1, 2022

ITextEditorAware API to inject ITextEditor. #108

Closed

angelozerr force-pushed the ITextViewerLifecycle branch from 2b25171 to bd18dd4 Compare November 1, 2022 21:09

angelozerr changed the title ~~Introduce ITextViewerLifecycle API~~ API to Track TextViewer install/uninstall in generic editor contribution Nov 1, 2022

angelozerr force-pushed the ITextViewerLifecycle branch from bd18dd4 to fcc2033 Compare November 10, 2022 10:21

angelozerr force-pushed the ITextViewerLifecycle branch from f470b2a to 29d381e Compare November 10, 2022 13:42

szarnekow reviewed Nov 10, 2022

View reviewed changes

szarnekow approved these changes Nov 10, 2022

View reviewed changes

org.eclipse.jface.text/src/org/eclipse/jface/text/reconciler/MonoReconciler.java Outdated Show resolved Hide resolved

org.eclipse.jface.text/src/org/eclipse/jface/text/source/SourceViewer.java Outdated Show resolved Hide resolved

angelozerr force-pushed the ITextViewerLifecycle branch 2 times, most recently from 6ab3561 to 70fb8ae Compare November 10, 2022 17:22

angelozerr force-pushed the ITextViewerLifecycle branch from 70fb8ae to d22a9ce Compare November 10, 2022 17:25

angelozerr changed the title ~~API to Track TextViewer install/uninstall in generic editor contribution~~ API to Track TextViewer install/uninstall Nov 15, 2022

angelozerr mentioned this pull request Nov 15, 2022

Register IReconcilingStrategy with GenericEditor extension point. #115

Closed

mickaelistria force-pushed the ITextViewerLifecycle branch from d22a9ce to 4426e8c Compare November 15, 2022 07:40

angelozerr force-pushed the ITextViewerLifecycle branch from 4426e8c to 898f348 Compare December 1, 2022 13:48

mickaelistria approved these changes Dec 1, 2022

View reviewed changes

API to Track TextViewer install/uninstall in generic editor contribution

8dc80eb

Signed-off-by: azerr <[email protected]>

mickaelistria force-pushed the ITextViewerLifecycle branch from 898f348 to 8dc80eb Compare December 1, 2022 20:46

mickaelistria merged commit 81a4465 into eclipse-platform:master Dec 2, 2022

mickaelistria added noteworthy Noteworthy feature api labels Dec 2, 2022

cdietrich reviewed Dec 6, 2022

View reviewed changes

cdietrich reviewed Dec 8, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API to Track TextViewer install/uninstall #107

API to Track TextViewer install/uninstall #107

angelozerr commented Oct 27, 2022 •

edited

Loading

laeubi commented Oct 28, 2022

angelozerr commented Oct 28, 2022

laeubi left a comment

angelozerr commented Oct 28, 2022

angelozerr commented Oct 28, 2022

laeubi commented Oct 28, 2022

angelozerr commented Oct 28, 2022

mickaelistria commented Nov 4, 2022

angelozerr commented Nov 4, 2022 •

edited

Loading

mickaelistria commented Nov 4, 2022

angelozerr commented Nov 5, 2022

angelozerr commented Nov 5, 2022 •

edited

Loading

mickaelistria commented Nov 9, 2022

angelozerr commented Nov 9, 2022 •

edited

Loading

mickaelistria commented Nov 10, 2022

szarnekow Nov 10, 2022

angelozerr Nov 10, 2022

szarnekow left a comment

mickaelistria commented Nov 10, 2022

angelozerr commented Nov 10, 2022

angelozerr commented Nov 10, 2022

mickaelistria commented Nov 10, 2022

laeubi commented Nov 11, 2022 •

edited

Loading

laeubi commented Nov 11, 2022

vik-chand commented Nov 11, 2022

mickaelistria commented Nov 13, 2022

angelozerr commented Nov 14, 2022

mickaelistria commented Dec 1, 2022

mickaelistria commented Dec 2, 2022

mickaelistria commented Dec 2, 2022

cdietrich Dec 6, 2022

cdietrich Dec 6, 2022

cdietrich Dec 8, 2022

angelozerr Dec 8, 2022

API to Track TextViewer install/uninstall #107

API to Track TextViewer install/uninstall #107

Conversation

angelozerr commented Oct 27, 2022 • edited Loading

laeubi commented Oct 28, 2022

angelozerr commented Oct 28, 2022

laeubi left a comment

Choose a reason for hiding this comment

angelozerr commented Oct 28, 2022

angelozerr commented Oct 28, 2022

laeubi commented Oct 28, 2022

angelozerr commented Oct 28, 2022

mickaelistria commented Nov 4, 2022

angelozerr commented Nov 4, 2022 • edited Loading

mickaelistria commented Nov 4, 2022

angelozerr commented Nov 5, 2022

angelozerr commented Nov 5, 2022 • edited Loading

mickaelistria commented Nov 9, 2022

angelozerr commented Nov 9, 2022 • edited Loading

mickaelistria commented Nov 10, 2022

szarnekow Nov 10, 2022

Choose a reason for hiding this comment

angelozerr Nov 10, 2022

Choose a reason for hiding this comment

szarnekow left a comment

Choose a reason for hiding this comment

mickaelistria commented Nov 10, 2022

angelozerr commented Nov 10, 2022

angelozerr commented Nov 10, 2022

mickaelistria commented Nov 10, 2022

laeubi commented Nov 11, 2022 • edited Loading

laeubi commented Nov 11, 2022

vik-chand commented Nov 11, 2022

mickaelistria commented Nov 13, 2022

angelozerr commented Nov 14, 2022

mickaelistria commented Dec 1, 2022

mickaelistria commented Dec 2, 2022

mickaelistria commented Dec 2, 2022

cdietrich Dec 6, 2022

Choose a reason for hiding this comment

cdietrich Dec 6, 2022

Choose a reason for hiding this comment

cdietrich Dec 8, 2022

Choose a reason for hiding this comment

angelozerr Dec 8, 2022

Choose a reason for hiding this comment

angelozerr commented Oct 27, 2022 •

edited

Loading

angelozerr commented Nov 4, 2022 •

edited

Loading

angelozerr commented Nov 5, 2022 •

edited

Loading

angelozerr commented Nov 9, 2022 •

edited

Loading

laeubi commented Nov 11, 2022 •

edited

Loading