Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically select first created CTabItem in a CTabFolder #46

Open
Torbjorn-Svensson opened this issue Apr 12, 2022 · 22 comments · Fixed by #1044 or eclipse-platform/eclipse.platform#1346
Assignees
Labels
bug Something isn't working

Comments

@Torbjorn-Svensson
Copy link
Contributor

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:
image

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
Copy link
Member

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 ?

@niraj-modi niraj-modi added the bug Something isn't working label Apr 14, 2022
@Torbjorn-Svensson
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

deepika-u commented Feb 8, 2024

Without the fix =>
image

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

With the pr 1044
image

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
Copy link
Member

With the pr 1044
image

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.

deepika-u added a commit to deepika-u/eclipse.platform.swt that referenced this issue Feb 20, 2024
deepika-u added a commit to deepika-u/eclipse.platform.swt that referenced this issue Feb 20, 2024
deepika-u added a commit to deepika-u/eclipse.platform.swt that referenced this issue Feb 20, 2024
deepika-u added a commit to deepika-u/eclipse.platform.swt that referenced this issue Feb 27, 2024
deepika-u added a commit to deepika-u/eclipse.platform.swt that referenced this issue Feb 27, 2024
deepika-u added a commit to deepika-u/eclipse.platform.swt that referenced this issue Feb 27, 2024
deepika-u added a commit to deepika-u/eclipse.platform.swt that referenced this issue Feb 27, 2024
@lshanmug
Copy link
Member

lshanmug commented Mar 7, 2024

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.

@lshanmug lshanmug reopened this Mar 7, 2024
@deepika-u
Copy link
Contributor

deepika-u commented Mar 8, 2024

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
Copy link
Member

niraj-modi commented Mar 8, 2024

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 (eclipse-platform/eclipse.platform#1239 and https://github.com/eclipse-platform/eclipse.platform/issues/1736).

@laeubi
Copy link
Contributor

laeubi commented Mar 8, 2024

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
Copy link
Contributor

merks commented Mar 8, 2024

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

@deepika-u
Copy link
Contributor

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
Copy link
Member

iloveeclipse commented Mar 8, 2024

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

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
Copy link
Contributor

As a first step, eclipse-platform/eclipse.platform.ui#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
Copy link
Contributor

I have further investigated on eclipse-platform/eclipse.platform#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)) {

fedejeanne added a commit to fedejeanne/eclipse.platform that referenced this issue Apr 10, 2024
Add some tests to check that no unnecessary de/activation of tabs is
happening and add some assertions to existing tests too. These
improvements are necessary in order to guarantee that fixing
eclipse-platform/eclipse.platform.swt#46 do
not change the existing behavior of LaunchConfigurationTabGroupViewer.

Contributes to
eclipse-platform#859
Contributes to
eclipse-platform/eclipse.platform.swt#46
fedejeanne added a commit to fedejeanne/eclipse.platform that referenced this issue Apr 10, 2024
Add some tests to check that no unnecessary de/activation of tabs is
happening and add some assertions to existing tests too. These
improvements are necessary in order to guarantee that fixing
eclipse-platform/eclipse.platform.swt#46 do
not change the existing behavior of LaunchConfigurationTabGroupViewer.

Contributes to
eclipse-platform#859
Contributes to
eclipse-platform/eclipse.platform.swt#46
fedejeanne added a commit to eclipse-platform/eclipse.platform that referenced this issue Apr 10, 2024
Add some tests to check that no unnecessary de/activation of tabs is
happening and add some assertions to existing tests too. These
improvements are necessary in order to guarantee that fixing
eclipse-platform/eclipse.platform.swt#46 do
not change the existing behavior of LaunchConfigurationTabGroupViewer.

Contributes to
#859
Contributes to
eclipse-platform/eclipse.platform.swt#46
@deepika-u
Copy link
Contributor

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
Copy link
Member

#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 49c04a7 fixes the regression.

image

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 iloveeclipse reopened this May 13, 2024
@iloveeclipse
Copy link
Member

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

@iloveeclipse
Copy link
Member

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 added a commit to iloveeclipse/eclipse.platform.swt that referenced this issue May 14, 2024
This reverts commit 49c04a7 because it
introduces regressions in client code.

See eclipse-platform#46 (comment)
@iloveeclipse
Copy link
Member

iloveeclipse commented May 14, 2024

@deepika-u : I plan to merge #1227 on this Thursday for 4.32 M3.

@deepika-u
Copy link
Contributor

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(eclipse-platform/eclipse.platform#1298) and UI with #1346(eclipse-platform/eclipse.platform#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 (#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
Copy link
Contributor

fedejeanne commented May 15, 2024

@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 #1164 (#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 eclipse-platform/eclipse.platform#1298 or eclipse-platform/eclipse.platform#1346, they work with or without #1164 ✔️

@iloveeclipse go ahead with #1227 🚀

iloveeclipse added a commit that referenced this issue May 15, 2024
This reverts commit 49c04a7 because it
introduces regressions in client code.

See #46 (comment)
@iloveeclipse
Copy link
Member

go ahead with #1227

Done. Thanks for looking into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment