Design improvement for scaling win32 widgets

[amartya4256]

This PR improves the design implementation for widgets in win32 by moving away from DPIZoomChangeRegistry to an event-driven design using ZoomChanged event and inheritance.

[github-actions]

Test Results

  118 files  ±0    118 suites  ±0   11m 17s ⏱️ +19s
4 439 tests ±0  4 417 ✅  - 5  17 💤 ±0  5 ❌ +5 
  298 runs  ±0    289 ✅  - 5   4 💤 ±0  5 ❌ +5 

For more details on these failures, see this check.

Results for commit c1359f9. ± Comparison against base commit 8444ec7.

♻️ This comment has been updated with latest results.

[HeikoKlare]

I am a bit confused by this change, as it introduces a mixture of event processing and direct method calls for DPI change processing. I expected the following order of changes according to what we have discussed:

1. Replace CommonWidgetsDPIChangeHandler: use listeners instead
2. Replace DPIZoomChangeRegistry: make direct calls of handleDPIChange for the native controls instead
3. Replace direct handleDPIChange calls: use listeners instead
4. Make the listener calls asynchronous (may be combined with the previous step)

This change seems to be a mixture of the first three. Can we please split that up and make the changes in a more incremental, comprehensive way?

[amartya4256]

@HeikoKlare I think we have mixed up things. With this PR we just wanted to move to Event driven architecture and not make it asynchronous. I understand that both the terms go hand in hand but we must note that the event model in SWT is synchronous and if an event is sent by a caller, at first the stub is executed and the caller waits for the event to be processed and then moves forward. Hence, this PR doesn't implement asynchrony. That's also the reason why I called notifyListeners instead of calling handleDpiChange because every widget registers to a listener then they must receive the DPI_CHANGE event also through listeners and not by some other widget calling handleDPIChange on them. The follow up PR would implement asynchrony while calling notifyListeners by the following:

void sendZoomChangedEvent(Event event, Shell shell) {
	AtomicInteger handleDPIChangedScheduledTasksCount = (AtomicInteger) event.data;
	handleDPIChangedScheduledTasksCount.incrementAndGet();
	getDisplay().asyncExec(() -> {
		try {
			if(!this.isDisposed()) {
				notifyListeners(SWT.ZoomChanged, event);
			}
		} finally {
			if (handleDPIChangedScheduledTasksCount.decrementAndGet() <= 0) {
				shell.WM_SIZE(0, 0);
			}
		}
	});
}

and notifyListeners will be replaced by this method.

[amartya4256]

So from what you are proposing and what I think how we could do it, I devised the following order:

1. Remove DPIZoomChangeHandler and use direct handleDPIChange calls for internal classes and use ZoomChangedEvent for Common widget.
2. Move to Event driven model
3. Make the event driven model asynchronous.

(The only problem I see is in the first step. If we handle ZoomChangedEvent for Common widgets like StyledText, etc, they are not scaled in the hierarchical order. I need to test if it could cause any trouble.)

[HeikoKlare]

Sounds good, I would just try to split the first step into the adaptation of common/custom widgets and the adaptation of the native part to keep it as small as possible.

(The only problem I see is in the first step. If we handle ZoomChangedEvent for Common widgets like StyledText, etc, they are not scaled in the hierarchical order. I need to test if it could cause any trouble.)

But since everything stays synchronous, it should not matter whether you call a registry or whether you send an event.

[amartya4256]

@HeikoKlare As per our discussion, Replacing CommonWidgetZoomChangeHandler with ZoomChanged Event handler is not possible since the Common widgets need to call for the handleDPIChange for their associated widgets which is win32 specific code and we cannot call notifyListeners on these widgets without making them register to ZoomChanged event as they still use DPIZoomChangeRegistry. Hence, the following is necessary to make it work all together:

1. Register Widget with ZoomChanged event.
2. Implement inheritance based super.handleDPIChange for widgets to mimic DPIZoomChangeRegistry's heirarchial behaviour.
3. For handling DPI Change for associated / children widgets, replace DPIZoomChangedRegistry.applyChange with widget.notifyListeners (This still executes synchronously).
4. Move the CommonWidgetZoomChangeHandler methods to the Common Widgets and register them on ZoomChanged events.
5. Remove CommonWidgetZoomChangeHandler & DPIZoomChangeRegistry/Handler.
6. Adapt the DPI_CHANGE event trigger point in Control to use Shell.notifyListeners

For the next step, i.e. asynchrony, we will replace these notifyListeners with a Widget:sendZoomChangedEvent which will call widget.notifyListener asyncly and handle the painting upon DPI scaling completion.

[fedejeanne]

It looks OK to me but there are still some unresolved discussions with @HeikoKlare .

Other than that, it's fine ✔️

[akoch-yatta]

Code changes are mostly converting the static DPI change callbacks into class methods. The changes look good to me and besides my comments, that are already updated in the current state, I didn't find anything else. I tested with and without monitor specific scaling (latest should not be affected by these changes) and didn't find any regression.

[HeikoKlare]

I have some minor comments and a general design question regarding mutability of the used event object.
In addition, I was wondering if all controls on which we call notifyListeners() now are guaranteed to not be disposed. In many cases we check for not being null and probably the composite structures should not return elements that are disposed, but I would just like to know that we have checked that.

[HeikoKlare]

I wonder if we need to clone the event at some place, because otherwise if any event consumer changes the detail value, it will break the complete rescaling mechanism, won't it?

[amartya4256]

That's why we do not clone event at all but pass the same event down the tree - I wish we could make it final so that no one could modify it. Or maybe use a getter instead for cleaner access.

[HeikoKlare]

What do you mean by ... ?

That's why we do not clone event at all but pass the same event down the tree

So it is required that a consumer modifies the detail field?

[amartya4256]

No, The consumer only consumes (reads) the detail field and then passes the same event instance to its children. Only producer of the event (Shell) sets the detail to the new zoom while creating the event.

[HeikoKlare]

The consumer only consumes (reads) the detail field

How can you ensure that? Anyone can register a listener and modify the event. Take this snippet where a listener to a zoom change on a label modifies the scaling of another label (you can imagine the potential consequences in more complex UIs):

	public static void main(String[] args) {
		System.setProperty("swt.autoScale.updateOnRuntime", "true");
		Display display = new Display();
		Shell shell = new Shell(display);
		shell.setText("Snippet 1");
		shell.setLayout(new FillLayout());
		Label label1 = new Label(shell, SWT.None);
		label1.setText("Hello World");
		label1.addListener(SWT.ZoomChanged, e -> {
			e.detail = 250;
		});
		Label label2 = new Label(shell, SWT.None);
		label2.setText("Hello World");

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

Started on 100% monitor you get:

And moved to 150% monitor you get:

[amartya4256]

I agree with @laeubi extending to that I think creating a copy every time we send the event down the widget tree wouldn't have any functional benefits given the design of SWT events.

[HeikoKlare]

As said before, we can agree that it's fine to not copy the event and accept the risk of modifications by the consumer, in particular given that other places do the same.

Still this conclusion seems to be based on inconsistent understanding of current design and data flows, so we should clarify that first:

creating a copy every time we send the event down the widget tree wouldn't have any functional benefits given the design of SWT events.

I do not understand that point. If we copy the event, modifications done by consumers will not affect other consumers (except the ones the modified event is explicitly forwarded to). If you take the snippet I provided, the shell would copy the event for each of the labels it passes it to. So as a result, the second label gets a copy of the original event rather than the modified one of the first label. That is a functional benefit.
As said, we don't have to conclude out of that that we need to copy the event, but we need to clarify where the current inconsistent understanding is coming from.

[amartya4256]

That is right only if the consumers do not send zoomchanged events to other widgets. Consumers like CCombo and StyledText do that and if they were to modify the detail and send the event to other widgets, it will still be broken. The idea of having the same event sent to every widget was to keep the connotation correct. The event is produced at DPI Changed event by the shell and passed down to every widget. If we go with copying the events inside SWT and the consumers do not do it, it is still prone to errors and also it brings design inconsistency.

[laeubi]

I'm not completely sure what the concerns are.

1. We are talking about quite low level SWT Event and detail is marked as "for application use" so already quite generic
2. Actually each low level event should have a higher level "typed" Event that is used in user code
3. I just skimmed through both of them on most of them use public modifiable field and I find a few places where this modification is even used to communicate values to the caller or to even modify / "fix" values there so it is not unusual.
4. There are some exceptions where getters are used or protected fields / classes
5. performance if often a concern

So over all this seem to quite well align with what we have in SWT and we can assume that usually people do not do malicious things (and if they want there are other ways e.g. reflection anyways) and it well aligns with not adding any overhead.

If on the other hand we think we can not trust the eventhandler and really bad things happen, one might simply set critical values again afterwards (here the zoom level) might be something between the lines of do a full copy.

[HeikoKlare]

Just talked with @amartya4256 and turns out we actually have the same understanding: copying the event (or restoring the zoom value after returning from forwarding the event as just proposed) would prevent the risk of custom listeners accidentally (not necessarily maliciously) modifying the event and breaking follow-up consumers by that. I will of course not prevent this if the modified event is forwarded anywhere else by the custom listener.

As said, I am totally fine with accepting this risk as I do not think anyone will do that maliciously (but at most accidentally) and as you all highlighted we have other similar situations where somethink like that could happen. So let's keep everything as is based on the common understanding that we now have. Thank you all for your input.

[HeikoKlare]

And please adapt the commit message in terms of removing the reference to an already closed issue and with some more explanation on such a large change (reason, basic concept etc.).