What do you mean by "internal"? Protected may be? It is marked as "EXPERIMENTAL" anyway.

Currently it is needed by clients, just see uses of it. Also if we add some item to the viewer and clients manipulate items in some way, they may want know if that item is one that they don't want to touch in any way. Without exposing this to clients they will have no possibility to do that.

What do you mean by "internal"? Protected may be?

Something like protected + @noreference, or even just package-visible is possible, to fully dissuade people to bind to this method.

Currently it is needed by clients, just see uses of it.

But it's more a smell that the API is still to be improved IMO.
The only clients I see are tests. But usage in tests is questionable:

ISelection selection = tableViewer.getSelection();
assertTrue("Selection must not be empty", selection instanceof IStructuredSelection);
Object selEle = ((IStructuredSelection) selection).getFirstElement();
assertTrue("Selection must be ExpandableNode", tableViewer.isExpandableNode(selEle);

This block seems to break the 24-years-old invariant that an element in the selection comes from the model/content provider. The placeholder internal expandable node should never be part of the selection because it is not part of the model, and should always be excluded from model-oriented results.
If one wants to access this element for testing purpose, they should go to the lower-level API, using the selection on the Tree widget and item.getData() to retrieve the element.

Also if we add some item to the viewer and clients manipulate items in some way, they may want know if that item is one that they don't want to touch in any way.

That assumption sounds wrong to me. Clients don't want to have to deal with this new object and consider it as something new, it's "just" something they may want to enable to optimize their rendering, not some business-logic.

The only clients I see are tests. But usage in tests is questionable

The test actually tests what one don't want the actual assertion should be that the selection is EMPTY in such case even if the node is there...

The only clients I see are tests

Nope, real code uses that. Please search in all of SDK.

Clients don't want to have to deal with this new object

Sure, but they sometimes have to understand if that is this new object or not because they may get it via various ways, just asking tree for an item at index etc.

Can you please at least make that getSelection() doesn't return the expandable node? This will avoid most of the risk of ClassCastException and the reduce drastically the need for all consumers to use this method.

they may get it via various ways, just asking tree for an item at index etc.

Indeed, it's probable. Are any of the consumers in Platform concerned by this approach or do they all use JFace's getSelection()?

See code in

• org.eclipse.jdt.internal.ui.packageview.PackageExplorerPart.PackageExplorerProblemTreeViewer.evaluateExpandableWithFilters(Object)
• org.eclipse.jdt.internal.ui.javaeditor.JavaOutlinePage.JavaOutlineViewer.isExpandable(Object)

The client gets some object via some JFace API and needs to make decisions based on the object type. Cases above is about "can I expand that item" and the answer should be "no" because expanding this "internal" node shouldn't be done by client code (they simply can't).

There is not much code yet simply because we have only few clients converted, but if Search view (which was on the list but we disabled it for now) decides hot to go next/previous result by looking into the item's data and here again, they will need to know if the next node in the tree/table is the "artificial" one or not (in first case they would ask viewer to show next items).

The client gets some object via some JFace API

This is a major issue, and an important breakage, in this design. The node shouldn't be queried for things like isExpandable, it shouldn't be queried for children, filtering nor anything provided by the consumer.
Here the internal is leaking into the model management, which has been stable for many many years. It is a source of breakage, particularly coupled with the preference applying to all trees.

It is a source of breakage, particularly coupled with the preference applying to all trees.

Note, the preference is not applied to all trees, it is applied only to viewers that explicitly enable new limit.

Here the internal is leaking into the model management, which has been stable for many many years.

The only reason for that is that we did not found any better solution that would fix a problem existed for many many years... The solution we have right now is may be not perfect one but solves the problem existed for many many years in a way that most clients would not even notice any change. Also the amount of new API exposed to clients (one method) is minimal, and they are not forced to use this API either. I believe this is really remarkable achievement.

This is a major issue, and an important breakage, in this design.

Why that? As explained, none of the existing clients are affected, so no breakage happens.

Clients have to touch their code to enable the feature and if they are doing that, they have to test and adopt the code if needed. Yes, if clients enable new limit and manually inspect or manipulate SWT objects and expect that all of them are created with client side model objects, they may need changes. Looking on the client side changes we made already in SDK, the changes are minimal, compared to the impact on the changed functionality.

Note, the preference is not applied to all trees, it is applied only to viewers that explicitly enable new limit.

Ah, OK. My bad.

The solution we have right now is may be not perfect one but solves the problem existed for many many years in a way that most clients would not even notice any change. Also the amount of new API exposed to clients (one method) is minimal, and they are not forced to use this API either. I believe this is really remarkable achievement.

I'm not saying it's all bad, and I do clearly see the value and the amount of high quality work that was spent here and am grateful for that.
I'm just pushing that the node can and should be even more internal. I don't challenge the whole design, I just challenge this small current issue.

Why that? As explained, none of the existing clients are affected, so no breakage happens.

I overinterpreted the issue. it's indeed not a breakage as long as it doesn't apply to existing trees.

Yes, if clients enable new limit and manually inspect or manipulate SWT objects and expect that all of them are created with client side model objects, they may need changes.

I think we can make it so that the node is never going through customer APIs (not queried for any extension). So we have the best of both worlds: the feature is still here, and in the vast majority of cases requiring no change in existing code beyond just enabling it.

I think we can make it so that the node is never going through customer APIs

I agree that we should hide a s much as possible, but if we "touch" SWT tree/table structure (which we do), the client side will be able to see this change by using standard SWT API. There is no way to fully "hide" SWT changes.

Having it visible at SWT level is fine, and even desirable I think. It's only the JFace APIs that must hide it as much as possible.

As I've stumbled upon more code using this method, I believe there shouldn't be an element for the "Expandable Node", it would be best to not instantiate it and just instantiate a TreeItem directly, and have the API becoming isExpandItem.

Not sure I understand that. Where should the code/data from ExpandableNode go then? And how should internal viewer code know that the Tree/Table Item in question belongs to the non created ExpandableNode object?

Where should the code/data from ExpandableNode go then?

The expandableNode doesn't own much data per se, its content can be deduced from the viewer's state.
And if it has data, it can use item.getData.

And how should internal viewer code know that the Tree/Table Item in question belongs to the non created ExpandableNode object?

if (item.getData() -- null) or adding anoter data to this item, so it would be item.getData(ColumnViewer.EXPAND_NODE_DATA_FLAG) != null. Or the expand items could be stored by the viewer as it creates them, and that would check some getExpandItems().contains(thisIItem).

Mickael, not sure it will work, please look at the code once again. Any item might have no data, this would not mean it is "special node". ExpandableNode data can't be deduced from Viewer internal state, this is the point of the change to not push everything into viewer.

TLDR: If you have some concrete proposal, it would be better to open draft PR and discuss it there.

I will try to submit a PR fixing remaining API issues.

Here is a proposal: #1061 , which I believe makes things more consistent with current implementation of filters and other stuff, and that will reduce the need for consumer to address the particular case of ExpandableNode when they use only JFace APIs.