Skip to content

Allow area to display multiple carets and selections#687

Merged
JordanMartinez merged 13 commits intoFXMisc:masterfrom
JordanMartinez:multiCaretSelection
Feb 23, 2018
Merged

Allow area to display multiple carets and selections#687
JordanMartinez merged 13 commits intoFXMisc:masterfrom
JordanMartinez:multiCaretSelection

Conversation

@JordanMartinez
Copy link
Copy Markdown
Contributor

@JordanMartinez JordanMartinez commented Feb 14, 2018

Resolves #222.

There are a few questions that still need to be answered:

  • How should the caret's bounds on screen be calculated? The old approach would force the ParagraphBox and ParagraphText to layout their respective contents before querying the bounds. Does this still need to be done or will the bounds be inaccurate? Also, how should one determine whether the caret is outside of the viewport (i.e. it's parent was removed) and distinguish that event from one in which the caret is moved to another paragraph within the viewport?
    • The old approach still works. I couldn't get it to work previously because there was a very subtle bug that had to be fixed first.
  • How should the area's pseudo class has caret be handled now? Do we rename that pseudo class to has-main-caret and add another pseudo class has-a-caret for each ParagraphBox, so that the first one applies when it has the main caret and the second one applies when it has any non-main caret?
    • This should go under the scope of a different issue/PR.
  • Should the main caret be prevented from being removed? Others might wish to use their own custom caret as the main caret. If allowed, how would the default behavior work since the caret is used considerably for that? If prohibited, how should it be prevented from being removed?
    • This requires special handling as one cannot just replace the caret with their own version. They need to replace it with a CaretSelectionBind, so that the default behavior still works since it updates the selection, which propagates changes to the caret.
    • This should also go under the scope of a different issue/PR.
  • How should a caret be distinguishable from another caret? I'm currently using a List (as it makes it easier to implement ObservableList via ModifiableObservableListBase, which allows the list change listeners), but this allows the same caret to be added twice. Thus, I've added additional checking to insure that can't occur. However, the better structure to use would be a Set, but I don't think they have an ModifiableObservableSetBase class, making such an implementation its own larger issue. Since there isn't one distinguishable thing about a given caret, how should they be equaled/hashed to be different? One idea is to name them.
    • Naming them seems simple enough.

Currently, the bounds related tests fail in this PR. I'm not sure whether the tests should be changed or the bounds intialization/updating should be changed.
All tests currently pass on Linux. Not sure about Windows/Mac

Also, I still need to write tests for this.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

Looks like some more tests need to be run via the JFX App Thread, not the test thread

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

The multiple selections doesn't quite work yet since it doesn't properly remove the selections from the area. Also, some tests fail because they aren't being run on the JFX App thread. I'll fix that on another day.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

JordanMartinez commented Feb 21, 2018

I've updated the tests so that they pass. However, 2 tests do not pass. I ran these tests on code from various commits in the project that do not include any commits from this PR and they fail there, too. I'm left wondering whether a newer Java version broke something.

When I watch the two failing tests on my screen, the TextFlow/mouse doesn't seem to work correctly

@JordanMartinez JordanMartinez changed the title Allow area to display multiple carets Allow area to display multiple carets and selections Feb 21, 2018
@JordanMartinez
Copy link
Copy Markdown
Contributor Author

I'm left wondering whether a newer Java version broke something.

Travis CI is using Java 8u151 still in this build and in a prior build that passed. So this really is an issue introduced somewhere in this PR.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

JordanMartinez commented Feb 22, 2018

Things left to do:

  • Write much clearer javadoc for various things
  • Write tests for the multiple caret/selection code
  • Add demo for multi caret/selection

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

During this process, I learned that Selection#selectionBoundsProperty throws an exception because it tries to calculate itself before the area is finished updating. I've corrected it in this PR as well.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

This is ready to be merged. I would appreciate any feedback on this.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for multiple carets and selection ranges

1 participant