Comment 1 Eclipse Genie 2018-08-20 03:21:13 EDT

New Gerrit change created: https://git.eclipse.org/r/127681

Comment 2 Thomas Wolf 2018-08-20 03:58:30 EDT

org.eclipse.jface.text.source.projection.ProjectionViewer is another candidate. Subclassed at least in JDT, PDE, EGit, and Ant.

Comment 3 Lars Vogel 2018-08-20 04:00:08 EDT

Thanks, Ed. Sounds a reasonable suggestion. Adding Dani for comments.

Comment 4 Andrey Loskutov 2018-08-20 04:24:48 EDT

I would vote -1 for the change *as proposed*.

All public and protected methods are non final in the class. 

Who is going to update all javadocs and explaining if the method can/can't be overridden by clients? This is at least what I would expect form a change of "noextend" to "do whatever you want".

Comment 5 Ed Merks 2018-08-20 05:08:52 EDT

Is there any reason to believe/suspect/suppose that any non-final method should not be overridden?

I believe the current usage pattern suggests that it is already entirely evident that "do whatever you want" is the defacto prevailing policy as far as downstream subclasses are concerned.  Is there any evident to suggest that I ought to believe otherwise?

The more fundamental question here is the following:

  What is the actual problem with "do whatever you want"?

I can guess at the fundamental problem.  Such a policy makes it much more difficult to change the internals of the implementation and the platform developers would like as much freedom as possible to change these internals.  If, in the future, the platform developers choose to do so and that break all kinds of downstream implementations, they'd like to say "bad bad developers" you should not extend this class, it was clearly documented, and you made no effort to discuss enhancements of the API nor to contribute such enhancements.  So too bad for you, because we are free (and in fact have the API-given right) to make such arbitrary changes.  

That's all great for the platform developers, but the fact is that this would screw the community and the community has made it clear by their actions (if not their words) that the current policy is bogus. 

So I'm speaking out to make it clear that the entire implementation hierarchy has in fact been API for the community for a very very long time and that every non-final method has been an override point for the community.  That cannot be changed now, so there is nothing to document.

Let's take a step back and dive into some more related details. Let's have a look at AbstractTreeViewer, which we are allowed to extend...  

We find Javadoc as follows: 

	/**
	 * Adds the given child elements to this viewer as children of the given
	 * parent element.
	 * <p>
	 * EXPERIMENTAL. Not to be used except by JDT. This method was added to
	 * support JDT's explorations into grouping by working sets. This method
	 * cannot be removed without breaking binary backwards compatibility, but
	 * should not be called by clients.
	 * </p>
	 *
	 * @param widget
	 *            the widget for the parent element
	 * @param parentElementOrTreePath
	 *            the parent element
	 * @param childElements
	 *            the child elements to add
	 * @since 3.1
	 */
	protected void internalAdd(Widget widget, Object parentElementOrTreePath,
			Object[] childElements) {

Do you think any other client can so easily have a method added?  As always, JDT has special rights...

Next in that class we see:

	/**
	 * Returns the index where the item should be inserted.
	 *
	 * @param parent
	 *            The parent widget the element will be inserted into.
	 * @param element
	 *            The element to insert.
	 * @return the index of the element
	 */
	protected int indexForElement(Widget parent, Object element) {

Is it okay to override this? I guess so because it's not final. Is it important to call super when overriding?  That's not documented, but looking at the implementation, it calls org.eclipse.jface.viewers.AbstractTreeViewer.internalCompare(ViewerComparator, TreePath, Object, Object) which is private, so I'd be kind of crippled trying to override it.  But then, there are no overrides (in my target platform), so who knows why it isn't final.  Moreover, I can't find any callers of this method either.  Hmm.

Comment 6 Andrey Loskutov 2018-08-21 05:25:52 EDT

(In reply to Ed Merks from comment #5)
> Is there any reason to believe/suspect/suppose that any non-final method
> should not be overridden?

Original authors explicitly decided the class should not be extended, they obviously had some reasons to do so.

> I believe the current usage pattern suggests that it is already entirely
> evident that "do whatever you want" is the defacto prevailing policy as far
> as downstream subclasses are concerned.  Is there any evident to suggest
> that I ought to believe otherwise?

The javadoc say "do not extend" clearly. If clients ignore this, it is a client problem.
 
> The more fundamental question here is the following:
> 
>   What is the actual problem with "do whatever you want"?
> 
> I can guess at the fundamental problem.  Such a policy makes it much more
> difficult to change the internals of the implementation and the platform
> developers would like as much freedom as possible to change these internals.
> If, in the future, the platform developers choose to do so and that break
> all kinds of downstream implementations, they'd like to say "bad bad
> developers" you should not extend this class, it was clearly documented, and
> you made no effort to discuss enhancements of the API nor to contribute such
> enhancements.  So too bad for you, because we are free (and in fact have the
> API-given right) to make such arbitrary changes.  
> 
> That's all great for the platform developers, but the fact is that this
> would screw the community and the community has made it clear by their
> actions (if not their words) that the current policy is bogus. 

Not sure what do you want to achieve here. Once we remove "noextend" we explicitly define all protected methods to be API and we will not be able to change them anymore. I guess you are NOT going to provide support for all the future cases which will break if clients start to override and change "internal" implementation, because "we can do this, it is allowed now"?

> So I'm speaking out to make it clear that the entire implementation
> hierarchy has in fact been API for the community for a very very long time
> and that every non-final method has been an override point for the
> community. 

Internal implementation was never API because of "noextend". Whoever changed it by overriding protected methods, used it on its own risk.

> That cannot be changed now, so there is nothing to document.

You *are* trying to change this now by declaring internal implementation as API, and this is what it was never intended by original authors. So either we document and actively support this use case by changing internals *to allow* proper extension, or we keep current state.

Comment 7 Ed Merks 2018-08-21 06:17:48 EDT

(In reply to Andrey Loskutov from comment #6)
> Original authors explicitly decided the class should not be extended, they
> obviously had some reasons to do so.
>
I'm sure the point is to be able to make arbitrary changes backed by the API-given right to do so.
The authors also chose what to make public, protected, and private, and what to make final.
It would also seem reasonable to assume they had good reasons for all those choices.
None of this should be changed now, and none of this should be changed in the future, ever.
 
> The javadoc say "do not extend" clearly. If clients ignore this, it is a
> client problem.
>  
Yes, the API-given rights allow you to make that claim. 
But the unfortunate reality is that the platform team is its own client and has ignored this; they obviously had some reason to do so.
In any case, if the platform team itself can't set the highest of standards internally it seems completely inappropriate to expect the rest of the world to keep a higher standard than the one our own team follows.

> Not sure what do you want to achieve here. Once we remove "noextend" we
> explicitly define all protected methods to be API and we will not be able to
> change them anymore. I guess you are NOT going to provide support for all
> the future cases which will break if clients start to override and change
> "internal" implementation, because "we can do this, it is allowed now"?
> 
Yes, that is exactly what I want to achieve. I want to ensure that in the future, no arbitrary changes will occur without consider of the impact.
Future changes to these implementation classes must recognize that clients are using these implementations as API.
And of course the subclassing clients are to a large extent the platform team itself.

> Internal implementation was never API because of "noextend". Whoever changed
> it by overriding protected methods, used it on its own risk.
> 
Yes and it's the belaboring of this point that I want to put to a stop.
I want a recognition of the fact that many interesting and useful things are possible only by subclassing.  The evidence of that fact speaks for itself.

> 
> You *are* trying to change this now by declaring internal implementation as
> API, and this is what it was never intended by original authors. So either
> we document and actively support this use case by changing internals *to
> allow* proper extension, or we keep current state.
>
What can I say?  This same issue always becomes a circular one.  The API-rights activists maintain a fire wall. The argument is always that something isn't API and one must make it API before using it and that involves lots of discussions first. But then the endless blocking hurdles come up, ones that are too high to clear, and of course who really has time for any of that in the first place.  So the firewall remains and the activists maintain their rights along with the status quo.

------------------

Perhaps you can spell out one specific example of what Javadoc could/would/should be added to TreeViewer because I really don't have the first clue of how to jump over your hurdle.

Comment 8 Ed Merks 2018-08-24 01:49:08 EDT

(In reply to Andrey Loskutov from comment #4)
> I would vote -1 for the change *as proposed*.
> 
> All public and protected methods are non final in the class. 
> 
> Who is going to update all javadocs and explaining if the method can/can't
> be overridden by clients? This is at least what I would expect form a change
> of "noextend" to "do whatever you want".

I've never seen Javadoc that says "you may override this method", of course I may if the method private or not final; nor have I seen ever Javadoc that says "you may not override this method", because of course I can't if it's private or final.

Note that *every* protected method in TreeViewer is an override of one from AbstractTreeViewer, and note that and all AbstractTreeViewer class says with regard to subclassing is:

 * <strong> This class is not intended to be subclassed outside of the JFace
 * viewers framework.</strong>

But without in the Javadoc:

 * @noextend This class is not intended to be subclassed by clients.

So I'm really not sure what the original authors are thinking. Is AbstractTreeViewer API or is it in some gray limbo zone?  

In the end, it's not clear what specifically you are expecting to see in the Javadoc.

Comment 9 Dani Megert 2018-08-24 05:04:50 EDT

I agree with Andrey.

The only path forward here would be someone investing (big) time to define the APIs. This would make the subclasses legal but some of the overridden or used members would still be flagged as illegal.

Comment 10 Ed Merks 2018-08-24 05:15:48 EDT

Again(In reply to Dani Megert from comment #9)
> I agree with Andrey.
> 
> The only path forward here would be someone investing (big) time to define
> the APIs. This would make the subclasses legal but some of the overridden or
> used members would still be flagged as illegal.

This is also a very vague comment. 

This whole issue completely confusing and is made further confusing by the fact that this comment was added to AbstractTreeViewer late in 2015:

 * <p>
 * <strong> This class is not intended to be subclassed outside of the JFace
 * viewers framework.</strong>
 * </p>

I can't imagine how one can retroactively forbid something...

In any case, would it be possible for someone to give a concrete/specific example of a method in TreeViewer that shouldn't be called (yet isn't private) and shouldn't be overridden (yet isn't final) with a justification for that determination along with a suggestion for how that affects what's in the Javadoc.

Comment 11 Dani Megert 2018-08-24 05:46:22 EDT

(In reply to Ed Merks from comment #10)
> In any case, would it be possible for someone to give a concrete/specific
> example of a method in TreeViewer that shouldn't be called (yet isn't
> private) and shouldn't be overridden (yet isn't final) with a justification
> for that determination along with a suggestion for how that affects what's
> in the Javadoc.

That's exactly the big work I mentioned. The original developers decided not to deal with that (at least at that point) and hence declared it as @noextend.

Comment 12 Ed Merks 2018-08-24 06:33:25 EDT

(In reply to Dani Megert from comment #11)
> (In reply to Ed Merks from comment #10)
> > In any case, would it be possible for someone to give a concrete/specific
> > example of a method in TreeViewer that shouldn't be called (yet isn't
> > private) and shouldn't be overridden (yet isn't final) with a justification
> > for that determination along with a suggestion for how that affects what's
> > in the Javadoc.
> 
> That's exactly the big work I mentioned. The original developers decided not
> to deal with that (at least at that point) and hence declared it as
> @noextend.

I'm doubtful about your conclusion given that all the rest of the team(s) decided to create subclasses with overrides. Perhaps in each case the developer(s) concluded that that discussing the reasoning for @noextend on TreeViewer is futile or realized that so much "big work" would the tossed their way that it is effectively futile.

I've concluded that because all the protected methods of TreeViewer are themselves overrides they are more than reasonable to override further. These methods will always be required to exist because they are declared in AbstractTreeViewer, though it has a dubious API status that seems to have arbitrarily changed in 2015. 

But how can I prove this assertion when no "gate keep" is willing to do even the tiniest part of the "big work"? 

There are only 43 public/protected methods in TreeViewer; the following protected methods are in fact overridden elsewhere already:

org.eclipse.jface.viewers.TreeViewer.hookControl(Control)
org.eclipse.jface.viewers.TreeViewer.setExpanded(Item, boolean)
org.eclipse.jface.viewers.TreeViewer.setChildCount(Object, int)
org.eclipse.jface.viewers.TreeViewer.internalAdd(Widget, Object, Object[])
org.eclipse.jface.viewers.TreeViewer.internalRefreshStruct(Widget, Object, boolean)
org.eclipse.jface.viewers.TreeViewer.internalRefreshStruct(Widget, Object, boolean)
org.eclipse.jface.viewers.TreeViewer.handleTreeExpand(TreeEvent)
org.eclipse.jface.viewers.TreeViewer.handleTreeCollapse(TreeEvent)

The public methods are more commonly overridden.

That just leaves the following 31 protected methods as candidates for something someone might theoretically argue should never be overridden.  (Or even called?)

addTreeListener(Control, TreeListener)
getColumnViewerOwner(int)
getChildren(Widget)
getExpanded(Item)
getItemAt(Point)
getItemCount(Control)
getItemCount(Item)
getItems(Item)
getParentItem(Item)
getSelection(Control)
hookControl(Control)
createViewerEditor()
newItem(Widget, int, int)
removeAll(Control)
setExpanded(Item, boolean)
setSelection(List<Item>)
showItem(Item)
getChild(Widget, int)
assertContentProviderType(IContentProvider)
getRawChildren(Object)
getParentElement(Object)
internalAdd(Widget, Object, Object[])
internalRefreshStruct(Widget, Object, boolean)
mapElement(Object, Widget)
getViewerRowFromItem(Widget)
internalInitializeTree(Control)
updatePlus(Item, Object)
handleTreeExpand(TreeEvent)
handleTreeCollapse(TreeEvent)
disassociate(Item)
doGetColumnCount()

Surely it's not so much "big work" for someone to point out why one of these should never be overridden. (Or called?) 

I can't imagine there being such a reason and assert, after significant investigation, that this is not the case.

Comment 13 Lars Vogel 2019-02-28 04:45:48 EST

Marking as wontfix for now, as the committers disagrees and no works seems to happen here.

Ed, please bring this to the PMC if you think this is important enough.

Comment 14 Ed Merks 2019-02-28 08:03:01 EST

(In reply to Lars Vogel from comment #13)
> Marking as wontfix for now, as the committers disagrees and no works seems
> to happen here.
> 

Given no one actually wants to spell out what exactly they require in order to make progress, it seems clear to me that this will just be stone walled indefinitely.

The current state of affairs completely flies in the face of reality, e.g., Nebula as table and tree viewer subclasses and even within the projects under the Eclipse PMC umbrella the "rules" are not respected. But no one seems to care, so why should I (or anyone else) care about meaningless rules when no one cares?

The most unfortunate fact is that the current state of affairs will be used as a weapon to be aimed at the bad people that violate the rules, despite the fact that JDT and another Eclipse projects are apparently exempted from said rules.  The future argument will be that one should negotiate the APIs, but this Bugzilla will illustrate that such negotiation is not possible because one side of the negotiation simple doesn't want these to be APIs and therefore sets hurdles of mystery height separated by mystery distance to ensure that anyone attempting the run will fail flat.

> Ed, please bring this to the PMC if you think this is important enough.

What I think is important is that there be a record to illustrate that negotiations such as this cannot be successful.

Comment 15 Phil Beauvoir 2019-12-03 03:51:24 EST

+1

Been extending TreeViewer for about 15 years now. Didn't even see @noextend there. Bu then I'm just a stupid Eclipse hacker who doesn't read the headers and instead follows all the tutorials and examples of the usage of TreeViewer out there for the last 15 years.