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

Use CTabFolder to improve dark theme on Windows #14

Merged
merged 1 commit into from
Apr 20, 2022

Conversation

Torbjorn-Svensson
Copy link
Contributor

Contributed by STMicroelectronics

Signed-off-by: Torbjörn Svensson torbjorn.svensson@st.com

tabItem.setText(page.getTitle());//
tabItem.setControl(createPageArea(tabFolder, page));
pageMap.put(tabItem, page);
}
tabFolder.setSelection(0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With eclipse-platform/eclipse.platform.swt#46 fixed, this line would not be needed.

folder.setLayout(new TabFolderLayout());
folder.setLayoutData(new GridData(GridData.FILL_BOTH));

TabItem item= new TabItem(folder, SWT.NONE);
CTabItem item= new CTabItem(folder, SWT.NONE);
folder.setSelection(0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With eclipse-platform/eclipse.platform.swt#46 fixed, this line would not be needed.

@Torbjorn-Svensson Torbjorn-Svensson force-pushed the pr/ctabfolder branch 2 times, most recently from fd1112b to 2a96fb4 Compare April 16, 2022 17:34
Contributed by STMicroelectronics

Signed-off-by: Torbjörn Svensson <torbjorn.svensson@st.com>
@vogella
Copy link
Contributor

vogella commented Apr 19, 2022

@Torbjorn-Svensson can you post before / after screenshots in this issue?

@Torbjorn-Svensson
Copy link
Contributor Author

@Torbjorn-Svensson can you post before / after screenshots in this issue?

Light theme

Before:
image

After:
image

Dark theme

Before:
image

After:
image

The preference dialog has similar change.

Copy link
Member

@niraj-modi niraj-modi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good, should improve dark-theme Eclipse.

@niraj-modi niraj-modi merged commit 3b04269 into eclipse-platform:master Apr 20, 2022
@Torbjorn-Svensson Torbjorn-Svensson deleted the pr/ctabfolder branch April 20, 2022 15:56
@iloveeclipse
Copy link
Member

iloveeclipse commented Apr 21, 2022

This change caused 3 test fails, please adopt either code or tests: eclipse-platform/eclipse.platform.ui#30

@Torbjorn-Svensson
Copy link
Contributor Author

This change caused 3 test fails, please adopt either code or tests: eclipse-platform/eclipse.platform.ui#30

With only the output below to go on, it's a bit too hard for me to know what's going on.

Warning: Button {>} Actual Width -> 0 Recommended Width -> 34

java.lang.AssertionError: Warning: Button {>}
Actual Width -> 0
Recommended Width -> 34
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.eclipse.ui.tests.harness.util.DialogCheck.verifyButtonText(DialogCheck.java:166)
at org.eclipse.ui.tests.harness.util.DialogCheck.verifyCompositeText(DialogCheck.java:128)
at org.eclipse.ui.tests.harness.util.DialogCheck.verifyCompositeText(DialogCheck.java:136)
at org.eclipse.ui.tests.harness.util.DialogCheck.verifyCompositeText(DialogCheck.java:136)
at org.eclipse.ui.tests.harness.util.DialogCheck.verifyCompositeText(DialogCheck.java:136)
at org.eclipse.ui.tests.harness.util.DialogCheck.verifyCompositeText(DialogCheck.java:136)
at org.eclipse.ui.tests.harness.util.DialogCheck.verifyCompositeText(DialogCheck.java:136)
at org.eclipse.ui.tests.harness.util.DialogCheck.verifyCompositeText(DialogCheck.java:136)
at org.eclipse.ui.tests.harness.util.DialogCheck.verifyCompositeText(DialogCheck.java:136)
at org.eclipse.ui.tests.harness.util.DialogCheck.verifyCompositeText(DialogCheck.java:136)
at org.eclipse.ui.tests.harness.util.DialogCheck.verifyCompositeText(DialogCheck.java:136)
at org.eclipse.ui.tests.harness.util.DialogCheck.verifyCompositeText(DialogCheck.java:136)
at org.eclipse.ui.tests.harness.util.DialogCheck.verifyCompositeText(DialogCheck.java:136)
at org.eclipse.ui.tests.harness.util.DialogCheck.assertDialogTexts(DialogCheck.java:89)
at org.eclipse.ui.tests.compare.UIComparePreferencesAuto.testCompareViewersPref(UIComparePreferencesAuto.java:59) 

I've spent the past 4h trying to get a runtime for the platform running (just so I could debug the UIComparePreferencesAuto unit-test. I see that there is another class called UIComparePreferences that requires some input from the user. I can run the UIComparePreferences class and it passes, but UIComparePreferencesAuto fails in my runtime with another error:

java.lang.NullPointerException: Cannot invoke "org.eclipse.e4.core.contexts.IEclipseContext.get(java.lang.Class)" because "this.e4Context" is null
	at org.eclipse.ui.internal.WorkbenchPlugin.getPreferenceManager(WorkbenchPlugin.java:575)
	at org.eclipse.ui.tests.compare.UIComparePreferencesAuto.getPreferenceDialog(UIComparePreferencesAuto.java:37)
	at org.eclipse.ui.tests.compare.UIComparePreferencesAuto.testCompareViewersPref(UIComparePreferencesAuto.java:56)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
	at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:93)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:40)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:529)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:756)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:452)
	at org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner.main(RemotePluginTestRunner.java:74)
	at org.eclipse.pde.internal.junit.runtime.CoreTestApplication.start(CoreTestApplication.java:28)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:203)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:136)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:402)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:255)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:659)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:596)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1467)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1440)

And yes, I'm running it as a "JUnit Plug-in Test" in Eclipse.

What I can see is that the method DialogCheck#verifyCompositeText(Composite) might need a copy of the TabFolder block that does the same thing, but with the CTabFolder type instead. As I can't run the unit-test, I don't know if this will make a difference or what button has the label ">". If I manually look in the preference dialog, I do not see any button that has this string.

@iloveeclipse: If you have a working runtime, can update DialogCheck#verifyCompositeText(Composite) to look like this and see if it helps with the UIComparePreferencesAuto test case? I think the other 2 failures are unrelated to the UI changes.

	private static void verifyCompositeText(Composite composite) {
		Control children[] = composite.getChildren();
		for (Control child : children) {
			if (child instanceof TabFolder) {
				TabFolder folder = (TabFolder) child;
				int numPages = folder.getItemCount();
				for (int j = 0; j < numPages; j++) {
					folder.setSelection(j);
				}
			}
			else if (child instanceof CTabFolder) {
				CTabFolder folder = (CTabFolder) child;
				int numPages = folder.getItemCount();
				for (int j = 0; j < numPages; j++) {
					folder.setSelection(j);
				}
			}
			else if (child instanceof Button) {
				//verify the text if the child is a button
				verifyButtonText((Button) child);
			}
			else if (child instanceof Label) {
				//child is not a button, maybe a label
				verifyLabelText((Label) child);
			}
			else if (child instanceof Composite) {
				//child is not a label, make a recursive call if it is a composite
				verifyCompositeText((Composite) child);
			}
		}
	}

jonahgraham added a commit to jonahgraham/eclipse.platform.ui that referenced this pull request Apr 22, 2022
The method verifyCompositeText in DialogCheck verifies various aspects
of composites, with
eclipse-platform/eclipse.platform.team#14 this
now needs to understand CTabFolder (in addition to TabFolder)

Also-by: Torbjörn Svensson <torbjorn.svensson@st.com>
jonahgraham added a commit to jonahgraham/eclipse.platform.ui that referenced this pull request Apr 22, 2022
The method verifyCompositeText in DialogCheck verifies various aspects
of composites, with
eclipse-platform/eclipse.platform.team#14 this
now needs to understand CTabFolder (in addition to TabFolder)

Also-by: Torbjörn Svensson <torbjorn.svensson@st.com>
vogella pushed a commit to eclipse-platform/eclipse.platform.ui that referenced this pull request Apr 22, 2022
The method verifyCompositeText in DialogCheck verifies various aspects
of composites, with
eclipse-platform/eclipse.platform.team#14 this
now needs to understand CTabFolder (in addition to TabFolder)

Also-by: Torbjörn Svensson <torbjorn.svensson@st.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants