Adding a recommendation for the usage of setLocation in Control

[ShahzaibIbrahim]

As mentioned in eclipse-platform/eclipse.platform.ui#2948 (review), setting the location of the control before setting the size could cause problems with HiDpi settings. In order to prevent that we are adding a recommendation in java doc for setLocation method of Control.

Usages as per non-recommended way to be changed in Platform repositories in separate issue: vi-eclipse/Eclipse-Platform#304

[github-actions]

Test Results

   539 files   -  6     539 suites   - 6   41m 29s ⏱️ + 5m 55s
 4 340 tests  - 37   4 324 ✅  - 35   15 💤  - 3  1 ❌ +1 
16 610 runs   - 37  16 471 ✅  - 35  138 💤  - 3  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 200934c. ± Comparison against base commit 63048a2.

This pull request removes 37 tests.

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
…

[mickaelistria]

I'm not sure adding such recommendation in doc/spec is a good thing. This seems like documenting and specifying a bug instead of attempting to fix it.

[laeubi]

As mentioned in eclipse-platform/eclipse.platform.ui#2948 (review), setting the location of the control before setting the size could cause problems with HiDpi settings.

I don't think that a particular order is something one could expect. It would be better when the control will take care (e.g. setting the position will also update the size (again)).

[HeikoKlare]

I'm not sure adding such recommendation in doc/spec is a good thing. This seems like documenting and specifying a bug instead of attempting to fix it.

Yes, it basically points to a "bug" of how the points coordinate system in SWT works. The coordinate system is by design incapable of properly supporting monitor-specific zooms like we have implemented for Windows. To "properly" support this, one would have to either completely exchange SWT autoscaling or to revise essential parts of the API (in particular using enhanced Rectangles and Points to properly carry monitor-related information). Since this is all rather infeasible (high effort for consumers to adapt), we have instead encapsulated as much as possible in SWT. By now, this seems like doing so and falling back to heuristics in seldom cases works quite fine. Still, at some places the necessary monitor information may be lost or not available (such as recreating Points/Rectangles used for positioning shells). One such situation we have seen is eclipse-platform/eclipse.platform.ui#2948, which leads to this recommendation. Note that following the recommendation is not "crucial" but may lead to undesired effects in the UI. We can also leave it out as we are not aware of any other situations where this has caused problems so far, still it may not hurt to have it documented.

[laeubi]

Setting size/position of Controls is what LayoutMangers do all the time, we can't put restrictions on them in the order of these operations to fix flaws in the SWT design as these are public API. For the same reason I don't think any "recommendations" are sufficient or helpful. This will simply lead to endless regressions as we can't reliable control such code. Especially as all of this only applies to windows as far as I can see.

I also don't see why order should matter here. If we detect that setting the location or size of the control is performed, one should be able to recompute maybe both values or one value base on the old/new values of the other.

[laeubi]

One such situation we have seen is eclipse-platform/eclipse.platform.ui#2948, which leads to this recommendation.

I think this actually shows the flaw in event handling I previously mentioned with using special indirection maps with callback instead of proper event handling in the control itself. That way it would be possible to inhibit a DPI change event for the time of a call, like we do on higher level elsewhere e.g. if updating viewers and so on.

[HeikoKlare]

I just saw that the recommendation here is added to the documentation of Control. This is obviously incorrect. It does not affect any Shell-internal layouting. It is only relevant for Shell.

That way it would be possible to inhibit a DPI change event for the time of a call, like we do on higher level elsewhere e.g. if updating viewers and so on.

If we find a generic way of doing so properly, that would of course be fine. But you cannot just disable for it for, e.g., setSize() calls as you will usually want the DPI change event for resizing to be processed properly.

[laeubi]

It is only relevant for Shell.

That would make it less serve, still I think we cannot enforce/recommend that really. Especially if this recommendation should "be changed in Platform repositories" it shows that it actually is not a recommendation but requirement. So we would possible need to change everything else that uses a Shell and position it! That seems not acceptable.

If we really have a hard requirement here, then better one would have s Shell#setBounds(Rectangle bounds) that includes position and size.

If we find a generic way of doing so properly, that would of course be fine. But you cannot just disable for it for, e.g., setSize() calls as you will usually want the DPI change event for resizing to be processed properly.

That's another issue here, the recommendation does not explain why and when it happen, and its also unclear for me, actually I would have assumed that setting the position before the size would be better, because then you know the final monitor. So something really feels flawed here and one should better describe why it fails instead of recommend something that feels like a workaround.

[HeikoKlare]

If we really have a hard requirement here, then better one would have s Shell#setBounds(Rectangle bounds) that includes position and size.

Yeah, we discussed that as well. Do you remember why we dropped it, @akoch-yatta @amartya4256?

That's another issue here, the recommendation does not explain why and when it happen, and its also unclear for me, actually I would have assumed that setting the position before the size would be better, because then you know the final monitor.

Agreed, the current formulation is definitely imprecise. The reason is that a shell is assigned to a monitor by having more than 50% of it's size on it. If setSize() makes the shell span to another monitor even though it will be located somewhere else later on, the monitor it becomes assigned to (leading to the DPI changed event, such that the location coordinates are then transformed according to a wrong monitor zoom) is wrong. We only faced this situation upon shell creation where some OS-dependent default size and location is used.

So something really feels flawed here and one should better describe why it fails instead of recommend something that feels like a workaround.

Yes, as I said, the coordinate system is by design limited to one zoom value. Extending it to monitor-specific zooms is conceptually impossible in a completely consistent way, which, you are right, can be considered a flaw.
The question is: what would be the alternative? Checking for setBounds() again is definitely one (as I don't remember the reasons against it).

[laeubi]

The question is: what would be the alternative?

I think one must describe the precise conditions under when it causes an issue and if it can be detected.

You wrote

We only faced this situation upon shell creation where some OS-dependent default size and location is used.

So that a Shell is created can be detected quite easy (its when running the constructor). The having a boolean flag (e.g. usingDefaults) can tell us the user has never explicitly called setSize(...) / setLocation(...).

Now assume I call setPosition() and usingDefaults is true one can do whatever is needed e.g. calling some API to get the "real" monitor, or set another flag that the next setSize class must do something special, or set a dummy size first (maybe even offscreen?) or first calculate the size in pixel and after setLoaction set it again ... so all such kinds are of course probably messy, but prevent us from odd behaviors when users are calling (either indirectly or directly) in the "wrong" order.

Checking for setBounds() again is definitely one (as I don't remember the reasons against it).

I think this is completely orthogonal here, and would be useful anyways as I assume in 90% of the time one wants to use either pack() or setSize() and position the shell somehow.