search

eclipse-platform / eclipse.platform.text

8 stars 45 forks source link

API to Track TextViewer install/uninstall #107

Closed angelozerr closed 1 year ago

angelozerr commented 2 years ago

API to Track TextViewer install/uninstall

Signed-off-by: azerr azerr@redhat.com

My motivation to provide this API is

laeubi commented 2 years ago

@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 commented 2 years ago

Thanks @laeubi for your feedback.

It should be fixed now.

angelozerr commented 2 years ago

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 commented 2 years ago

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

laeubi commented 2 years ago

@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.

angelozerr commented 2 years ago

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

I created a PR on LSP4e side which consumes this API https://github.com/eclipse/lsp4e/pull/272

mickaelistria commented 2 years ago

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 commented 2 years ago

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

So it means that SourceViewershould store a list of ITextViewerLifecycleand 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 / uninstallmethods instead of hard coding that. Is this idea that you would like to follow ?

mickaelistria commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/eclipse-platform/eclipse.platform.text/pull/108

Now the question is where host those 2 APIs?

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 commented 1 year ago

t according https://github.com/eclipse-platform/eclipse.platform.text/pull/107#discussion_r1007844308 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 commented 1 year ago

@mickaelistria is https://github.com/eclipse-platform/eclipse.platform.text/pull/107#issuecomment-1303964453 will follow your idea?

mickaelistria commented 1 year ago

@mickaelistria is https://github.com/eclipse-platform/eclipse.platform.text/pull/107#issuecomment-1303964453 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.

angelozerr commented 1 year ago

@mickaelistria I push a new commit, please tell me if it is was your idea.

angelozerr commented 1 year ago

[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 interface org.eclipse.jface.text.information.IInformationPresenter

Could you help me please to fix that?

akurtakov commented 1 year ago

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

mickaelistria commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

vik-chand commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

mickaelistria commented 1 year ago

@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.

mickaelistria commented 1 year ago

Thank you!

mickaelistria commented 1 year ago

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