Re: [cross-project-issues-dev] JFace Generics

[Ed Merks <ed.merks@xxxxxxxxx>]

Michael,

Comments below.

On 29/08/2013 3:25 PM, Mickael Istria wrote:

On 08/29/2013 02:36 PM, Ed Merks wrote:

Not to rain on anyone's parade, but respectfully suggest the platform team should reconsider the gain verses the client impact for generifying the JFace APIs.   Designing generic containers isn't easy, and it's all too easy to make mistakes that will be difficult to correct in the future
[...]
In the end, many of the things being changed are effectively SPI.  One implements the APIs for providers and passes them to a generic container that does the right things with it.  There's little to be gained from adding generics, to justify the cost to the ecosystem, and getting it right is much harder than it appears at first glance...

Although I trust people saying that adding generics is difficult and error-prone (<T> or <? extends T> ?), I believe that adding generics in JFace has a lot of value, simply because of the number of cast necessary when dealing with a ContentProvider/LabelProvider couple. Dealing with current APIs forces us to remind the actual types for the JFace viewer, and it is a tricky exercise that is error prone: I've often seen/written some ClassCastException that takes more time to notice and debug that if I could see immediate feedback in IDE thanks to generic.
Overall, adding generics to JFace will really boost productivity of JFace users. That's IMO a valid reason to introduce warnings on existing code.

No, I 99% disagree.  The vast majority of the implementations of org.eclipse.jface.viewers.ITreeContentProvider.getChildren(Object) look like this.

        public Object[] getChildren(Object parent) {
            // Need an identifying class for label provider
            if (parent instanceof ImportPackageHeader) {
                return ((ImportPackageHeader) parent).getPackages();
            } else if (parent instanceof ExportPackageHeader) {
                return ((ExportPackageHeader) parent).getPackages();
            } else if (parent instanceof RequiredExecutionEnvironmentHeader) {
                return ((RequiredExecutionEnvironmentHeader) parent).getEnvironments();
            } else if (parent instanceof RequireBundleHeader) {
                return ((RequireBundleHeader) parent).getRequiredBundles();
            } else if (parent instanceof BundleClasspathHeader) {
                return getPluginLibraries();
            }
            return new Object[0];
        }

Even ones that look like this would need to be rewitten to use a different toArray call.

        public Object[] getChildren(Object element) {
            if (element instanceof IVariableBinding) {
                List<DelegateEntry> result= new ArrayList<DelegateEntry>();
                for (int i= 0; i < fDelegateEntries.length; i++) {
                    if (element == fDelegateEntries[i].field) {
                        result.add(fDelegateEntries[i]);
                    }
                }
                return result.toArray();
            }
            return null;
        }

What is the current issue?

I explained the issue. 

Is it that existing code using JFace will now show warnings?

Absolutely that's a major issue.

Is this really an issue, does it prevent code to work?

Some of us maintain warning free code religiously.

I don't think so.

Some of us apparently don't care about warnings and will just suppress them.  But I'm definitely not in that camp, and strive to generate code that uses no raw types...

Those warnings just reveals that current usages of JFace don't leverage this new smart mechanism to prevent from ClassCastException.

The majority of such uses will not benefit from this; they will be forced to use Object and keep their instanceof tests...

It's not a big deal to see them.

Yes, it is.  It's unacceptable my code base and EMF strives not generate raw type warnings.

IMO, just asking to roll back everything is just like disabling a FindBugs rule: it doesn't solve anything, it just hides a place for improvement. In the end, it reduce code quality.

No, it's not like that at all.  As I explained, the implementation is error prone and questionable and I don't expect the platform to distribute a marginally completed flawed code base.

So, IMHO, the balance is more in favor of adding generics that in many cases will provide a better productivity and quality.

In the vast majority of cases it provides little or no value, and given you seem to feel most people can just ignore it all, you'll find that all the framework you use, will end up providing raw types and will still give you no value, just endless warnings that you'll be powerless to address.  Like the annoying signature of org.eclipse.core.runtime.IAdaptable.getAdapter(Class) I have to implement like this:

  @SuppressWarnings("rawtypes")
  public Object getAdapter(Class adapter)
  {
    if (EMFPlugin.IS_RESOURCES_BUNDLE_AVAILABLE)
    {
      Object result = EclipseUtil.getAdapter(adapter, uri);
      if (result != null)
      {
        return result;
      }
    }
    return null;
  }

And I think it is totally fine to start merging such change so early in the release train to get maximal feedback.

It's totally not fine to do so unannounced, incomplete, and with design flaws, so I'm mostly certainly giving my maximal feedback. 

I see (in Gerrit; thanks Matthias) that you've committed changes for TreeViewer and here I think the whole approach is completely questionable.  When is it the case that a tree view has uniformly the same elements throughout?   I think that's so rarely the case that it's worse than useless to make such an assumption; I would argue it's mostly just noise that will never solve real problems.

I've seen many TreeViewers using a super type other than Object, especially in the modeling world, where almost everything is an EObject (and often a specialized super-type shared across the whole metamodel),

Yes, but nevertheless, views often contain wrappers and it's often the case that generated APIs don't explicitly extend EObject, so that argument doesn't fly.

or in some JDT or PDE code where there are some TreeNodeAdapter (or whatever pattern which create a dedicated type to put in TreeContentProvider). So it seems to make sense for TreeViewers as well. And for other cases, it's not a big deal to write "TreeContentProvider<? extends Object>".

Yes, it is a big deal.  Revisiting 100,000 of thousands of lines of code and revisiting the generator, where 99% of which will just use ? or Object is not very palatable.  Of course if I saw the value balance well against the impact, I'd be happier to accommodate, but truly I think it's questionable at best.

--
Mickael Istria
Eclipse developer at JBoss, by Red Hat
My blog - My Tweets

_______________________________________________
cross-project-issues-dev mailing list
cross-project-issues-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/cross-project-issues-dev