Skip to content
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.

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

Some MouseEvent behaviors cannot be safely overridden because hooks for them do not yet exist #357

Closed
MichelMunoz opened this issue Sep 11, 2016 · 16 comments

Comments

@MichelMunoz
Copy link

MichelMunoz commented Sep 11, 2016

First, thanks for this very promising component.

Now, the observed behavior. In my code (a console widget) I wanted to reposition the cursor after a text selection was made (on mouse released, using moveTo), it worked well except when the drag ended out of the component area. Annoyed by the thing, I digged, and ended finding that it was somehow linked to hit computation and drag code ; when the drag (of selection) ends within the component all is fine, but when the drag ends outside the component the hit methods is called infinitelly and the moveTo doesnt work as expected.

Version used : the last one (7-M2), but the behavior was observed pre-7.

To reproduce/observe the problem, a "crude" approach :

  • create a subclass of StyleClassedTextArea , let's call it StyleClassedTextAreaExt, it only overrides the hit method to trace the calls and stack
@Override
    public CharacterHit hit(double x, double y) {
        CharacterHit hit = super.hit(x, y);
        System.out.println("HIT CALLED : ");
        (new Exception()).printStackTrace();
        return hit;
    }
  • create a basic fx app, border pane, and in center the StyleClassedTextAreaExt
  • launch the app, not in full screen, and keep an eye on console output
  • type some text in the text area
  • start a text selection in the text, keep the mouse pressed, move the cursor out of the text area to the left (same to the right), and the release the mouse
  • you then observe infinite calls in the logs

The observed stacks of the continuous calls are :

at fr.mmu.st2.console.ui.widget.console.StyleClassedTextAreaExt.hit(Console.java:246)
at org.fxmisc.richtext.StyledTextAreaBehavior.dragTo(StyledTextAreaBehavior.java:471)
at org.reactfx.value.Val.ifPresent(Val.java:151)
at org.fxmisc.richtext.StyledTextAreaBehavior.lambda$new$38(StyledTextAreaBehavior.java:259)
at org.reactfx.util.NonAccumulativeStreamNotifications.lambda$head$0(NotificationAccumulator.java:134)
at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:68)
at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:57)
at org.reactfx.ProperEventStream.emit(ProperEventStream.java:18)
at org.reactfx.util.NonAccumulativeStreamNotifications.lambda$head$0(NotificationAccumulator.java:134)
at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:68)
at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:57)
at org.reactfx.ProperEventStream.emit(ProperEventStream.java:18)
at org.reactfx.MappedStream.lambda$observeInputs$0(MappedStream.java:25)
at org.reactfx.util.NonAccumulativeStreamNotifications.lambda$head$0(NotificationAccumulator.java:134)
at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:68)
at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:57)
at org.reactfx.ProperEventStream.emit(ProperEventStream.java:18)
at org.reactfx.EmitBothOnEachStream.lambda$observeInputs$1(EmitBothOnEachStream.java:31)
at org.reactfx.util.NonAccumulativeStreamNotifications.lambda$head$0(NotificationAccumulator.java:134)
at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:68)
at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:57)
at org.reactfx.ProperEventStream.emit(ProperEventStream.java:18)
at org.reactfx.MappedStream.lambda$observeInputs$0(MappedStream.java:25)
at org.reactfx.util.NonAccumulativeStreamNotifications.lambda$head$0(NotificationAccumulator.java:134)
at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:68)
at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:57)
at org.reactfx.ProperEventStream.emit(ProperEventStream.java:18)
at org.reactfx.AccumulatingStream.lambda$observeInputs$0(AccumulatingStream.java:34)
at org.reactfx.util.NonAccumulativeStreamNotifications.lambda$head$0(NotificationAccumulator.java:134)
at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:68)
at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:57)
at org.reactfx.ProperEventStream.emit(ProperEventStream.java:18)
at org.reactfx.EventStreams$20$1.handle(EventStreams.java:421)
at javafx.animation.AnimationTimer$AnimationTimerReceiver.lambda$handle$484(AnimationTimer.java:57)
at java.security.AccessController.doPrivileged(Native Method)
at javafx.animation.AnimationTimer$AnimationTimerReceiver.handle(AnimationTimer.java:56)
at com.sun.scenario.animation.AbstractMasterTimer.timePulseImpl(AbstractMasterTimer.java:357)
at com.sun.scenario.animation.AbstractMasterTimer$MainLoop.run(AbstractMasterTimer.java:267)
at com.sun.javafx.tk.quantum.QuantumToolkit.pulse(QuantumToolkit.java:506)
at com.sun.javafx.tk.quantum.QuantumToolkit.pulse(QuantumToolkit.java:490)
at com.sun.javafx.tk.quantum.QuantumToolkit.lambda$runToolkit$404(QuantumToolkit.java:319)
at com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:95)
at com.sun.glass.ui.win.WinApplication._runLoop(Native Method)
at com.sun.glass.ui.win.WinApplication.lambda$null$148(WinApplication.java:191)
at java.lang.Thread.run(Thread.java:745)

P.S.
If it is of any value, one point I observed in the code of your component while digging the problem :

  • in debug, while reproducing the problem, one can see that if you put breakpoints in StyledTextArea.hit(x,y) in the two first conditions (that should be triggered, I guess, once the mouse goes out of the component area) ; they are never triggered during the problem :
public CharacterHit hit(double x, double y) {
        VirtualFlowHit<Cell<Paragraph<PS, S>, ParagraphBox<PS, S>>> hit = virtualFlow.hit(x, y);
        if(hit.isBeforeCells()) {
            return CharacterHit.insertionAt(0);     <----- (breakpoint here) never true when mouse goes out
        } else if(hit.isAfterCells()) {
            return CharacterHit.insertionAt(getLength());      <----- (breakpoint here) never true when mouse goes out
        } else {
            int parIdx = hit.getCellIndex();
            int parOffset = getParagraphOffset(parIdx);
            ParagraphBox<PS, S> cell = hit.getCell().getNode();
            Point2D cellOffset = hit.getCellOffset();
            CharacterHit parHit = cell.hit(cellOffset);
            return parHit.offset(parOffset);
        }
    }
  • what is triggerred though (and should not in this case, I guess) is in StyledTextAreaBehavior, the start auto scroll :
private void mouseDragged(MouseEvent e) {
        // don't respond if disabled
        if(view.isDisabled()) {
            return;
        }

        // only respond to primary button alone
        if(e.getButton() != MouseButton.PRIMARY || e.isMiddleButtonDown() || e.isSecondaryButtonDown()) {
            return;
        }

        Point2D p = new Point2D(e.getX(), e.getY());
        if(view.getLayoutBounds().contains(p)) {
            dragTo(p);
            autoscrollTo.setValue(null); // stops auto-scroll
        } else {
            autoscrollTo.setValue(p);    // starts auto-scroll     <------ HERE : triggered twice before infinite loop starts
        }

        e.consume();
    }

@MichelMunoz MichelMunoz changed the title Infinite loop call to StyleClassedTextArea.hit(x,y) on StyledTextAreaBehavior.dragTo outside widget boundaries Infinite calls to StyleClassedTextArea.hit(x,y) on StyledTextAreaBehavior.dragTo outside widget boundaries Sep 11, 2016
@JordanMartinez
Copy link
Contributor

If I'm understanding you correctly, this is what happens...?

  1. select some text
  2. move the caret outside of the StyledTextArea (or one of its flavors)
  3. release the mouse
  4. Expected: some amount of text is selected that correlates to the position of the mouse when released
  5. Reality: an infinite hit(x, y) loop occurs and crashes the program

@JordanMartinez
Copy link
Contributor

Additionally, does this correlate with #321 ?

@MichelMunoz
Copy link
Author

MichelMunoz commented Sep 12, 2016

Yeah i suppose it is correlated to #321
For your interpretation, I would phrase it as such :

  1. start selecting some text [ie. press mouse button and move to the left for example]
  2. continue the drag movement to the left and move the caret cursor outside of the StyledTextArea (or one of its flavors)
  3. release the mouse
  4. Expected: some amount of text is selected that correlates to the position of the mouse when released
  5. Reality: an infinite hit(x, y) loop occurs and crashes the program (no crash, just incorrect behavior)

I'll try to do a screen capture tonight.

@JordanMartinez
Copy link
Contributor

JordanMartinez commented Sep 12, 2016

What you described is what I meant, but your rephrasing communicates it much better than my original one 😃

@MichelMunoz
Copy link
Author

MichelMunoz commented Sep 12, 2016

There; I have done a screen capture showing the thing. You'll notice I've numbered the calls to hit.
First a I do some "normal" selection (where the mouse stays within the component), and the hit calls stop rapidly ; finally, I do a "drag outside" and there you see the hit calls never stopping (notice also how the whole text is selected) :

capture

@JordanMartinez
Copy link
Contributor

@MichelMunoz I tried reproducing the bug using the 7-M2 release and wasn't able to. After I release the mouse (even outside the JavaFX window), it stops the emission.

Could you show us a small stand-alone program of your code? I wonder if there's something else going on that causes this to occur, such as you overriding some of the default behavior that runs package-private code. Perhaps your issue arises because that code isn't executed.

@MichelMunoz
Copy link
Author

Ok, I'll try to do that this week end

2016-10-07 6:55 GMT+02:00 JordanMartinez [email protected]:

@MichelMunoz https://github.com/MichelMunoz I tried reproducing the bug
using the 7-M2 release and wasn't able to. After I release the mouse (even
outside the JavaFX window), it stops the emission.

Could you show us a small stand-alone program of your code? I wonder if
there's something else going on that causes this to occur, such as you
overriding some of the default behavior that runs package-private code.
Perhaps your issue arises because that code isn't executed.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#357 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ARaktiLfw6z2ylsb_eKlBBjnQy7P8-mcks5qxdC8gaJpZM4J6DpT
.

@TomasMikula
Copy link
Member

Let's also compare the environments. @MichelMunoz is apparently on Windows on JDK 1.8.0_91.

@MichelMunoz
Copy link
Author

Okay, I've done it, some explanations first....

The use case : I have a pseudo console with a prompt, I do a selection via drag ; end of drag automatically copies the selection to the clipboard (like some unix consoles), and the reposition the cursor at a valid position (ie. at minimum after the "prompt").

In the attached code you will see that you can trigger the bug by having the drag going outside the component in any direction (top, bottom, left, right, same problem) ; but if selection gesture stays within the component, no problemo.
As noted by @JordanMartinez if you disable the mouse-handling the problem does not appear.

I've joined the gradle build file (3+) to the sources and made the source as simple as possible
RTFX357.zip

@MichelMunoz
Copy link
Author

An even more minimalist version, without prompt mechanism or copy, same gradle as previous reply.
RTFX357simpler.zip

@JordanMartinez
Copy link
Contributor

Aye, it is an issue with overriding the default behavior.

@JordanMartinez
Copy link
Contributor

I'm posting @MichelMunoz's code here so one doesn't need to download it, and then I'll surround the issue with comment indicators ("/////"):

import static org.fxmisc.wellbehaved.event.EventPattern.eventType;
import static org.fxmisc.wellbehaved.event.InputMap.consume;
import static org.fxmisc.wellbehaved.event.InputMap.sequence;

import org.fxmisc.richtext.CharacterHit;
import org.fxmisc.richtext.StyledTextArea;
import org.fxmisc.wellbehaved.event.Nodes;

import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.input.MouseButton;
import javafx.scene.input.MouseEvent;
import javafx.scene.layout.BorderPane;
import javafx.stage.Stage;

public class RTFxBug2 extends Application {

    @Override
    public void start(Stage primaryStage) {
        StyleClassedTextAreaExt2 inputArea = new StyleClassedTextAreaExt2();

        Nodes.addInputMap(inputArea, sequence(
            /////////////// OVERRIDES DEFAULT BEHAVIOR ////////////////
            consume(eventType(MouseEvent.MOUSE_RELEASED), 
                // the following line prevents `autoscrollTo.set(null)`, 
                // which causes your issue
                            e-> inputArea.mouseReleased(e))
            /////////////// OVERRIDES DEFAULT BEHAVIOR ////////////////
        ));

        BorderPane root = new BorderPane();
        root.setCenter(inputArea);
        Scene scene = new Scene(root, 300, 250);

        primaryStage.setTitle("Bug #357");
        primaryStage.setScene(scene);
        primaryStage.show();
    }

    public static void main(String[] args) {
        launch(args);
    }
}

class StyleClassedTextAreaExt2 extends StyledTextArea<String, String> {

    public StyleClassedTextAreaExt2() {
        super("", (tf,s) -> {}, "", (te, s) -> {}, true);
    }

    int counter = 0;
    @Override
    public CharacterHit hit(double x, double y) {
        CharacterHit hit = super.hit(x, y);
        System.out.println("HIT call #" + counter++);
        return hit;
    }

///////////// To Achieve the same thing, you'll need to use a hook /////////
    public void mouseReleased(MouseEvent e) {
        if (e.getButton() == MouseButton.PRIMARY) {
            moveTo(0, SelectionPolicy.CLEAR);
        }
    }
}

You might already be able to use the hook we've implemented. See onSelectionDrop (and its placement in the behavior class here). I'm not sure whether we'll need to add other hooks for the POTENTIAL_DRAG or NO_DRAG cases in StyledTextAreaBehavior#mouseReleased's switch statement or whether onSelectionDrop hook alone will suffice..

@JordanMartinez
Copy link
Contributor

I just tested the onSelectionDrop hook and it won't work for your use case.

I think the real issue here is that STA's mouse behavior should not be overridden. Rather, there needs to be more hooks that can be overridden for special circumstances like this.

@MichelMunoz
Copy link
Author

Ok, so, I guess in my case a new hook "onSelectionDone" that would be called in the "case NO_DRAG:" of your link would work, no ?
(another question : how to disable some STA behavior like drag of selection ?, for example when you only want selection behavior.)

@JordanMartinez
Copy link
Contributor

Ok, so, I guess in my case a new hook "onSelectionDone" that would be called in the "case NO_DRAG:" of your link would work, no ?

Yes. As an alternative for right now until this gets fixed, you might be able to use area.setOnMouseReleased(consumer). However, I'm not sure if that would be called before or after the default behavior.

(another question : how to disable some STA behavior like drag of selection ?, for example when you only want selection behavior.)

We implemented the onSelectionDrop hook so that I could prevent text being dragged to another place. Rather, it pastes a copy of the selection to wherever I drag it. If you want to disable it completely, just overwrite the default code in onSelectonDrop with something else, such as an empty lambda:

area.setOnSelectionDrop(e -> {});

@JordanMartinez JordanMartinez changed the title Infinite calls to StyleClassedTextArea.hit(x,y) on StyledTextAreaBehavior.dragTo outside widget boundaries Some MouseEvent behaviors cannot be safely overridden because hooks for them do not yet exist Mar 17, 2017
@JordanMartinez
Copy link
Contributor

Closed by #468

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants