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

select moved text after drag-and-drop #576

Merged
merged 3 commits into from
Sep 12, 2017

Conversation

JFormDesigner
Copy link
Contributor

When doing drag-and-drop then the moved text was not selected (as in other text editors).

@JordanMartinez
Copy link
Contributor

Could you update the failed tests so that they don't fail? Then I'll merge this.

@JFormDesigner
Copy link
Contributor Author

Tried to fix the failed test, but on my development machine about 50% of all tests in ClickAndDragTests fail. So I made the change without testing it, pushed it and thought that Travis runs the tests again, but it did not yet start.

Could you trigger a build at Travis? There is a Trigger build in More options menu (if you're logged into Travis).

Any idea why that many test fail on my machine?
Have a 40inch 4K Monitor running at 150% scale under Windows 10.
Can the larger font size cause the failures?

@JordanMartinez
Copy link
Contributor

Restarted the build

@JordanMartinez
Copy link
Contributor

Can the larger font size cause the failures?

Probably. Some tests need a specific window size and font size so that the text is laid out a certain way and the caret is then moved to a specific position.

Are you using Windows or Mac to test things?

@JordanMartinez
Copy link
Contributor

Not sure if the test is flaky or if the conditional loop that adjusts the pos value changes something. It could be that the test is just poorly written and doesn't actually test what we want it to test.

@JordanMartinez
Copy link
Contributor

@JFormDesigner Can you rebase this on top of the current master? I'm curious if updating to XCode 9 will make this build pass.

@JFormDesigner
Copy link
Contributor Author

OK, rebased and fixed tests.

The Y parameter passed to moveBy() method was too small for macOS.

I've also made the asserts more precise.

Doing something like assertTrue(caretPos != area.getCaretPosition()); is IMHO too vague and should be avoided in unit tests. E.g. the test pressingMouseOnSelectionAndDraggingDisplacesCaret succeeded on macOS although the caret was at position 0 and not as expected at the beginning of the second line.

@JordanMartinez JordanMartinez merged commit 78fcff1 into FXMisc:master Sep 12, 2017
@JordanMartinez
Copy link
Contributor

Doing something like assertTrue(caretPos != area.getCaretPosition()); is IMHO too vague and should be avoided in unit tests. E.g. the test pressingMouseOnSelectionAndDraggingDisplacesCaret succeeded on macOS although the caret was at position 0 and not as expected at the beginning of the second line.

I agree. However, when I wrote the integration tests, I was simply trying to write enough tests to prove that the code worked for the most part. (Let's be honest: it's better than not having them in the first place as that's what caused #472 to go undetected until after the release was made) It wasn't my goal to make them super precise, and no doubt there are other tests whose accuracy could be improved.

@JFormDesigner JFormDesigner deleted the select-dropped-text branch September 12, 2017 16:21
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.

3 participants