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

New gestures #249

Closed
wants to merge 3 commits into from
Closed

Conversation

andreynovikov
Copy link

This PR fixes gesture handling like the problem with long press still firing on drag zoom, lets adding new gestures and introduces two new gestures: triple tap and two finger tap for zoom out (last one is commonly used in popular mapping applications).

I need help in checking it on other platforms. For instance I do not know what to do with this:
https://github.com/mapsforge/vtm/blob/master/vtm-gdx/src/org/oscim/gdx/LayerHandler.java

Looks like it can be removed the same way as for Android but I am not sure. Can someone look at this PR on iOS and Windows?

@devemux86
Copy link
Collaborator

Thanks!

Will check it.
I see Travis failing, do you have any more work?

@devemux86
Copy link
Collaborator

The Android overlay gestures don't work now (e.g. MarkerOverlayActivity in vtm-android-example).
Why the remove of Android GestureHandler?

Regarding the LayerHandler it manages the overlay gestures on GDX platforms, see #151 (e.g. MarkerLayerTest in vtm-playground).

@devemux86
Copy link
Collaborator

BTW since all those are core features we should preferably implement / integrate them step by step.
Right now there are changes in too many places and not easy to examine which broke what functionality. 🙂

For example separately:

  • Fix long press
  • Add triple tap (what it does in VTM?)
  • Add two finger tap zoom out
  • Remove carefully redundant classes (after thorough testing)
  • ...

@andreynovikov
Copy link
Author

This can not be done in steps as this is all about replacing Android gesture handler.

I'll have a look at MarkerOverlayActivity

@devemux86
Copy link
Collaborator

Well I guess first we need to discuss why we need to replace something.. we don't even have an open issue for a gesture bug?
And then proceed fix by fix and gesture by gesture. No need all done simultaneously.
That certainly makes our lives difficult and the testing / integration taking forever.

@andreynovikov
Copy link
Author

What exactly does not work in MarkerOverlayActivity? It sets marker and shows toast.

@devemux86
Copy link
Collaborator

devemux86 commented Nov 22, 2016

The overlays have gestures managed in Android by GestureHandler, while the MapEventLayer handles the map events (not overlays).

Now first one or two tapped markers work (changed icons) and then with the rest nothing.

@devemux86
Copy link
Collaborator

  • I updated the marker samples on Android and Desktop to include also overlay long press events
  • At Android samples I added also events on map (tap + long press)
  • In Android: Quick scale vs Long press #250 I pushed a fix for the quick scale vs long press issue

@andreynovikov
Copy link
Author

I have fixed all issues. The major problem is that events are reused. Both by Android framework and VTM. It leads to double copying of objects to make them available for reference in future. I do not like this approach at all. So if you wish not to merge this I will just implement everything I need in my application using native Android methods.

@devemux86
Copy link
Collaborator

Two cases here:

  • Generic: any major / core changes should be discussed first for all to understand why / how we should modify some library's API or behavior. Also PRs should contain the absolutely minimum changes per fix, improvement, one feature each time, i.e. better multiple small PRs than one complicated and difficult to debug. 🙂
  • Specific: since VTM is a multi-platform library, some things live in the core (like the map gestures) and some are exposed via each platform's native API. For example we handle differently the overlay gestures on Android and libGDX in the two handlers, but the events themselves come from the platforms, e.g. we don't re-write the marker long-press gesture or delay from the start.

So here we need first to clarify what we want to fix or change or add and then go for it.

  • Are there any bugs in gestures (after Android: Quick scale vs Long press #250)? We should fix them separately.
  • Are there any duplicated uses? We should report, check them and examine why this happens.
  • Which are in detail the new gestures we want to introduce? We could discuss the procedure and add them one at a time, for better testing.

Finally VTM (like Mapsforge) is an open source library affecting many users. It's not meant to satisfy only some developers needs, but all who use it. And they use it in many platforms not just Android. It should progress in time and with good testing without haste.

@devemux86
Copy link
Collaborator

@andreynovikov really appreciating your contributions 🙂 best to integrate it as a 2nd MapEventLayer.
We leave existing and side classes unmodified and set a selection in MapView initialization.
It's been done before and would permit us having convenient progress & testing by all users.

@devemux86
Copy link
Collaborator

devemux86 commented Nov 24, 2016

I have a first draft of the two MapEventLayer working quite well, will push probably later.

BTW testing the GDX events with the PR, I see them coming two times, e.g. MarkerLayerTest.
Edit: I fixed the GDX double events by skipping the LayerHandler in case of new gestures.

@devemux86 devemux86 added this to the 0.7.0 milestone Nov 25, 2016
@devemux86 devemux86 mentioned this pull request Nov 25, 2016
devemux86 pushed a commit that referenced this pull request Nov 25, 2016
@devemux86
Copy link
Collaborator

devemux86 commented Nov 25, 2016

Hmm, tried the PR on latest VTM and GTW complains (see Travis):

No source code is available for type java.util.TimerTask
No source code is available for type java.util.Timer

@andreynovikov
Copy link
Author

How can it be? It was introduced in Java 5:
http://docs.oracle.com/javase/7/docs/api/java/util/TimerTask.html

@devemux86
Copy link
Collaborator

devemux86 commented Nov 25, 2016

GWT includes a library that emulates a subset of the Java runtime library, e.g. see for java.util.

@andreynovikov
Copy link
Author

It looks like VTM has some kind of cancelable tasks:
https://github.com/mapsforge/vtm/blob/master/vtm/src/org/oscim/utils/async/Task.java
Didn't know that. If they can be post delayed it should be better to use them for consistency.

@devemux86
Copy link
Collaborator

devemux86 commented Nov 25, 2016

Indeed, will you make changes?
Then could you do them in issue_249 branch's MapEventLayer2 containing the PR.

Edit: New gestures can be enabled with Map.NEW_GESTURES flag.

@andreynovikov
Copy link
Author

Ok

@devemux86
Copy link
Collaborator

There are also some post methods in Map class that could be used.

@andreynovikov
Copy link
Author

Success: #260

devemux86 pushed a commit that referenced this pull request Nov 28, 2016
@devemux86
Copy link
Collaborator

Thanks!
Squashed and merged via 71f7c45.

@devemux86 devemux86 closed this Nov 28, 2016
@andreynovikov andreynovikov deleted the gestures branch November 28, 2016 12:51
@devemux86 devemux86 changed the title Native gestures New gestures Feb 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants