Automatically select first created CTabItem in a CTabFolder

[Torbjorn-Svensson]

I originally reported this on https://bugs.eclipse.org/bugs/show_bug.cgi?id=579665, but then I got the information that you likely check github issues more often than the bugzilla entries. Feel free to close either one as a duplicate...

After creating one or several o.e.swt.custom.CTabItem for a o.e.swt.custom.CTabFolder, no tab will be selected by default and thus, the content will be "blank". This is a difference to the behavior of o.e.swt.widget.TabFolder where the first created o.e.swt.widget.TabItem will be selected by default.

Is there any reason why the first CTabItem is not selected by default? The user of the class can still override if needed...

The below snippet is based on Snippet76.java:

/*******************************************************************************
 * Copyright (c) 2000, 2011 IBM Corporation and others.
 *
 * This program and the accompanying materials
 * are made available under the terms of the Eclipse Public License 2.0
 * which accompanies this distribution, and is available at
 * https://www.eclipse.org/legal/epl-2.0/
 *
 * SPDX-License-Identifier: EPL-2.0
 *
 * Contributors:
 *     IBM Corporation - initial API and implementation
 *******************************************************************************/
package org.eclipse.swt.snippets;

/*
 * TabFolder example snippet: create a tab folder (six pages)
 *
 * For a list of all SWT example snippets see
 * http://www.eclipse.org/swt/snippets/
 */
import org.eclipse.swt.SWT;
import org.eclipse.swt.custom.CTabFolder;
import org.eclipse.swt.custom.CTabItem;
import org.eclipse.swt.layout.GridData;
import org.eclipse.swt.layout.GridLayout;
import org.eclipse.swt.widgets.Button;
import org.eclipse.swt.widgets.Composite;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Shell;
import org.eclipse.swt.widgets.TabFolder;
import org.eclipse.swt.widgets.TabItem;

public class Snippet76 {
    public static void main (String [] args) {
        Display display = new Display ();
        final Shell shell = new Shell (display);
        shell.setText("Snippet 76");
        shell.setLayout(new GridLayout());

        createTabFolder(shell);
        createCTabFolder(shell);

        shell.setSize(400, 400);
        shell.open ();
        while (!shell.isDisposed ()) {
            if (!display.readAndDispatch ()) display.sleep ();
        }
        display.dispose ();
    }

    private static void createTabFolder(Composite parent) {
        TabFolder tabFolder = new TabFolder (parent, SWT.NONE);
        tabFolder.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true, 1, 1));
        tabFolder.setLayout(new GridLayout());
        for (int i=0; i<6; i++) {
            TabItem item = new TabItem (tabFolder, SWT.NONE);
            item.setText ("TabItem " + i);
            Button button = new Button (tabFolder, SWT.PUSH);
            button.setText ("Page " + i);
            item.setControl (button);
        }
    }

    private static void createCTabFolder(Composite parent) {
        CTabFolder tabFolder = new CTabFolder (parent, SWT.NONE);
        tabFolder.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true, 1, 1));
        tabFolder.setLayout(new GridLayout());
        for (int i=0; i<6; i++) {
            CTabItem item = new CTabItem (tabFolder, SWT.NONE);
            item.setText ("CTabItem " + i);
            Button button = new Button (tabFolder, SWT.PUSH);
            button.setText ("Page " + i);
            item.setControl (button);
        }
    }
}

This is how it will be rendered on Windows 10:

The problem does not appear if "Shell#setSize(int,int)" is replaced by "Shell#pack()". I first saw the issue in a view inside the Eclipse IDE, so I suppose it's the setSize variant that is generally used there and not the pack variant.

[niraj-modi]

The problem does not appear if "Shell#setSize(int,int)" is replaced by "Shell#pack()". I first saw the issue in a view inside the Eclipse IDE, so I suppose it's the setSize variant that is generally used there and not the pack variant.

Thanks for the SWT test snippet. For our reference: please let us know which view in Eclipse IDE did you notice this issue ?

[Torbjorn-Svensson]

For our reference: please let us know which view in Eclipse IDE did you notice this issue ?

It was with a custom view in our own product. What I meant by "Eclipse IDE" was that is was not in a snippet or similar simple application. Sorry for the bad wording! The reason I noticed the problem is that I've been trying to address dark theme issues in both our internal plugins and in plugins provided in our target platform.

[deepika-u]

I have investigated this issue further and by adding the below line, one can make 1st CTabItem become selected from user program. ctabFolder.setSelection(0);

But the concern here is by default it should be selected without this line in user program in comparison with behavior of TabItem, so got this working with proposed change in my new pr #1044

Please share me any inputs if there might be any problems with this fix?

[deepika-u]

Without the fix =>

With this liner in the above extended Snippet76 ctabFolder.setSelection(0);

With the pr 1044

with the pr i am seeing the tab is not differentiable if selected or not though the content in the page is correct. Is this the same others are also seeing? If seeing what i need to check further for this? any inputs are please welcome.

[niraj-modi]

With the pr 1044

with the pr i am seeing the tab is not differentiable if selected or not though the content in the page is correct. Is this the same others are also seeing?

@deepika-u TabFolder content looks right now, but need to improve on below:

• Tab header should show the right selection.
• Also good to cross validate the behavior on other platforms and self hosted Eclipse as well.

[lshanmug]

Reverted fix due to failures in platform ui (https://github.com/eclipse-platform/eclipse.platform/issues/1239 and https://github.com/eclipse-platform/eclipse.platform/issues/1736). @deepika-u Please take a look.

[deepika-u]

Reverted fix due to failures in platform ui (eclipse-platform/eclipse.platform#1239 and https://github.com/eclipse-platform/eclipse.platform/issues/1736). @deepika-u Please take a look.

checking on them, able to recreate 1736. Checking further.

[niraj-modi]

Reverted fix due to failures in platform ui (eclipse-platform/eclipse.platform#1239 and https://github.com/eclipse-platform/eclipse.platform/issues/1736). @deepika-u Please take a look.

checking on them, able to recreate 1736. Checking further.

@deepika-u, Try to come up with a fix for this issue that doesn't break other cases, as reported in (https://github.com/eclipse-platform/eclipse.platform/issues/1239 and https://github.com/eclipse-platform/eclipse.platform/issues/1736).

[laeubi]

I'm not sure the failures are maybe just wrong assumptions, and this PR changes the assumption. e.g. the one failing (other link give 404 error for me) it says it testFirstTabIsActivatedByDefault, so maybe just that code now does not activate because it already is and should in this case create a syntetic "activate" event.

[merks]

Indeed it might well be the case that the test needs to change its assumption.

[deepika-u]

When my swt pr #1044 is in place, for the eclipse.platform.ui issue #1736 reported error.

When i select this "MultiVariablePageTest.java" file's testContextActivation and run as "JUnit Plug-in Test"

failing case: line 210 checkActiveContext(globalService, ContextTextEditor.TEXT_CONTEXT_ID, true);

passing case: Now when i change that line as checkActiveContext(globalService, ContextTextEditor.TEXT_CONTEXT_ID, false);

I then tried running the complete file "MultiVariablePageTest.java" as "JUnit Plug-in Test", all the 9 tests passed.

I hope i am doing the right fix. Please confirm.

[iloveeclipse]

@deepika-u : there are two different test cases failing:

• https://github.com/eclipse-platform/eclipse.platform/issues/1239
• https://github.com/eclipse-platform/eclipse.platform.ui/issues/1736

What you should do is (for each test failing)

1) Understand which use case test is validating 2) Check if the use case (== production code) that is tested is still working with your patch 3) If the use case is not working (== regression in production code), add a note here and try to understand why it is not working 4) If the use case is working, but the test makes wrong assumptions, add a note here and try to fix the test

[deepika-u]

As a first step, https://github.com/eclipse-platform/eclipse.platform.ui/issues/1736

I have further investigated and i see that this block of code needs a change. https://github.com/eclipse-platform/eclipse.platform.ui/blob/2673dde18f02301aeaa9b56dbd7ddc990d606067/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/part/MultiPageEditorPart.java#L329-L348

Originally the code around these lines are

        if (getActivePage() == -1) {
            setActivePage(0);
            IEditorPart part = getEditor(0);
            if (part != null) {
                final IServiceLocator serviceLocator = part.getEditorSite();
                if (serviceLocator instanceof INestable) {
                    activeServiceLocator = (INestable) serviceLocator;
                    activeServiceLocator.activate();
                }
            }
        }

Without my #1044 change, there is no CTabItem selected, so getActivePage() returns -1 as a result the above block of code gets executed(activeServiceLocator.activate() is being called).

With my #1044 pr, there is already CTabItem[0] being activated as default page. So getActivePage() returns zero as a result the block is never executed(activeServiceLocator.activate() is being never called).

On further investigation, this can be updated as below to accommodate my pr.

        if (getActivePage() == -1) {
            setActivePage(0);
        }           
                IEditorPart part = getEditor(0);
        if (part != null) {
            final IServiceLocator serviceLocator = part.getEditorSite();
            if (serviceLocator instanceof INestable) {
                activeServiceLocator = (INestable) serviceLocator;
                activeServiceLocator.activate();
            }
        }

Is it ok to fix it in MultiPageEditorPart.java which is in platform.ui

By the way now the UI test case is passing. I am yet to investigate on other failure(#1239).

[deepika-u]

I have further investigated on https://github.com/eclipse-platform/eclipse.platform/issues/1239 and found the below. The test case assumption needs to be corrected here as below.

https://github.com/eclipse-platform/eclipse.platform/blob/e2facf9f24a4345594a89d435d4c75ef2c8acb84/debug/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/launchConfigurations/LaunchConfigurationTabGroupViewer.java#L1450-L1461

Before the SWT fix there is no tab active, so previousTabIndex has value "-1", fCurrentTabIndex will have zero when first tab needs to be activated. As a result the if condition at line 1458 fails and propagateTabActivation() is called.

Now after the SWT fix, first tab is activated by default(it means selectedIndex is zero). So previousTabIndex and fCurrentTabIndex both are having zero value. As a result the if condition at 1458 returns true and it returns from there without calling propagateTabActivation(). So first tab is not getting activated via the test case, so test case fails.

It means the test case assumes no tab is active so previousTabIndex and fCurrentTabIndex are never equal. Now that assumption is not valid anymore so the if condition at 1458 needs to be corrected in such a way that it fails and proceeds to call propagateTabActivation() which activates the first tab via the test case.

    if (previousTabIndex == fCurrentTabIndex || tabs == null || tabs.length == 0
            || previousTabIndex > (tabs.length - 1)) {

to

    if (tabs == null || tabs.length == 0
            || previousTabIndex > (tabs.length - 1)) {

[deepika-u]

Verified this issue and works as expected on below build.

Eclipse SDK Version: 2024-06 (4.32) Build id: I20240512-1800 OS: Windows 11, v.10.0, x86_64 / win32 Java vendor: Oracle Corporation Java runtime version: 22+36-2370 Java version: 22

[iloveeclipse]

https://github.com/eclipse-platform/eclipse.platform.swt/pull/1164 introduces regression for clients.

• Go to the History view of any git project.
• Right click any commit and select "Open in Commit Viewer"
• Empty gray area is shown. Clicking on a "Diff" tab and back to "Commit" tab shows expected content.
• Reverting 49c04a7f17f50cd667c20c6400099e1e5e06a717 fixes the regression.

Since that affects existing client code, it seem to be a breaking change. Please check & provide a fix or let revert it once again till better solution will be found.

[iloveeclipse]

Another multipage editor that doesn't work is target file editor. Open any .target file in the IDE and "enjoy the silence".

[iloveeclipse]

Note: 4.32 M3 contribution is planned for May 17 (this Friday) with the nightly build from the day before, so ideally the fix should be developed & merged before May 16. In case there will be no fix before May 16, we should revert the change on May 16 latest.

[iloveeclipse]

@deepika-u : I plan to merge https://github.com/eclipse-platform/eclipse.platform.swt/pull/1227 on this Thursday for 4.32 M3.

[deepika-u]

Hi @iloveeclipse , We have investigated further on this issue and we are also seeing the 2 issues that you have mentioned above. Thanks for the early catch.

Looks like this widget behavior of defaulting the index to -1(old behavior) is being used by lot of clients. When this behavior is -1 they are doing some additional initialization part. Now since that index is no more -1 and becoming zero that additional initialization is missing and that is causing the issue. So looks like they are basically dependent on the original behavior.

Right now we have fixed platform issue with #1298(https://github.com/eclipse-platform/eclipse.platform/pull/1298) and UI with #1346(https://github.com/eclipse-platform/eclipse.platform/pull/1346). Even if we fix GIT client and once the product is released, we might get lot of other clients(users using cTabFolder widget) who may also report multiple failures in the silimar lines.

Though we have tried to make consistent behavior between tabFolder(here the default index is zero) and cTabFolder(here the default index is -1), looks like this is indirectly changing the existing behavior which may not be acceptable.

Later if it is really needed to be fixed, we might have to come with approach like providing a flag option provided to customer whether to work like old behavior or behave like new behavior and then proceed accordingly.

@fedejeanne : If this pr #1164 (https://github.com/eclipse-platform/eclipse.platform.swt/pull/1164) is reverted, can the other 2 prs exist or do they also need to be reverted(#1298 and #1346). Can you please confirm?

Once fedejeanne confirms, then i think we can go ahead with merge of #1227(revert of #1164).

[fedejeanne]

@deepika-u thank you for the analysis. I made a quick analysis myself and reached the same conclusion: if the expected default behavior is changed then the client code needs to be changed too. I even tried making some changes in the superclasses of the involved editors but I saw that those changes may trigger a "double activation" in some (client) editors. Long story short: I couldn't find a solution for the current problem 👎

@fedejeanne : If this pr https://github.com/eclipse-platform/eclipse.platform.swt/pull/1164 (https://github.com/eclipse-platform/eclipse.platform.swt/pull/1164) is reverted, can the other 2 prs exist or do they also need to be reverted(#1298 and #1346). Can you please confirm?

No need to revert https://github.com/eclipse-platform/eclipse.platform/pull/1298 or https://github.com/eclipse-platform/eclipse.platform/pull/1346, they work with or without #1164 ✔️

@iloveeclipse go ahead with #1227 🚀

[iloveeclipse]

go ahead with #1227

Done. Thanks for looking into.