Skip to content

Make DPIZoomChangeHandlers concurrent R/W able #62 #1806

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 7, 2025
Merged

Make DPIZoomChangeHandlers concurrent R/W able #62 #1806

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ...les/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/internal/DPIZoomChangeRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,14 @@

import java.util.*;
import java.util.Map.*;
import java.util.concurrent.*;

import org.eclipse.swt.*;
import org.eclipse.swt.widgets.*;

public class DPIZoomChangeRegistry {

private static Map<Class<? extends Widget>, DPIZoomChangeHandler> dpiZoomChangeHandlers = new TreeMap<>(
private static Map<Class<? extends Widget>, DPIZoomChangeHandler> dpiZoomChangeHandlers = new ConcurrentSkipListMap<>(
Copy link
Contributor

@laeubi laeubi Feb 6, 2025

Choose a reason for hiding this comment

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

I just wanted to mention that his will leak classloaders and lead to bundles that are updated/uninstalled retained forever.

Copy link
Member

@fedejeanne fedejeanne Feb 6, 2025

Choose a reason for hiding this comment

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

Hm, interesting point, I hadn't seen it before. That would mean that some consumer registers a custom subclass of Widget by calling this method and then unloads the bundle that contained that custom subclass, right? If that's the case, some sort of WeakHashMap could help or simply offering a deregister method, right?

One thing in advance, this method is part of internal so I see this issue as having slightly lower priority.

WDYT?

Copy link
Contributor

@laeubi laeubi Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

hat would mean that some consumer registers a custom subclass of Widget

Yes I don't know where an listener is registered globally, but if DPI awareness only works for "buildins" that would be really bad.

e.g. https://github.com/EclipseNebula/nebula offers a lot of nice and useful custom widgets.

Copy link
Contributor

@akoch-yatta akoch-yatta Feb 6, 2025

Choose a reason for hiding this comment

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

Every custom widget is based on SWT widget, so they inherit the awareness. If there is DPI relevant state in a custom widget, you can refresh that by reacting on a ZoomChanged event.

Copy link
Contributor

@laeubi laeubi Feb 6, 2025

Choose a reason for hiding this comment

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

If there is DPI relevant state in a custom widget, you can refresh that by reacting on a ZoomChanged event.

Sure but why don't we use that in SWT directly?

Copy link
Member

@fedejeanne fedejeanne Feb 6, 2025

Choose a reason for hiding this comment

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

Sure but why don't we use that in SWT directly?

The answer is in the section DPI zoom update propagation of #1064 (comment). IIRC one of the key points was: guaranteeing the order in which the event is processed is important.

Copy link
Contributor

@laeubi laeubi Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

IIRC one of the key points was: guaranteeing the order in which the event is processed is important.

I can only see that we need different handlers for different classes what is possible and as far as I know ordering is also some kind of guaranteed where I would like to see an example where it is actually important.

And then the same question arise: If it really is important how are custom widgets supposed to do it?

Copy link
Member

@fedejeanne fedejeanne Feb 6, 2025

Choose a reason for hiding this comment

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

Can we move this discussion to vi-eclipse/Eclipse-Platform#232 ?

Copy link
Contributor

@laeubi laeubi Feb 6, 2025

Choose a reason for hiding this comment

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

Theoretically yes, but why not at the SWT repo where the code lives and where it actually is implemented and seem to cause issues now?

Copy link
Contributor

@akoch-yatta akoch-yatta Feb 6, 2025

Choose a reason for hiding this comment

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

IIRC one of the key points was: guaranteeing the order in which the event is processed is important.

I can only see that we need different handlers for different classes what is possible and as far as I know ordering is also some kind of guaranteed where I would like to see an example where it is actually important.

What does "some kind of guaranteed" mean? You mean by order of registration for the event?
Order is e.g. important when internal state is updated a subclass relies upon. The easiest example for this is updating the nativeZoom in the Widget callback. I don't remember more examples out of my head. There are probably more examples about resources or resizing, but I would need to have a deeper look. I see it like the order of constructor execution: you always call super first, so the super class can make sure it adapt its provided state accordingly first.

One thing we could rethink is externalizing re-layouting, that is triggered inside of the callbacks currently. This was inherited from the initial approach and there were issues in the beginning when we tried to move that, but perhaps those were related to side effects, that we solved in the mean time.

And then the same question arise: If it really is important how are custom widgets supposed to do it?

Custom widgets are in my point of view quite different from the base widgets in SWT, e.g. they are extensions of existing widgets, a composition of existing widgets or just drawing directly on a GC.
If there is DPI relevant state in a custom widget, you can refresh that by reacting on a ZoomChanged event, that will be propagated additionally. If you directly refer to the order, then you can probably create issues when you have a hierarchy of custom widgets. We did e.g. a test run with nebula widgets and as far as I remember we only found issues where DPI relevant state was used.

(o1, o2) -> {
if(o1.isAssignableFrom(o2)) {
return -1;
Expand Down
Loading