Bug 224390 - [TabbedProperties] Added method tabSelected(ITabDescriptor) in an interface that is intended to be implemented
Summary: [TabbedProperties] Added method tabSelected(ITabDescriptor) in an interface t...
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.4 RC2   Edit
Assignee: Anthony Hunter CLA
QA Contact:
URL:
Whiteboard:
Keywords: api
Depends on:
Blocks: 225601
  Show dependency tree
 
Reported: 2008-03-27 11:05 EDT by Boris Bokowski CLA
Modified: 2008-08-05 18:30 EDT (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Boris Bokowski CLA 2008-03-27 11:05:58 EDT
ITabSelectionListener.java in org.eclipse.ui.views.properties.tabbed
Comment 1 Boris Bokowski CLA 2008-03-31 16:39:32 EDT
Here is another one:

The method updateTabs(TabDescriptor[]) has been removed from TabbedPropertySheetPage.java.

We should fix these API issues soon.
Comment 2 Anthony Hunter CLA 2008-03-31 17:18:03 EDT
Hi Boris,

I am not sure I agree that we can fix these.

In both cases, we have fixed non functional API that would have been caught by API tooling if such beast had existed in 3.3.

TabDescriptor is internal, but we have two cases were our public API depends on TabDescriptor. So we have replaced with the correct public class ITabDescriptor (and TabDescriptor implements ITabDescriptor ).

In 3.3 nobody noticed or complained about the internal access warnings. But in 3.4 we now have clients who have migrated from WTP and noticed the issue. The code in 3.3 was originally there for the dynamic tabbed support, but we did not actaully complete the dynamic tabbed support until 3.4 M2 ( Bug 131855 ).



 
Comment 3 Boris Bokowski CLA 2008-04-02 12:44:53 EDT
I talked to the PMC (McQ) and he said that we should try to remain binary compatible with 3.3 even if it means deprecating the existing addListener/removeListener methods as well as the listener type, and then adding the proper API.

Unfortunately, this is going to require changes in WTP since they are using the incompatibly changed API. Can we coordinate this with them to minimize the breakage? Do you know who on the WTP side is using this?

To be clear, the alternative would be to leave the breaking API change in, but then we would have to increase the major version number of the tabbed properties plugin (i.e. make it 4.0). A change like this would probably affect a lot of downstream clients - they would have to update the version range in their plugin dependencies to ensure that their application even comes up properly.

(Do you see any other possibility?)
Comment 4 Anthony Hunter CLA 2008-04-02 13:06:34 EDT
I will have to play with the API tools again and retest.

Note no WTP clients will be broken, dynamic tab adoption is a 3.4 event.
Comment 5 Anthony Hunter CLA 2008-04-04 15:25:27 EDT
OK looked at this another time.

In Eclipse 3.3, we had an API that broke the rules:

ITabSelectionListener Leaks non-API type
org.eclipse.ui.internal.views.properties.tabbed.view.TabDescriptor via method
parameter

There are actually three places in Eclipse 3.3 where we leaked internal API.
These are displayed when we run the API tools on the Eclipse 3.3 code.

In Eclipse 3.4, adopters caught this fact and we resolved with a proper API.
There are now no API leakages.

But now when we saying that we want to put these leaked methods back because we
are causing API breakage.

One of the other API breakage according to the API tools in 3.4 is:

TabbedPropertySheetPage The method getCurrentTab() has been removed

In fact I cannot fix this one, The old method returned the internal class Tab.
We are now returning API TabContents. I cannot put a backward compatible API in
since Tab does not exist anymore, we made this internal API public.

In 3.4 we fixed these API leads, as discovered when someone actually adopted
and raised internal API issues on us.

So in fact the only the only users we break are those who used internal API.
And adding these API will cause the original three API tools breakages we had
in Eclipse 3.3.

Bottom line, the only users who we are breaking are those who use internal API
in Eclipse 3.3.
Comment 6 Anthony Hunter CLA 2008-04-04 15:27:08 EDT
(In reply to comment #1)
> The method updateTabs(TabDescriptor[]) has been removed from
> TabbedPropertySheetPage.java.

I do not see this error with HEAD even if I set all the API tools settings to report errors.
Comment 7 Anthony Hunter CLA 2008-04-04 15:30:10 EDT
(In reply to comment #3)
> To be clear, the alternative would be to leave the breaking API change in, but
> then we would have to increase the major version number of the tabbed
> properties plugin (i.e. make it 4.0). A change like this would probably affect
> a lot of downstream clients - they would have to update the version range in
> their plugin dependencies to ensure that their application even comes up
> properly.

Making the major version 4.0 will break everyone, this is definitely now an option.

All we are saying now is to break anyone who use internal API in Eclipse 3.3 and expect their code to work in 3.4.
Comment 8 Anthony Hunter CLA 2008-04-04 15:35:34 EDT
These are the three API tools errors we fixed in 3.4 M2:

TabbedPropertySheetPage.java Leaks non-API type org.eclipse.ui.internal.views.properties.tabbed.view.Tab via return type
ITabSelectionListener.java Leaks non-API type org.eclipse.ui.internal.views.properties.tabbed.view.TabDescriptor via method parameter
TabbedPropertySheetPage.java Leaks non-API type org.eclipse.ui.internal.views.properties.tabbed.view.TabDescriptor via method parameter

By fixing these three problems with API for our clients, we have introduced the other issues raised in this Bugzilla.
Comment 9 Boris Bokowski CLA 2008-04-04 15:37:40 EDT
(In reply to comment #8)
> By fixing these three problems with API for our clients, we have introduced the
> other issues raised in this Bugzilla.

Why don't you fix them in a way that does not cause API incompatibilities?
Comment 10 Boris Bokowski CLA 2008-04-04 15:42:41 EDT
(In reply to comment #7)
> Making the major version 4.0 will break everyone, this is definitely now an
> option.
> 
> All we are saying now is to break anyone who use internal API in Eclipse 3.3
> and expect their code to work in 3.4.

You probably meant to write "not an option"?
Comment 11 Anthony Hunter CLA 2008-04-04 16:24:38 EDT
(In reply to comment #7)
> Making the major version 4.0 will break everyone, this is definitely now an
> option.
opps, yes, this is not an option.
Comment 12 Mike Wilson CLA 2008-04-07 10:41:45 EDT
Meta-comment: Early on in the 3.4 cycle, we decided that application compatibility between 3.3 and 3.4 is one of the most important drivers. I am always in favor of solutions that do not break existing code, if that is possible.

Comment 13 Anthony Hunter CLA 2008-04-07 11:05:27 EDT
(In reply to comment #12)
> Meta-comment: Early on in the 3.4 cycle, we decided that application
> compatibility between 3.3 and 3.4 is one of the most important drivers. I am
> always in favor of solutions that do not break existing code, if that is
> possible.
> 

At the highest level, we leaked non-API (internal) type in 3.3.

I tried on the weekend, but I cannot find out a way to fix this in the next 3.4 release, without breaking API.

I am going to create an independent example for us to bang at to demonstrate the problem.

It never occurred to me in 3.4 M2 that removing a method that leaded internals would be an issue, since the only users who would be broken were those that we making use of Eclipse internals. 

Comment 14 Anthony Hunter CLA 2008-05-21 16:18:41 EDT
Adopted the API tooling for the plug-in.
Added the missing @since tags.
Added filters to remove the issues when we fixed the internal leaks.  
Comment 15 Stefan Baramov CLA 2008-08-01 17:08:34 EDT
I understand that this is resolved, but I want to make this clear for myself or may be for others. The change in the TabbedPropertySheetPage makes a public API class totally unusable for a plug-in that need to run on both 3.3 and 3.4. I see two possible way to solve this: 

a) Never, ever refer to TabbedPropertySheetPage in any possible way except through reflection. Which means: 
   new TabbedPropertySheetPage(this)
renders in 
   Class.forName("org.eclipse.ui.views.properties.tabbed.TabbedPropertySheetPage")
               .getConstructor(ITabbedPropertySheetPageContributor.class)
                   .newInstance(this)

b) create a two fragments, one for 3.3 and another one for 3.4 that handle the tabbed property sheet page. Then the build process is kind of complicated because part of the app should be compiled against 3.3 and part 3.4. 

c) maintain two separate builds, one build against 3.3 and another one build against 3.4. 


What is the preferred/recommend way of handling cases like this. Is there a better solution. If there is, I am wonder can someone shows an example how to correctly use TabbedPropertySheetPage in a plug-in that runs across both 3.3 and 3.4

Thanks,
Stefan
Comment 16 Boris Bokowski CLA 2008-08-05 15:53:08 EDT
sorry, wrong bug
Comment 17 Anthony Hunter CLA 2008-08-05 16:13:26 EDT
(In reply to comment #15)
> I understand that this is resolved, but I want to make this clear for myself or
> may be for others. The change in the TabbedPropertySheetPage makes a public API
> class totally unusable for a plug-in that need to run on both 3.3 and 3.4.

What API are you using in 3.3 that is different or broken in 3.4?
Comment 18 Stefan Baramov CLA 2008-08-05 16:40:20 EDT
I am using  org.eclipse.ui.views.properties.tabbed.TabbedPropertySheetPage.getCurrentTab() method. It returns org.eclipse.ui.views.properties.tabbed.TabContents in Eclipse 3.4 and return org.eclipse.ui.internal.views.properties.tabbed.view.Tab in Eclipse 3.3.  Note, the Tab class does not exist in 3.4 at all. 

My class, being an editor part, instantiates the TabbedPropertySheetPage and it compiles fine against both Eclipse 3.3 and 3.4. However it does not link. My application is compiled against 3.3 code base. When I deploy it on 3.4 code base I get 

java.lang.NoClassDefFoundError: org/eclipse/ui/internal/views/properties/tabbed/view/Tab
at com.infor.clearux.studio.ui.editor.page.UIElementEditorPart.createPages(UIElementEditorPart.java:152)
at org.eclipse.ui.part.MultiPageEditorPart.createPartControl(MultiPageEditorPart.java:310)
at org.eclipse.ui.internal.EditorReference.createPartHelper(EditorReference.java:661)
at org.eclipse.ui.internal.EditorReference.createPart(EditorReference.java:428)
at org.eclipse.ui.internal.WorkbenchPartReference.getPart(WorkbenchPartReference.java:594)
at org.eclipse.ui.internal.PartPane.setVisible(PartPane.java:306)
at org.eclipse.ui.internal.presentations.PresentablePart.setVisible(PresentablePart.java:180)
at org.eclipse.ui.internal.presentations.util.PresentablePartFolder.select(PresentablePartFolder.java:270)
at org.eclipse.ui.internal.presentations.util.LeftToRightTabOrder.select(LeftToRightTabOrder.java:65)
at org.eclipse.ui.internal.presentations.util.TabbedStackPresentation.selectPart(TabbedStackPresentation.java:473)
at org.eclipse.ui.internal.PartStack.refreshPresentationSelection(PartStack.java:1256)
.....
Caused by: java.lang.ClassNotFoundException: org.eclipse.ui.internal.views.properties.tabbed.view.Tab
at org.eclipse.osgi.framework.internal.core.BundleLoader.findClassInternal(BundleLoader.java:481)

If I compile against 3.4 and deploy on 3.3 I get the same error except this time the missing class is the Tab class. 


Therefore I gradually reached the conclusion, that if I want to make my app works on both 3.3 and 3.4 I would have to eliminate all direct references and imports of the TabbedPropertySheetPage. 
Comment 19 Anthony Hunter CLA 2008-08-05 17:21:14 EDT
(In reply to comment #18)
> [...] return org.eclipse.ui.internal.views.properties.tabbed.view.Tab
> in Eclipse 3.3.  [...]

Exactly, Tab was in an internal package. *internal* meanings it was not API and you cannot use it. Which is why we fixed it in 3.4.
Comment 20 Boris Bokowski CLA 2008-08-05 17:33:25 EDT
If your code absolutely needs to call getCurrentTab (i.e. it needs to access internals when running against 3.3), I would recommend using reflection as you described under a) in comment 15.
Comment 21 Stefan Baramov CLA 2008-08-05 18:30:31 EDT
I resolved the problem. To instantiate the TabbedPropertySheetPage, I have to extend it with my own class. Next, I have to use reflection to call the impacted method - getCurrentTab(). The source code below does not cause NoClassDefFoundError and could be used for future reference: 

package com.mycompany.myprod.ui.editor.page;

import java.lang.reflect.Method;

import org.eclipse.ui.views.properties.tabbed.ITabbedPropertySheetPageContributor;
import org.eclipse.ui.views.properties.tabbed.TabbedPropertySheetPage;

import com.mycompany.myprod.ui.Activator;

/**
 * An extension of the {@link TabbedPropertySheetPage} to handle
 * Incompatibilities between Eclipse 3.3 and 3.4.
 *
 */
public class UIElementTabbedPropertySheetPage extends TabbedPropertySheetPage {

    /**
     * A compatibility wrapper of the TabContents class, that did not exist in
     * Eclipse 3.3 and it was added in 3.4.
     */
    public static class TabContentWrapper {

        /**
         * the current property sheet page.
         */
        private final UIElementTabbedPropertySheetPage page;

        /**
         * Constructor for the TabContentWrapper class.
         *
         * @param page
         *            the current property sheet page.
         */
        public TabContentWrapper(final UIElementTabbedPropertySheetPage page) {
            this.page = page;
        }

        /**
         * Sends the lifecycle event to the page's current tab.
         *
         */
        public void aboutToBeShown() {
            invokeOnCurrentTab("aboutToBeShown");
        }

        /**
         * If there is a current tab and the controls have been created, refresh
         * all sections on the page.
         */
        public void refresh() {
            invokeOnCurrentTab("refresh");
        }

        /**
         * Invoke the specified method through reflection.
         *
         * @param methodName
         *            the method to invoke.
         */
        private void invokeOnCurrentTab(final String methodName) {
            try {
                /*
                 * get the current tab first.
                 */
                final Method getCurrentTab = page.getClass().getMethod("getCurrentTab"); //$NON-NLS-1$
                final Object tab = getCurrentTab.invoke(page);

                if (tab != null) {
                    Method invokeTarget = tab.getClass().getMethod(methodName);
                    invokeTarget.invoke(tab);
                }
                // else ignore : there is not a current tab

            } catch (Throwable ex) {
                /*
                 * Nothing we can do at this point. Just ignore the exception.
                 */
                final String msg = String.format(
                        "Failed to call %s method on the current tabbed proeprty sheet pag.",
                        methodName);

                Activator.getDefault().getLogHelper().logError(msg, ex);
            }

        }

    }

    /**
     * Constructor for the UIElementTabbedPropertySectionPage class.
     *
     * @param contributor
     *            the contributor.
     */
    public UIElementTabbedPropertySheetPage(final ITabbedPropertySheetPageContributor contributor) {
        super(contributor);
    }

    /**
     * Notifies the current tab that it is about to be shown.
     */
    public void handlePropertySheetPageActivated() {
        getCurrentTabCompatible().aboutToBeShown();
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public void refresh() {
        /*
         * extended to check for null when getting the current tab.
         */
        getCurrentTabCompatible().refresh();
    }

    /**
     * A replacement of the getCurrentTab() method in the
     * TabbedPropertySheetPage that is compatible with both Eclipse 3.3 and 3.4.
     *
     * @return a wrapper around Tab or TabContents depending on which eclipse
     *         version is running.
     */
    public TabContentWrapper getCurrentTabCompatible() {
        return new TabContentWrapper(this);
    }
}