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

fixing bitbucket #748 https://bitbucket.org/controlsfx/controlsfx/iss… #1202

Merged
merged 4 commits into from Dec 16, 2019

Conversation

@FlorianKirmaier
Copy link
Contributor

FlorianKirmaier commented Oct 29, 2019

original pull request: #1200
the original pull request was a bit buggy, so I fixed it and added unit tests

The new change adds unit-tests for memory leaks.
It also adds a small Utilityclass for detecting memory leaks. Actually, I plan to make a library for unittesting for memoryleaks. But for this change, this utility class should be enough.

Calling configureButton twice on the same button, without unconfiguring the old Action is now forbidden and causes an Exception. This change was not, because the previous behaviour let to memory leaks and unpredictable behavior of the application.

@FlorianKirmaier
FlorianKirmaier committed 15 hours ago
…ues/748/memory-leak-in-actionutils-permanent

fixing bitbucket #748 https://bitbucket.org/controlsfx/controlsfx/issues/748/memory-leak-in-actionutils-permanent

It also contains a unit-test which checks for memoryleaks.
@FlorianKirmaier FlorianKirmaier force-pushed the FlorianKirmaier:master branch from a340ef1 to c5f4ef9 Oct 30, 2019
@FlorianKirmaier

This comment has been minimized.

Copy link
Contributor Author

FlorianKirmaier commented Oct 30, 2019

The last version changed every line in ActionUtils because of different whitespaces.
I've fixed it and forced pushed the fixed version

@FlorianKirmaier

This comment has been minimized.

Copy link
Contributor Author

FlorianKirmaier commented Nov 8, 2019

As mentioned, i moved the unittest class to an library which can now be used by different projects.
The library is named JMemoryBuddy
This should simplify the pullrequest.
The library also contains some unit-tests to verify this kind of testing is working fine.
It's also tested against openjdk13 with travis

@Maxoudela

This comment has been minimized.

Copy link
Collaborator

Maxoudela commented Nov 18, 2019

Sorry for the delay.
Apart from the copyright header in the test file, the whole thing seems good.

I don't know the policy on foreign imports but I think it would be shame to remove tests because we don't want to import external libraries. This could open the door to importing TestFX library that is very nice for testing JavaFX application.

@abhinayagarwal What is your opinion on that?

Updated JMemoryBuddy.
@FlorianKirmaier

This comment has been minimized.

Copy link
Contributor Author

FlorianKirmaier commented Nov 21, 2019

Great, I've just added the copyright header and I've updated JMemoryBuddy/improved the naming of the methods.

@Maxoudela

This comment has been minimized.

Copy link
Collaborator

Maxoudela commented Nov 22, 2019

Thanks for the PR, have you signed the CLA as mentioned in the Contributing doc?

@FlorianKirmaier

This comment has been minimized.

Copy link
Contributor Author

FlorianKirmaier commented Nov 26, 2019

I've just signed it!

@Maxoudela

This comment has been minimized.

Copy link
Collaborator

Maxoudela commented Nov 29, 2019

Thank you. On my side, this seems good but I'll other reviewers (@abhinayagarwal ?) give their opinion on the third party for test

@abhinayagarwal

This comment has been minimized.

Copy link
Member

abhinayagarwal commented Dec 12, 2019

Since this PR adds a "test" scope dependency and has minimum impact on the project, I am OK with this.

@FlorianKirmaier Can you please the conflict and update the header on ActionUtils.java ?

@abhinayagarwal

This comment has been minimized.

Copy link
Member

abhinayagarwal commented Dec 12, 2019

@FlorianKirmaier Does you library work with JDK 11? Just wondering if this addition will still be useful on 9.0.0 branch?

@FlorianKirmaier

This comment has been minimized.

Copy link
Contributor Author

FlorianKirmaier commented Dec 12, 2019

@abhinayagarwal
Yes, JMemoryBuddy is tested against 8, 11 and 13.
Currently, no setup is known, which doesn't work with the library.

@abhinayagarwal

This comment has been minimized.

Copy link
Member

abhinayagarwal commented Dec 13, 2019

@FlorianKirmaier That's great. Please resolve the conflict so that we can merge this PR.

@abhinayagarwal abhinayagarwal merged commit 989f48e into controlsfx:master Dec 16, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.