RFE implementation for #653 / #655 #656
Conversation
| private final ObjectProperty<Consumer<MouseEvent>> onNewSelectionDragEnd = new SimpleObjectProperty<>(e -> { | ||
| CharacterHit hit = hit(e.getX(), e.getY()); | ||
| moveTo(hit.getInsertionIndex(), SelectionPolicy.ADJUST); | ||
| }); |
There was a problem hiding this comment.
Looks like onNewSelectionDragEnd was properly implemented, but it seems you deleted it somewhere along the way and then had to re-add it?
There was a problem hiding this comment.
My bad for not explaining properly. Although implemented here, this code is not used as neither onNewSelectionDragEndProperty() nor getOnNewSelectionDragEnd() are referenced from anywhere as far as I could tell.
| * Indicates whether a new selection is being made by the user. | ||
| */ | ||
| private DragState dragNewSelection = DragState.NO_DRAG; | ||
|
|
There was a problem hiding this comment.
Could you explain why this is being added? If I recall correctly, the dragging behavior already works without adding a second one to track things.
There was a problem hiding this comment.
So this is being added to fix onNewSelectionDragEnd/Finished not being referenced as mentioned above. This isn't for the dragging of an existing selection, but rather for what happens at the end of creating a new selection via: click on unselected text, drag, release click sequence.
I did the changes just based off the variable name and existing code, before seeing the Java doc for this. I've noticed now that my understanding of this seems to be in conflict with the java doc, which in turn conflicts with itself. Compare the java doc on setOnSelectionDropped with setOnNewSelectionDragFinished, where they both say the same thing. So this is either a copy paste java doc that wasn't edited or a duplicate specification ?
There was a problem hiding this comment.
I looked through the repo's current code (not the changes you've submitted) and have concluded that we don't have a onNewSelectionFinished mouse hook. We have onNewSelectionDrag, which is similar but not the same thing. When a user presses the mouse outside of a selection, and then drags the mouse somewhere but does not release it, the newSelectionDrag hook gets called each time the mouse moves. However, once the mouse is released, there isn't a hook to indicate that the selection will not continue being updated. Hence, your support for this makes sense.
Initially, your approach for adding support for the onNewSelectionFinished via another DragState variable seemed unnecessary if one could just add a if (view.getSelection().getLength() != 0) { /* run hook */ } to the NO_DRAG case in the processMouseReleased() method. No need to take up additional memory when we already have another way of figuring that out.
However, when I looked through more of the code, I realized that there are some assumptions I am making that would lead to the code breaking if one overrode the mouse behavior code in a specific way. For example, on the first primary mouse button press, I assume that the developer will not override the onOutsideSelectionMousePress code, which clears any selection that may have existed before the mouse was pressed. Thus, when the user releases the mouse, no additional code needs to run (e.g. clear any selection) because of my assumption. Should someone override that part of the code, so that it doesn't clear the selection, my approach to adding such support would no longer work whereas yours still would.
All in all, it's tricky to design a default overrideable mouse behavior implementation because of its complexity (e.g. autoscrolling; whether there will be a selection at some point in the code's execution or not; etc.) and the lack of knowledge we have about how it will be overridden.
| EventHandler<MouseEvent> getOnOutsideSelectionMousePressed(); | ||
|
|
||
| /** Use setOnOutsideSelectionMousePress<u>ed</u>(<i>EventHandler>MouseEvent<</i>) instead, which is FXML compatible */ | ||
| @Deprecated default void setOnOutsideSelectionMousePress(Consumer<MouseEvent> consumer) { onOutsideSelectionMousePressProperty().set(consumer); } |
There was a problem hiding this comment.
Again, move these deprecated methods down to the bottom of the file.
|
Thanks for the feedback :-) I'll unfortunately only be able get to them on Sunday or Monday at the latest. |
|
No worries |
|
Just a quick question, but are you using GitHub directly to modify these files rather than an IDE? |
|
Yes that's right, I am using GitHub directly. |
|
Ok, thanks for clarifying. Mind sharing why you aren't using an IDE? How are you going to rebase your work into as few commits as possible? |
|
It's simply because I don't know / am unfamiliar with using GitHub through either an IDE or command line. So I'm not sure on the rebase .... I could do a PR from my branch to my master, squashing or rebasing along the way, and submit a new PR to you from that ? (So we close this one) Sorry, I didn't know that it would cause a mess this way :-( |
|
Well, your 3 choices of IDEs for Java are usually IntelliJ Idea Community Edition (free), Eclipse (free) and NetBeans (free). I use IntelliJ; others use Eclipse; I haven't come across a person who uses NetBeans as far as I can remember. I'd use this "problem" as an opportunity to learn something new. |
|
Thanks for the encouragement ! :-) I've been using Eclipse for a while, but since I'm a lone developer I've had no need to use git. Anyway I have tried once or twice in the past to use git from inside Eclipse, read tutorials, etc but I've just found the learning curve too steep to push through with it. I gave it shot again now and I'm not getting it right: got the project going in Eclipse, created and checked out a new branch, made changes to two files, committed, pushed branch, and synchronized - but I don't see my changes on GitHub ? Honestly, it just frustrates me - it shouldn't be this hard. |
|
Personally, I use my IDE to make changes and push those commits to my fork of this repo. Then I open up GitHub in a web browser, go to the main repo (FXMisc's repo), see a small banner that suggests opening a new PR with the recent changes I've pushed to my repo, and I do so by clicking on it's corresponding button. Once that PR is open, I can make additional changes and use Since you already have a PR open, you could just rebase all your commits in your IDE and force push those changes to your remote repo's "#653 - Phase 2" branch and it should update this PR. |
I'm a lone developer as well on some projects, but I can't fathom not using Git. What else would one use to track the changes they've made? I'm constantly experimenting and such experiments do not always go into the master branch. Or perhaps you are referring to using Git alongside of GitHub. That's a different matter if you aren't using GitHub to host your remote repo. |
|
I suppose I've just been coding since before git (2005), so I've got my own system that works for me :-) |
|
Any update on this @Jugen? |
|
Sorry, I've haven't been well. Will try and get to it this week. |
|
I hope you feel better. You could, but then we'd lose all the discussion here, unless you referred to it in the next PR. I would encourage you to use git to fix it if you plan on doing additional PRs in the future. If not, then just open a new PR that fixes the issues presented. |
|
@Jugen any updates on this? What did you decide to do? |
|
Sorry, still recovering and had to take some time off. |
|
Ok, I'm back again and am trying to finish this but am having some trouble getting it right. |
|
Looks like you need to invest more time into learning Git. Have you tried asking a question on StackOverflow about this? That would be a better place to figure issues of this kind out rather than on this PR. |
|
I've been on git IRC all day with no joy, and I've spent 2 days on this now. So unfortunately I think the best course of action is to close this PR and for me to submit a new one. BTW someone on the IRC did mention that
so I don't know if that is an option ? |
Well, thanks for putting in so much effort! That sounds terribly frustrating!
I checked and that is indeed an option. Whenever Tomas merged commits, it was always through the "Create a merge commit" option and that's what he directed me to do when maintaining this project. If anything, this reveals a lapse in my knowledge about GitHub because such an issue never arose in my experience. I'm going to take a deeper look at your code again since it's been a while since I've looked at it. |
|
Thanks :-) I think the "merge by squashing" button option is a fairly new feature that they added last year sometime if I recall correctly ? |
| else { | ||
| CharacterHit hit = hit(e.getX(), e.getY()); | ||
| moveTo(hit.getInsertionIndex(), SelectionPolicy.CLEAR); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I decided to move this conversation down a bit (delete originals and re-add them here) so that the full scope of the code could be more easily viewed in GitHub.
Originally, I wrote:
Could you move all deprecated methods down to the bottom of the file? It gets them out of the way.
You responded with:
Tried to do this, but then the compiler complains about an Illegal forward reference so I've reverted this back to the way it was. [Confused emoticon]
So, let's break down what's going on here...
// old method & name
@Deprecated private final ObjectProperty<Consumer<MouseEvent>> onOutsideSelectionMousePress
= new SimpleObjectProperty<>( null );
@Override public final ObjectProperty<Consumer<MouseEvent>> onOutsideSelectionMousePressProperty() {
return onOutsideSelectionMousePress;
}
// new method & name
private final ObjectProperty<EventHandler<MouseEvent>> onOutsideSelectionMousePressed
= new SimpleObjectProperty<>( e ->
{
// in order to support the deprecated method `onOutsideSelectionMousePressProperty` and
// their respective getter/setter methods, you need to run that method first if it's not null
if ( onOutsideSelectionMousePress.get() != null ) onOutsideSelectionMousePress.get().accept(e);
else {
// If it's null, then run the default behavior specified here
CharacterHit hit = hit(e.getX(), e.getY());
moveTo(hit.getInsertionIndex(), SelectionPolicy.CLEAR);
}
});Since you reference the onOutsideSelectionMousePress field directly rather than through its method, the field needs to be declared before its first reference. This explains your "illegal forward reference" issue since you put it at the bottom of the file. If you put the field declaration and its corresponding property method at the bottom of the file, (with the field's declaration occurring one line above its method), then the compiler won't raise that issue if you refer to the field not directly but indirectly through its property method.
However, I have to ask whether your approach is the best in terms of modifying the source code as little as possible. If a developer overrides the original press property, then the press property still gets run because the code actually runs the pressed property, which will then run the press property because it's non-null. If a developer overrides the new pressed property, then it gets rid of the null check and runs whatever custom code the developer specifies. So why not write it this way:
// Place this old code at the bottom of the file
// I only put it here to communicate my point easier
private final ObjectProperty<Consumer<MouseEvent>> onOutsideSelectionMousePress
= new SimpleObjectProperty<>(
// Add this comment to the source. No need to change anything here.
// Note: this code should be moved `onOutsideSelectionMousePressed
// in the next major release before removing this deprecated field
// due to a change in type (Consumer to EventHandler)
e -> {
CharacterHit hit = hit(e.getX(), e.getY());
moveTo(hit.getInsertionIndex(), SelectionPolicy.CLEAR);
});
@Deprecated
@Override public final ObjectProperty<Consumer<MouseEvent>> onOutsideSelectionMousePressProperty() {
return onOutsideSelectionMousePress;
}
// new method & name
private final ObjectProperty<EventHandler<MouseEvent>> onOutsideSelectionMousePressed
= new SimpleObjectProperty<>( e ->
{
// in order to support the deprecated method `onOutsideSelectionMousePressProperty` and
// their respective getter/setter methods, this handler just runs the property's code
// which is referenced through its property method, and not directly to avoid the illegal forward reference
onOutsideSelectionMousePressProperty().get().accept(e);
});Also, you deprecated the field, which was private and inaccessible anyway, but not the field's public property method, which is accessible and needs to be deprecated. I fixed that in my example above.
There was a problem hiding this comment.
I understand the need to "modify the source code as little as possible" and have no problem with your alternative.
I deprecated the private field for internal "documentation" purposes, and the public property wasn't because it's already deprecated in the ViewActions interface. :-)
|
All in all, your work has revealed a couple of things:
Here are the issues that still need to be resolved:
Although I could squash these commits into one commit once these things are resolved and the tests pass, doing so would break best practices because having smaller commits that change one thing at a time with clear messages would be best. Since your own attempt at squashing the commits hasn't worked and since you've already put a lot of work into this, would you be willing to open a new PR to fully implement all of these changes? If so, the commit history (earlier commits are at the top and the last commit is at the bottom) might look like:
If not, I can merge this using the squash option, explain what was accomplished in this PR, and resolve the remaining issues myself in a separate PR that cleans this one up a bit. |
|
No problem I'll make the changes you've requested .... |
|
Since I wasn't sure how familiar you are with TestFX, I wrote tests for the |
|
The Java 9 build failed whereas the Java 8 builds passed. I'm restarting the Java 9 build. It might just be a flaky test. If so, I can try to refactor it to get rid of the flakiness in a later PR. |
|
After restarting it, the Java 9 build failed again. I ran the tests on my own system using Java 9 and they pass. I've restarted it again. Hopefully, it will pass, but if it doesn't, I'll still merge this since it worked on my end. |
|
Correction. The Java 8 Mac build is failing, not the Java 9 Linux build. |
|
Thanks again for all your work @Jugen! You spent quite a lot of time (and emotions) on this. I hope it was a good opportunity to learn some new things along the way. |
|
Yes, and thank you for all the time and effort you put into RichTextFX too. |
|
You're welcome, although most of the credit of this work should go to Tomas since he developed all four of RichTextFX's dependencies as well as this project. |
RFE implementation for #655
The methods in
ViewActionsrelated to the following have been @Deprecated:They have been replaced with FXML friendly equivalent (non default) interface methods:
Which have been implemented in
GenericStyledArea. The references to the deprecated methods inGenericStyledAreaBehaviorhave been changed to the new ones.During this process it was found that the
onNewSelectionDragEnd/Finishedfunctionality hadn't been implemented inGenericStyledAreaBehaviorand this was rectified as well.