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

Add/Android Supporting sending & receiving geo: intents #812

Merged
merged 74 commits into from
Oct 23, 2019

Conversation

lains
Copy link
Contributor

@lains lains commented Jul 20, 2019

This PR adds 2 features to navit for Android:

  • When a geo: intent is started on Android, for example by scanning a geo QR code, navit is able to takes these coordinates as input

  • When a specific point is clicked on the map, this PR adds a new item in the contextual menu: "View". By clicking on that item (and if there are applications to handle it) a geo intent is generated in Android

lains and others added 30 commits May 15, 2019 10:32
…s_geo()

And fixing the example strings to match what is actually output by the function
…rmat

And add internal coord_format_with_sep() to specify the separator
…rt format in pcoord_format_short() and coord_geo_format_short()
@lains
Copy link
Contributor Author

lains commented Oct 7, 2019

Hi @jandegr, I could not reproduce the issue, but I'm sure I've noticed before this feeling of multiple instances running.
I have implemented the changes you suggested, and the singleTask is indeed a singleton, so this is probably what is needed.
Please let me know if this new version of the code fixes it.

@lains
Copy link
Contributor Author

lains commented Oct 7, 2019

I could not reproduce the issue, but I'm sure I've noticed before this feeling of multiple instances running.

Actually, I just reproduced the issue on a Samsung device running Android 4.4.2.
And there, it seems fixed by the singleTask launch mode...

@lains
Copy link
Contributor Author

lains commented Oct 7, 2019

And I also restested the intent loopback issue (crash) after I rebased on the code from trunk including PR877 and it did not crash. So, I think you already fixed the crash in that PR.

@lains lains removed the WIP Work in progress. Do not merge as-is, but please contribute label Oct 7, 2019
@jandegr
Copy link
Contributor

jandegr commented Oct 7, 2019

I will take some time to test it soon, but even without testing I suspect it now indeed sticks to being a singleton, but since you did not implement onNewIntent(), it will just recreate navit and that is not what we want. I might be wrong, but I just think it will now show in the log it takes the onCreate() route (or maybe, but probably not, the onRestart() route) since you don't open the onNewIntent entrypoint. At least it is what the API makes me think.

THX

@lains lains added the WIP Work in progress. Do not merge as-is, but please contribute label Oct 8, 2019
@lains
Copy link
Contributor Author

lains commented Oct 8, 2019

I did some additional testing, and the execution flow actually doesn't go through onCreate() but it is even worse because the intent now is not handled properly.

When navit is initially started from a geo intent, the sequence is:

onCreate()
**1**A android.intent.action.VIEW
**1**D geo:48.709807,15.193542
OnStart()
OnResume()
**2**A android.intent.action.VIEW
**2**D geo:48.709807,15.193542
target found (b): 48.709807,15.193542

But when navit is already running and resumed with a new geo intent, we go through:

OnRestart()
OnStart()

But does not take the intent into account... it keeps the previous destination, if any, so the intent reception feature is actually not working properly anymore.
I'll have a look into this. Probably, as suggested, I need to implement onNewIntent() and also decode the intent there... and do as what is currently done inside onCreate() (intent is cached inside member variable sStartupIntent, then processed later on in onResume())

To be continued...

@lains lains mentioned this pull request Oct 13, 2019
@lains lains removed the WIP Work in progress. Do not merge as-is, but please contribute label Oct 14, 2019
@lains
Copy link
Contributor Author

lains commented Oct 14, 2019

I have tested on this new code and receiving a geo intent now works as expected (starts navigating to the coordinates provided) even when navit is already running.
If navit was inside a menu when the intent is received, it remains inside that menu. Not sure whether this should be kept like that or if map should be displayed immediately and current menu dropped.

I am already planning to build some future work on this code base, opening a contextual menu based on the geolocation extracted from the intent, so that the exact action to perform on the input coordinates can be selected by the user, exactly the same way as when coordinates are input using the Action>Coordinates menu in the internal GUI...
But I would rather not add more code to the current PR and handle this in a future PR as explained in a previous comment.

@jandegr already told me sending geo intents is working but I fixed receiving geo intents since then, so I'd be happy if someone can test and give me feedback on this feature.

Shall I merge?

Thanks!

@jandegr
Copy link
Contributor

jandegr commented Oct 18, 2019

Hi,
There are failing scenario's for the existing google.navigation intent in the existing codebase and your work just inherits those.
There are several ways to deal with this and the way that not turns this into an endless story for you is you commit this and I will apply the fixes afterwards together with some other rework. You will see some of your lines being shuffled around afterwards.
The other way is you change all of it into a solution working in every scenario for both the intents.

your choice.

@jandegr
Copy link
Contributor

jandegr commented Oct 21, 2019

@lains pls. merge this since future work depends on it.
After merging I can post another (but smaller this time) Android rework and then you can proceed with the future extensions you mentioned whenever it suits you.

THX

@pgrandin pgrandin merged commit 7021575 into navit-gps:trunk Oct 23, 2019
@jandegr jandegr mentioned this pull request Oct 24, 2019
@lains
Copy link
Contributor Author

lains commented Oct 24, 2019

Great. Thanks @pgrandin for merging this.
@jandegr, yes, I will wait for #919 before doing further work on the view intent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants