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

feature/#1418 - custom zoom refactoring while removing the old standard zoom controllers #1419

Merged
merged 2 commits into from Oct 14, 2019

Conversation

@monsieurtanuki
Copy link
Collaborator

monsieurtanuki commented Oct 9, 2019

Impacted classes:

  • AsyncTaskDemoFragment, CustomRecycler, SampleFollowMe, SampleHeadingCompassUp, SampleSplitScreen, WeathForceActivity: removed a useless call to now deprecated method MapView.setBuiltInZoomControls, which used to be the default behavior anyway
  • MapView: removed all the references on the deprecated Android zoom controls; moved the calls to zoom control test in dispatchTouchEvent; deprecated method setBuiltInZoomControls; unrelated minor refactoring
  • CustomZoomButtonsController: made public method isTouched which uses new method CustomZoomButtonsDisplay.isTouched; deprecated method onSingleTapConfirmed and onLongPress
  • CustomZoomButtonsDisplay: deprecated method isTouchedRotated as we now check the touch event before applying the rotation; created method isTouched(MotionEvent,boolean) for the same reasons; unrelated minor refactoring
…rd zoom controllers

Impacted classes:
* `AsyncTaskDemoFragment`, `CustomRecycler`, `SampleFollowMe`, `SampleHeadingCompassUp`, `SampleSplitScreen`, `WeathForceActivity`: removed a useless call to now deprecated method `MapView.setBuiltInZoomControls`, which used to be the default behavior anyway
* `MapView`: removed all the references on the deprecated Android zoom controls; moved the calls to zoom control test in `dispatchTouchEvent`; deprecated method `setBuiltInZoomControls`; unrelated minor refactoring
* `CustomZoomButtonsController`: made `public` method `isTouched` which uses new method `CustomZoomButtonsDisplay.isTouched`; deprecated method `onSingleTapConfirmed` and `onLongPress`
* `CustomZoomButtonsDisplay`: deprecated method `isTouchedRotated` as we now check the touch event before applying the rotation; created method `isTouched(MotionEvent,boolean)` for the same reasons; unrelated minor refactoring
@InI4

This comment has been minimized.

Copy link

InI4 commented Oct 11, 2019

Works for me. I.e. I do not fully understand it, but it solves the issues solved by #1414 .

Remark: I needed to run the build with

gradlew -x testReleaseUnitTest :osmdroid-android:publishToMavenLocal

to sidestep failing tests.

org.osmdroid.util.GarbageCollectorTest > testSecond FAILED
    junit.framework.AssertionFailedError at GarbageCollectorTest.java:51
@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator Author

monsieurtanuki commented Oct 12, 2019

@InI4 Actually it works because it's just what you coded in #1414 ("moving the zoom ontouch detection test"), but with more impact checks and refactoring.
I thought it made no sense to accept #1414 as it was (didn't work with orientated maps for instance) nor to reject it again and again with new justifications ("oh you should also do this and that"). My point was not to take the credits of this fix from you, it was to fix comprehensively the custom zoom controller that I painfully coded.

That being said, I've had a look at the (unrelated) crashing unit test.
It works OK for me, and it looks like "bad luck crash" in your case.
There are threads involved in this unit test, one being with low priority, and the unit test is about checking if the thread is over after n milliseconds. In your case I guess that the thread started later (because of the low priority), therefore is not over after n milliseconds of the main thread. Thence the crash.
I'm working on it.

Impacted class:
* `GarbageCollectorTest`
@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator Author

monsieurtanuki commented Oct 12, 2019

@InI4 I've just fixed the unit test. Can you please try again?

@InI4

This comment has been minimized.

Copy link

InI4 commented Oct 13, 2019

I can confirm, the unit test is now also running smoothly here.

Am I supposed to close #1414 now?

@monsieurtanuki monsieurtanuki merged commit 795171a into master Oct 14, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@monsieurtanuki monsieurtanuki referenced this pull request Oct 21, 2019
1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.