Skip to content

Control: remove dependency on Browser and OleClientSite #2182

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tmssngr
Copy link
Contributor

@tmssngr tmssngr commented May 24, 2025

This is better design and allows to compile SWT without the "bundles\org.eclipse.swt\Eclipse SWT Browser" source path.

@tmssngr
Copy link
Contributor Author

tmssngr commented May 24, 2025

Huh, my email still was fine for PR #2173 3 days ago.

Copy link
Contributor

github-actions bot commented May 24, 2025

Test Results

  218 files   -    321    218 suites   - 321   7m 12s ⏱️ - 22m 59s
4 361 tests ±     0  4 301 ✅  -     44  60 💤 +45  0 ❌  - 1 
4 508 runs   - 12 174  4 448 ✅  - 12 095  60 💤  - 78  0 ❌  - 1 

Results for commit dccc6c6. ± Comparison against base commit 925a294.

This pull request removes 37 and adds 37 tests. Note that renamed tests count towards both.
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_dollarSign
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_emptyString
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_letterA
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_letters
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16LE_null
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_AsciiLetters
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_Asciiletter
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_LotsOfLetters
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_letter
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_letters
…
AllWin32Tests org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageDataForDifferentFractionalZoomsShouldBeDifferent
AllWin32Tests org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageShouldHaveDimesionAsPerZoomLevel
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testByteArrayTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testFileTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testHtmlTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromCopiedImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageData
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageDataFromImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testRtfTransfer
…
This pull request removes 2 skipped tests and adds 3 skipped tests. Note that renamed tests count towards both.
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_Heuristic_specialSingleCases
org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_Heuristic_specialSingleCases
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_events_KeyEvent ‑ testEnglishUs_multipleLetters
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_events_KeyEvent ‑ testEnglishUs_unorderedCtrlC
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_events_KeyEvent ‑ testEnglishUs_unpairedKeyUp
This pull request skips 44 tests.
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_StatusTextListener_hoverMouseOverLink[browser flags: 0]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color[browser flags: 0]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_setTextContainingScript_applicationLayerProgressListenerMustSeeUpToDateDom[browser flags: 0]
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_CCombo ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_CLabel ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_CTabFolder ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_custom_StyledText ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Button ‑ test_setForegroundAlphaCheckButton
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Button ‑ test_setForegroundAlphaLorg_eclipse_swt_graphics_Color
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_Button ‑ test_setForegroundAlphaRadiokButton
…

♻️ This comment has been updated with latest results.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

Getting rid of the instanceof checks would of course be great. I do not remember why exactly we have implemented it this way. Probably it was related to the fact that browser is a common widget, which is also why the proposed change does not compile.
@tmssngr do you plan to fix this PR?

@tmssngr
Copy link
Contributor Author

tmssngr commented May 28, 2025

@HeikoKlare Thanks for taking a look. I don't actually understand what exactly is causing this PR to fail.

@HeikoKlare
Copy link
Contributor

You have added a protected method to one of the native control classes without adding it to the others. This leads to a compilation error as you override that method in a common control (Browser) without the method being present in the different native superclasses. This is what the logs say:

Error:  Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:4.0.13-SNAPSHOT:compile (default-compile) on project org.eclipse.swt.cocoa.macosx.x86_64: Compilation failure: Compilation failure: 
Error:  /home/runner/work/eclipse.platform.swt/eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT Browser/common/org/eclipse/swt/browser/Browser.java:[1272] 
Error:  	protected boolean embedsWin32Control () {
Error:  	                  ^^^^^^^^^^^^^^^^^^^^^
Error:  The method embedsWin32Control() of type Browser must override or implement a supertype method

@tmssngr tmssngr force-pushed the feature/control-remove-dependencies branch 2 times, most recently from 6513c6e to dccc6c6 Compare May 28, 2025 15:28
This allows to compile SWT without the "bundles\org.eclipse.swt\Eclipse SWT Browser\" source path.
@tmssngr
Copy link
Contributor Author

tmssngr commented May 28, 2025

Thanks. I still don't understand why my email address is rejected.

@HeikoKlare
Copy link
Contributor

Thanks. I still don't understand why my email address is rejected.

I don't understand the issue either. I guess you still have your GitHub user linked to your Eclipse account with signed ECA? There seem to be multiple Eclipse accounts for your name, so I don't know which one to validate.

Regarding the change: note that an embedsWin32Control method does not really make sense for a cross-OS class like Control, which is probably why we did not add such a method back then. So it would rather need to be a method with some OS-agnostic semantics, if necessary at all. In addition, adding a protected method is an API change, so at least an according @since tag would be required.

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.

2 participants