Open Mazi-S opened 5 months ago
Now that you mention it and after some thinking I agree that the Swing behavior is really unintuitive
behavior which we should definitely correct as part of the SwingTree API.
I was also thinking about naming and really like the idea of naming
the current listener behavior onMouseEnterSurface
/ onMouseExitSurface
.
What I am not sure about is the implementation details here. How would we implement this to be reliable as well as maintainable? I would want to avoid having to register mouse listener on all sub-components. But then how do we filter out the exit/enter events between the component/sub-component transition and the mouse leaving the component entirely? What if the sub-component fills out the component to its entirety, how do we get the events then?
I think the behavior is even weirder than I thought at first :sweat_smile:
If you do not register a listener on the child, it will behave as expected.
of(this).withLayout("wrap 2, fill", "20[100]20[100]20", "20[100]20[100]20")
.withBackground(COLOR_1)
.onMouseEnter(delegate -> delegate.getComponent().setBackground(COLOR_1_HOVER))
.onMouseExit(delegate -> delegate.getComponent().setBackground(COLOR_1))
.add(GROW,panel().withBackground(COLOR_2)
.onMouseEnter(delegate -> delegate.getComponent().setBackground(COLOR_2_HOVER))
.onMouseExit(delegate -> delegate.getComponent().setBackground(COLOR_2))
.add(label("With listener"))
)
.add(GROW, panel().withBackground(COLOR_2).add(label("No listener")))
.add(GROW, panel().withBackground(COLOR_2).add(label("No listener")))
.add(GROW, panel().withBackground(COLOR_2).add(label("No listener")));
Yeah that is weird, I have no idea what kind of usage pattern they were imagining with this behavior. But it is really really unintuitive.
It is even worse than what we expected initially because it essentially allows child components to steal away the enter/exit event handling calls from a parent component. So if you design a parent component with the assumption that it knows when the cursor is hovering over it, you are in for a couple of bug fix iterations as soon as child components get more advanced...
This is actually a mix between the two behaviors we expected to exist. If every component has listener it reflects surface based events, and in case of the children not having event listener it behaves like how we know it from the web. That is just awful!
I guess what goes on under the hood is that it is based on the event being consumed. It's exactly like a button click event. You can only click on button at a time. So the event is consumed and can no longer trigger other event handlers.
But mouse exit/enter events are not button clicks... So I would consider this a bug that really should be fixed sooner rather than later so that the code in the framework does not rely on this awful awful behavior too much!
I think we might be able to implement the expected default mouse enter/exit behavior, where a hover is based on the mouse being on top of something, by re-dispatching the events to the parent:
diff --git a/src/main/java/swingtree/UIForAnySwing.java b/src/main/java/swingtree/UIForAnySwing.java
index 35746825..33b07b47 100644
--- a/src/main/java/swingtree/UIForAnySwing.java
+++ b/src/main/java/swingtree/UIForAnySwing.java
@@ -3136,6 +3136,7 @@ public abstract class UIForAnySwing<I, C extends JComponent> extends UIForAnythi
c.addMouseListener(new MouseAdapter() {
@Override public void mouseEntered(MouseEvent e) {
_runInApp(() -> onEnter.accept(new ComponentMouseEventDelegate<>(c, e )));
+ c.getParent().dispatchEvent(e);
}
});
})
@@ -3157,6 +3158,7 @@ public abstract class UIForAnySwing<I, C extends JComponent> extends UIForAnythi
c.addMouseListener(new MouseAdapter() {
@Override public void mouseExited(MouseEvent e) {
_runInApp(() -> onExit.accept(new ComponentMouseEventDelegate<>(c, e )));
+ c.getParent().dispatchEvent(e);
}
});
})
I have not tested this properly though...
And no idea how to implement onMouseEnterSurface
and onMouseExitSurface
.
Also, I fear that one problem with the c.getParent().dispatchEvent(e);
approach
is that that it gets messy when the mouse cursor transitions from the parent surface to
the child surface in case of the child also having both enter/exit listeners.
In that case the parent will receive an exit event, which is then immediately followed by the re-dispatched enter event from the child component. So we would probably need to ignore exit events if the mouse cursor is still in the bounds of the (parent) component. And I guess we should also ignore enter events from the child.
Also, before I forget, I thing it is also important to check if the user explicitly requested the event to be consumed. So something along the lines of:
if ( !e.isConsumed())
c.getParent().dispatchEvent(e);
Another after thought:
Maybe this whole re-dispatch approach is also not necessary by merely letting the parent check if the source component of an enter/exit event during the transition between parent and child is coming from the child and then just ignore that.
I think we might be able to implement the expected default mouse enter/exit behavior, where a hover is based on the mouse being on top of something, by re-dispatching the events to the parent:
diff --git a/src/main/java/swingtree/UIForAnySwing.java b/src/main/java/swingtree/UIForAnySwing.java index 35746825..33b07b47 100644 --- a/src/main/java/swingtree/UIForAnySwing.java +++ b/src/main/java/swingtree/UIForAnySwing.java @@ -3136,6 +3136,7 @@ public abstract class UIForAnySwing<I, C extends JComponent> extends UIForAnythi c.addMouseListener(new MouseAdapter() { @Override public void mouseEntered(MouseEvent e) { _runInApp(() -> onEnter.accept(new ComponentMouseEventDelegate<>(c, e ))); + c.getParent().dispatchEvent(e); } }); }) @@ -3157,6 +3158,7 @@ public abstract class UIForAnySwing<I, C extends JComponent> extends UIForAnythi c.addMouseListener(new MouseAdapter() { @Override public void mouseExited(MouseEvent e) { _runInApp(() -> onExit.accept(new ComponentMouseEventDelegate<>(c, e ))); + c.getParent().dispatchEvent(e); } }); })
I have not tested this properly though...
If we register a mouse listener on the child using .peek(p -> p.addMouseListener(...))
, this approach would be broken. In this case we can't re-dispatching the events to the parent. So the parent might not receive enter/exit events.
This would result in breaking the hover behavior of the parent by changing the child. I think this could be hard to debug and is not a good idea.
Another after thought:
Maybe this whole re-dispatch approach is also not necessary by merely letting the parent check if the source component of an enter/exit event during the transition between parent and child is coming from the child and then just ignore that.
When a child component is entered, the parent receives an exit event with the parent as the source. So the source is of no help. However, we can check if the event location is inside the parent and then just ignore the exit event.
But this still does not solve our problem. When we enter/exit the child component without hovering over the parent (e.g. when there is no gap between parent and child). The parent does not get the enter/exit event.
I pushed one possible solution. I do not really like having a separate event dispatcher, and if we decide to use it, we should definitely use weak references. But I think this is the only option we have. :thinking:
And we can't ignore exit parent + enter child events when we enter a subcomponent because we don't know if a parent enter was fired before we entered the child. And I don't think we should track any state whether an enter event was fired or not.
@Gleethos Maybe you can check it out and have an opinion about it.
I looked at your suggested implementation and have a lot to say. Here a few things I noticed:
swingtree
package is so huge... :(
WeakHashMap
s but also we need to ensure that
the mouse listener do also not have strong references to the components!!
Otherwise the weak hash maps are completely obsolete.I don't want to mess with the branch, since you may have
local changes there, so here a git diff
containing the suggested changes;
You may want to apply it and look over it:
Index: src/main/java/swingtree/UIForAnySwing.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/main/java/swingtree/UIForAnySwing.java b/src/main/java/swingtree/UIForAnySwing.java
--- a/src/main/java/swingtree/UIForAnySwing.java (revision d553d90d2d9eaba05526ad37cbb1bf1e8d3418ed)
+++ b/src/main/java/swingtree/UIForAnySwing.java (date 1723616464208)
@@ -19,7 +19,6 @@
import swingtree.api.Styler;
import swingtree.api.UIVerifier;
import swingtree.api.mvvm.ViewSupplier;
-import swingtree.event.AdvancedEventDispatcher;
import swingtree.input.Keyboard;
import swingtree.layout.AddConstraint;
import swingtree.layout.LayoutConstraint;
@@ -30,6 +29,7 @@
import javax.swing.border.TitledBorder;
import java.awt.*;
import java.awt.event.*;
+import java.lang.ref.WeakReference;
import java.util.ArrayList;
import java.util.Objects;
import java.util.Optional;
@@ -3133,14 +3133,17 @@
*/
public final I onMouseEnter( Action<ComponentMouseEventDelegate<C>> onEnter ) {
NullUtil.nullArgCheck(onEnter, "onEnter", Action.class);
- return _with( c -> {
+ return _with( thisComponent -> {
+ WeakReference<@Nullable C> source = new WeakReference<>(thisComponent);
MouseListener listener = new MouseAdapter() {
@Override public void mouseEntered(MouseEvent e) {
- _runInApp(() -> onEnter.accept(new ComponentMouseEventDelegate<>(c, e )));
+ @Nullable C localComponent = source.get();
+ if ( localComponent != null )
+ _runInApp(() -> onEnter.accept(new ComponentMouseEventDelegate<>( localComponent, e )));
}
};
- c.addMouseListener(listener);
- AdvancedEventDispatcher.addMouseEnterListener(c, listener);
+ thisComponent.addMouseListener(listener);
+ AdvancedEventDispatcher.addMouseEnterListener(thisComponent, listener);
})
._this();
}
@@ -3156,14 +3159,17 @@
*/
public final I onMouseExit( Action<ComponentMouseEventDelegate<C>> onExit ) {
NullUtil.nullArgCheck(onExit, "onExit", Action.class);
- return _with( c -> {
+ return _with( thisComponent -> {
+ WeakReference<@Nullable C> source = new WeakReference<>(thisComponent);
MouseListener listener = new MouseAdapter() {
@Override public void mouseExited(MouseEvent e) {
- _runInApp(() -> onExit.accept(new ComponentMouseEventDelegate<>(c, e )));
+ @Nullable C localComponent = source.get();
+ if ( localComponent != null )
+ _runInApp(() -> onExit.accept(new ComponentMouseEventDelegate<>( localComponent, e )));
}
};
- c.addMouseListener(listener);
- AdvancedEventDispatcher.addMouseExitListener(c, listener);
+ thisComponent.addMouseListener(listener);
+ AdvancedEventDispatcher.addMouseExitListener(thisComponent, listener);
})
._this();
}
Index: src/main/java/swingtree/event/AdvancedEventDispatcher.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/main/java/swingtree/event/AdvancedEventDispatcher.java b/src/main/java/swingtree/AdvancedEventDispatcher.java
rename from src/main/java/swingtree/event/AdvancedEventDispatcher.java
rename to src/main/java/swingtree/AdvancedEventDispatcher.java
--- a/src/main/java/swingtree/event/AdvancedEventDispatcher.java (revision d553d90d2d9eaba05526ad37cbb1bf1e8d3418ed)
+++ b/src/main/java/swingtree/AdvancedEventDispatcher.java (date 1723616464199)
@@ -1,21 +1,23 @@
-package swingtree.event;
+package swingtree;
import java.awt.*;
import java.awt.event.MouseEvent;
import java.awt.event.MouseListener;
-import java.util.*;
+import java.util.ArrayList;
import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
-public class AdvancedEventDispatcher {
+final class AdvancedEventDispatcher {
private static final AdvancedEventDispatcher eventDispatcher = new AdvancedEventDispatcher();
- public static void addMouseEnterListener(Component component, MouseListener listener) {
+ static void addMouseEnterListener(Component component, MouseListener listener) {
List<MouseListener> listeners = eventDispatcher.enterListeners.computeIfAbsent(component, k -> new ArrayList<>());
listeners.add(listener);
}
- public static void addMouseExitListener(Component component, MouseListener listener) {
+ static void addMouseExitListener(Component component, MouseListener listener) {
List<MouseListener> listeners = eventDispatcher.exitListeners.computeIfAbsent(component, k -> new ArrayList<>());
listeners.add(listener);
}
@@ -24,8 +26,8 @@
private final Map<Component, List<MouseListener>> exitListeners;
private AdvancedEventDispatcher() {
- enterListeners = new HashMap<>();
- exitListeners = new HashMap<>();
+ enterListeners = new WeakHashMap<>();
+ exitListeners = new WeakHashMap<>();
Toolkit.getDefaultToolkit().addAWTEventListener(this::onMouseEvent, AWTEvent.MOUSE_EVENT_MASK);
}
@@ -53,8 +55,9 @@
List<MouseListener> listeners = enterListeners.get(component);
if (listeners != null) {
+ MouseEvent localMouseEvent = withNewSource(mouseEvent, component);
for (MouseListener listener : listeners) {
- listener.mouseEntered(mouseEvent);
+ listener.mouseEntered(localMouseEvent);
}
}
@@ -69,8 +72,9 @@
List<MouseListener> listeners = exitListeners.get(component);
if (listeners != null) {
+ MouseEvent localMouseEvent = withNewSource(mouseEvent, component);
for (MouseListener listener : listeners) {
- listener.mouseExited(mouseEvent);
+ listener.mouseExited(localMouseEvent);
}
}
@@ -78,7 +82,21 @@
}
}
- public static boolean containsScreenLocation(Component component, Point screenLocation) {
+ private MouseEvent withNewSource( MouseEvent event, Component newSource ) {
+ return new MouseEvent(
+ newSource,
+ event.getID(),
+ event.getWhen(),
+ event.getModifiersEx(),
+ event.getX(),
+ event.getY(),
+ event.getClickCount(),
+ event.isPopupTrigger(),
+ event.getButton()
+ );
+ }
+
+ private static boolean containsScreenLocation(Component component, Point screenLocation) {
Point compLocation = component.getLocationOnScreen();
Dimension compSize = component.getSize();
int relativeX = screenLocation.x - compLocation.x;
Now some more thoughts from a holistic point of view:
In principle I like the approach of handling this through the
Toolkit.getDefaultToolkit().addAWTEventListener(..);
to really capture
the intended behavior across all possible exit/enter event.
So it is definitely consistent, and therefore in theory the desired solution.
But at the same time, that is itself a problem because it is truly global.
As soon as the AdvancedEventDispatcher
class is loaded into the runtime by
the class loader (which happens as soon as a consumer uses SwingTree) the behvaior
of all mouse enter/exit listeners suddenly changes!
Please correct me if I am wrong, but that is how it looks to me.
This is not a big deal when writing a new application from scratch. But I can't even begin to imagine the amount of unforeseen side effects this might have in larger systems. On a second thought, you may not even need a big system to have side effects for this sort of intrusive change to the entire event system. Things like the look and feel implementations also depend on the current (buggy) behavior of the AWT event system. So our little fix can cause all kinds of strange bugs.
To me that is a very big risk to take here.
If we register a mouse listener on the child using .peek(p -> p.addMouseListener(...)), this approach would be broken. In this case we can't re-dispatching the events to the parent. So the parent might not receive enter/exit events.
This would result in breaking the hover behavior of the parent by changing the child. I think this could be hard to debug and is not a good idea.
I agree that this is not an ideal situation, but we may want to accept this inconsistency with the simple rationale of
I think this is something that is an okay tradeoff we can make in this situation.
I also think that the native behavior does allow for a comprehensible mental model when combined with the SwingTree exit/enter mouse events. A native listener will not re-dispatch the event to the parent, which makes it inherently greedy. So you can think of it as a greedy event listener which overrides the need of the the SwingTree based parent event listener.
An interesting use case for this would be a kind of torus shaped UI with an inner and an outer component. The outer component has an outset shadow, which raises it from the environment, and the inner (child) component has an inset shadow which lowers it... A hover effect on the outer component would only really make sense when the mouse is directly on its surface because it has the visual suggestion that the inner component is not really part of the parent surface. Here you would want to use the native behavior instead of what we are talking about.
I still find the native Swing behavior inconsistent and something to be discouraged, but I don't think it is a good idea to override the native behavior entirely.
So in my opinion we should investigate the localized alternative some more.
I really like your suggestions and have implemented your changes. You are absolutely right about not exposing the AdvancedEventDispatcher
.
My thoughts regarding the event source was that we might want that information in the parent. Perhaps to distinguish between events from the parent itself or from a child. But you are probably right and we should be consistent.
About the side effects of using AdvancedEventDispatcher
approach.
You're right, we can't change the default behavior of swing because some other parts may rely on it. But child events are only distributed to parents if they are registered at the AdvancedEventDispatcher
. If you don't use SwingTree
, the events won't be registered and therefore won't be propagated. So the side effects you describe should not occur.
However, there are side effects. For each mouse event, we check if a parent component is registered. This can affect performance.
My thoughts regarding the event source was that we might want that information in the parent.
Ah, I see. Yeah that could possibly be valuable information in some cases. But if we want to convey this information I would like to do so deliberately. So for example introducing a method on the delegate object which returns an optional of an "event parent" or something along those lines...
If you don't use SwingTree, the events won't be registered and therefore won't be propagated. So the side effects you describe should not occur.
I see! Ok then it is probably fine to go about this approach. Although I would really like to test this some more and especially make sure that we do not get any memory leaks through global references here...
However, there are side effects. For each mouse event, we check if a parent component is registered. This can affect performance.
Well we do a hashmap based lookup, which should be fine. Event in giant applications I don't ever expect there to be much more than a couple thousand entries (correct me if I am wrong but they are only added if they have enter/exit listeners right?). And so the lookup will stay super cheap. The loop on the other hand might be a problem if the user wants to register thousands of listeners to a component... I'd argue that is an abuse of the feature and the user is at fault here.
One thing I noticed here:
for (MouseListener listener : listeners) {
listener.mouseEntered(localMouseEvent);
}
...and here:
for (MouseListener listener : listeners) {
listener.mouseExited(localMouseEvent);
}
Here we are calling client code! Usually this leads to a SwingTree listener...
But it can also be an event listener registered through peek( it -> it .addMouseListener(..) )
.
The client can do all sorts of messy thing in there, most of which does not concern us.
But what does concern us is exceptions being thrown. Because they can seriously mess up our
control flow here and prevent all other listeners from being called!
We should catch and log exceptions here with helpful context information for the client and then continue executing.
Also, I did some debugging and noticed that the kinds of exit/enter events that are being fire on the parent are really inconsistent when the mouse cursor movement involves a child that also has listeners...
Here an illustration that explains what I observed:
I added print statements to the enter and exit event listeners of the parent panel (which turns read when the mouse hovers over it).
The order in which what events are fired here is completely nonsensical and does not follow an intuitive pattern whatsoever. I know it is difficult to get rid of enter/exit event pairs on the transition between parent and child surface. But then I would expect there to be a consistent sequence in which after every enter follows an exit, and after every exit an enter...
I don't think having this be inconsistent is acceptable. A reliable ping pong kind of switch between the two is ok, event if it is between parent with listener and child with listener.
Ideally I would like to see these events gone entirely. It is kind of nonsensical that the parent component gets enter and exits events while the mouse cursor is right in the middle of the component,,,,
So for example introducing a method on the delegate object which returns an optional of an "event parent" or something along those lines...
I really like this idea :wink: this would allow to have the Swing behavior with SwingTree
by just ignoring events with a parent.
And so the lookup will stay super cheap.
Yes, the lookup itself should not be the problem.
The loop on the other hand might be a problem if the user wants to register thousands of listeners to a component...
I also think that the for
loop should not be a problem. Only if the user really wants to register thousands of listeners for a single component. But as you said, I think this is an abuse.
Where I see the problem is by doing for each mouse event a lookup for each parent and for the parent of that parent and so on. This happens for every mouse enter/exit event. Not just the ones registered with SwingTree
.
I was suppressing exit events on a component when the mouse was still inside the component, which caused this inconsistency. I have removed this check and the order is now as follows:
root -> enter
root -> exit
root -> enter
root -> exit
root -> enter
root -> exit
The same example with a child listener that prints exit / enter.
root -> enter
root -> exit
root -> enter
child -> enter
root -> exit
child -> exit
root -> enter
root -> exit
Ideally I would like to see these events gone entirely. It is kind of nonsensical that the parent component gets enter and exits events while the mouse cursor is right in the middle of the component,,,,
I think we can do that. But we would also need to store the information that an enter/exit event was fired, e.g. if the component is hovered or not. However, we should decide whether we want to do this at least for each component with an enter/exit listener.
I added a new delegate that holds information about the source of the root event.
I like the way it works now, and I think this is the best way to fix it. 🤔
Looking at the changes, I am happy with the event execution control flow being protected from user exceptions now.
But I am not sure I understand the need for the two new classes.
The AdvancedSurfaceListener
seems to be the exact same thing as the MouseAdapter
in terms of usecase.
So I suggest using the MouseAdapter
instead, as it is something developers are more familiar with.
In principle I am in favor of introducing new API surface, if it is better API. But seems to be a duplication
and it is also a package private class, so there is no additional utility to a library user either...
And I am also not sure we need a dedicated type for mouse exit and enter events. It might make sense if the "surface events" had much much more information to offer, than just a secondary source object. But I don't see that ever happening because Swing is no longer receiving new features...
Instead I suggest adding this to the regular ComponentMouseEventDelegate
.
From the API users point of view it is also completely unclear what the two new methods on the delegate are for, or what exactly they tell me!
/**
* Returns the {@link MouseEvent#getSource()} of the root {@link MouseEvent}.
*
* @return The source associated with this delegate.
*/
public Object getSource() {
return _source;
}
/**
* Determines if the current component is the {@link MouseEvent#getSource()} of the root {@link MouseEvent}.
*
* @return {@code true} if the component associated with this delegate is the source, {@code false} otherwise.
*/
public boolean isSource() {
return _component() == _source;
}
Here it says that getSource
returns the root of the mouse event.
As a user I ask myself "What exactly is the root of an event, and more importantly, how is it different from getEvent().getSource()
!". Documentation should not just tell the user what it is, but why does this even exist and how should it typically be used.
And that is where I have to object to this being exposed to the user in general. What is the usecase of accessing this information?
I think if we are being honest here. The fact that there can be a secondary source object... is just
implementation detail on the SwingTree side. Or am I missing something?
Checking if _component() != _source
is basically answering the question: "Is this event based on the SwingTree workaround for Swing enter/exit events?". What does the user need to know that we are doing a workaround for them?
Does that make sense?
Yeah, so I guess I am kind of objecting to this whole "root source"/"secondary source" object complexity being expose to the user in general. Why should they care? I am genuinely asking.
The current implementation is more of a proof of concept, so the naming and documentation would need some work if we were to move in that direction. So yes, you are right.
In my opinion, the use case of the event root is to preserve the Swing behavior. Without this information, there is no way to have the Swing behavior with SwingTree. And having events fired when you enter/exit a child is even more misleading.
Suppressing all false enter and exit events adds complexity and can have a performance impact. The only way to do this is to track the current state (isEntered
or something like that). We always need at least a second listener when adding an enter or exit listener.
But I agree with you and we should not expose this root event. Furthermore, I would suggest to have a proper enter/exit behavior with swing tree, with events fired only on actual enter or exit.
The swing behavior can always be achieved with .peek
, and the performance impact should not be too bad.
In Swing, a component triggers a mouse exit when entering a subcomponent. I find this rather unintuitive and not the behavior I would expect.
Especially if I want to model a simple (CSS style) hover effect, I would need to register listeners on each child of the component. It would be very useful if we had an easy way to do this.
Maybe we can make it the default behavior of the
onMouseEnter
/onMouseExit
listener and introduce two new listeners (onMouseEnterSurface
,onMouseExitSurface
) for the Swing style approach?