Fix for Flickering in Embedded Apps

[amartya4256]

What it does

---

<TL;DR>

Drastically reduces the flickering when resizing embedded components by ignoring calls to Control::setRedraw(boolean) for controls with embedded children.

---

This PR contributes to fixing the flicker issue in SashLayout whenever an embedded app is present inside it. The fix disallows the setredraw on controls if they contain any embedded application as their descendant. There's an issue open https://bugs.eclipse.org/bugs/show_bug.cgi?id=558392 for a long time for the resizing of the sashlayout. The current way of performing layout on mouse move works fine for eclipse widgets by calling setRedraw to false which freezes the state of UI and performing a re-layout and then setting redraw to true leading to a smooth transition of the Sashlayout when its borders are dragged by mouse but it is not the same for external Embedded applications.

The embedded applications, e.g. webview 2, excel, etc, use a different renderer because of which whenever setRedraw is set to false, the whole portion where it was drawn goes blank white and when its set back, the apps are visible again, leading to a flicker effect.

One possible solution is to never allow this setredraw to be set back and forth when there's a child with embedded application present and visible. This PR implements the same approach disallowing setRedraw to be enforcedin this case. Even if it is not called from SashLayout, if a client sets it to false, it will lead to the disappearance of the application from the display, which is not desirable. So, the approach is to only allow it when the embedded applications are either not visible or not present.

The solution is not perfect but it definitely feels better as compared to the flickering effect. It definitely brings back a shaky effect on the min/max button as mentioned in the bug above while resizing but only when embedded apps are visible. However, this effect is also visible in these scenarios by default: #1122 (comment) and also when you resize the shell.

Contributes to #1122

[github-actions]

Test Results

   483 files  ±0     483 suites  ±0   8m 44s ⏱️ -29s
 4 095 tests ±0   4 085 ✅ ±0   7 💤 ±0  3 ❌ ±0 
16 173 runs  ±0  16 080 ✅ ±0  90 💤 ±0  3 ❌ ±0 

For more details on these failures, see this check.

Results for commit 7cee4b6. ± Comparison against base commit 485f9db.

♻️ This comment has been updated with latest results.

[fedejeanne]

It works as expected and the flickering is reduced.

@sratz would you like to take a look at it before I merge it?

[HeikoKlare]

Can't we calculate this on-the-fly (via the children) instead of managing a data structure storing those widgets?

[fedejeanne]

Done. I had to move the check to Composite since Control doesn't know its children.

[HeikoKlare]

Why move this to Composite? If I call setRedraw() on an embedded control, the logic will not apply anymore. That sounds incorrect to me. E.g., if I call setRedraw() on a browser, it will unintendedly be applied.

[fedejeanne]

Good point. How would you suggest to do it?

[HeikoKlare]

I would simply evaluate this in the setRedraw() method of Control instead of the one of Composite.

[fedejeanne]

How would you get the children? Can you provide a snippet?

[HeikoKlare]

I am afraid I do not understand the question, as I would do it like it is done now, just at a different place. Best may be to use a polymorphic implementation, i.e, something like:

In Control:

boolean hasEmbeddedChildren() {
	if (isDisposed() || !isVisible()) {
		return false;
	}
	return (this.style & SWT.EMBEDDED) != 0;
}

In Composite:

boolean hasEmbeddedChildren() {
	if (super.hasEmbeddedChildren()) {
		return true;
	}
        return Stream.of(getChildren()).anyMatch(Control::hasEmbeddedChildren);
}

[fedejeanne]

Got it. I added your snippets (with some slight modifications)

[sratz]

Is this really a good indicator? Do all 'embedded' controls actually have this problem?

Also: In Edge there is a comment that this flag is added as some workaround/hack only:

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT Browser/common/org/eclipse/swt/browser/Browser.java

Lines 202 to 205 in fa71076

	if ("win32".equals (platform) && (style & SWT.EDGE) != 0) { //$NON-NLS-1$
	/* Hack to enable Browser to receive focus. */
	style |= SWT.EMBEDDED;
	}

Is this workaround actually needed still? I removed it for testing and cannot see any problem.

[fedejeanne]

Do all 'embedded' controls actually have this problem?

AFAIK, yes. You can try embedding an Excel sheet, it'll flicker too.

Is this workaround actually needed still? I removed it for testing and cannot see any problem.

Since the logic checks for the presence of SWT.EMBEDDED, if you remove it then Edge will flicker. I tried and it flickers (I had to restart the application after the change, hot-code replacement didn't work).

[HeikoKlare]

My understanding of the original question (whether the original workaround is still needed) asks for the flag being required as a workaround for the focus handling, not for identifying that for the control redrawing should not be disabled.
So rephrasing the question: with this PR, does setting the embedded flag become a workaround just for identifying that redrawing should not be disabled for the browser control? If that's the case, we might think of other properties of all controls for which redrawing should not be disabled.

[fedejeanne]

I don't know if SWT.EMBEDDED is still needed for the browser to receive focus (according to @sratz, it isn't) but it is needed for this PR.

What other identifier would you propose instead?

[HeikoKlare]

but it is needed for this PR.

That's not necessarily true. From a conceptual perspective, what is needed for this PR is some identifier for controls for which redrawing must not be disabled because they simply disappear when disabling redrawing (and then lead to flickering). The current assumption was that SWT.EMBEDDED is a valid identifier, but if Edge just uses this flag as some workaround (that we may remove now), it does not seem to be a proper identifier but is rather another workaround to attach some (rather random) flag to controls for identifying that redrawing should not be disabled for them. If I remember correctly, this embedded flag is also used for embedding Swing controls, isn't it? And is it correct to disable redrawing for them as well?

Might be that there currently is no proper identifier for those controls for which redrawing must not be disabled. Then a solution might be to add some method to Control to identify whether redrawing can be disabled, which can then be properly implemented for the affected controls like a browser using Edge (and other embeddings of native controls on Windows).

[fedejeanne]

I get what you guys mean now, thank you for the clarification.

I changed the code so it doesn't use SWT.EMBEDDED but a newly created method instead.

[fedejeanne]

@HeikoKlare I added the changes you requested and they work as expected. I have currently 2 commits just in case it's necessary to revert back to a previous implementation. All commits will be squashed before merging.

[HeikoKlare]

This looks like an acceptable solution to me now. The name of the new method seems a bit "unspecific" to me, as I would a methods called embeds to be somehow consistent with the EMBEDDED flag being specified, but it's actually somewthing different. So it should rather be some embedWin32Control or the like?

Only, if I am not mistaken, the change is not okay from API perspective: protected methods are part of the API, so they need to exist in all three OS implementations consistently. So either the new method has to be added to all OS implementations or it needs to be implemented in a way that is not part of the API (e.g., using package protected methods only, but since the classes are in different packages, that does not seem to be possible).

[fedejeanne]

I changed the name to win32_embedsControl so it follows the same "standard" of other OS-specific methods like Image::win32_getHandle.

Mac and Linux implementations are also provided.

[HeikoKlare]

The updated version is semantically a bit strange: There are now win32_* methods within cocoa/gtk implementations, which does not feel correct.
I would either ...

• make the methods OS-independent, i.e., just let them yield whether the control contains an embedded native control. But that may not be correct for cocoa/gtk either, as there also native controls may be embedded, for which the method then returns false unexpectedly.
• place the method in the win32 implementation only. Then it will probably not be possible to use a polymorphic implementation but just some central logic with a case distinction over types, but since the type hierarchy is supposed to be sealed anyway, that may be fine here.

I would probably go with the latter solution to not unnecessarily bloat the cocoa/gtk implementations.

And I would still use a proper name for the new method in any case. embedsControl makes me expect "true" as a result for every composite (at least for every composite actually having children). The name should be specific to what you can actually expect from that method.

[fedejeanne]

@HeikoKlare would you mind coding it? I think it would be easier, since you probably have a very specific solution in mind :-)

You have write permissions in this PR and you were also added as co-author in a previous commit so you have green light 🟢

[HeikoKlare]

would you mind coding it? I think it would be easier, since you probably have a very specific solution in mind :-)

Actually, I don't have a specific solution in mind. I just posted the abstract options that I see to properly realize the intended behavior. And unfortunately, I will probably not have the time to work on this.

[HeikoKlare]

This now looks like the most local, least invasive solution for the issue we can achieve with current understanding.