[android] Differentiate requestCode for PendingIntent #9198

rtsisyk · 2024-09-01T23:14:07Z

RicoElectrico commented

2024-09-01 23:14:07 +00:00

(Migrated from github.com)

Fixes a bug when tapping the track recorder notification does not return to Organic Maps if there was a navigation session and its corresponding notification.

Steps to reproduce the issue this PR fixes:

Enable track recording
Start navigation
Exit navigation
Switch OM into background
Tapping the track recording notification will do nothing instead of opening OM

Fixes a bug when tapping the track recorder notification does not return to Organic Maps if there was a navigation session and its corresponding notification. Steps to reproduce the issue this PR fixes: 1. Enable track recording 2. Start navigation 3. Exit navigation 4. Switch OM into background 5. Tapping the track recording notification will do nothing instead of opening OM

👍 2

rtsisyk reviewed 2024-09-01 23:14:07 +00:00

biodranik (Migrated from github.com) reviewed 2024-09-02 10:52:56 +00:00

android/app/src/main/java/app/organicmaps/downloader/DownloaderNotifier.java

biodranik (Migrated from github.com) commented

2024-09-02 10:52:56 +00:00

Thanks! What do these codes/integers mean? Are they specified somewhere else? Do they need some constants?

Another question: what are the steps to reproduce the issue you're fixing? Can you please write them in the PR description (or create an issue)?

Thanks! What do these codes/integers mean? Are they specified somewhere else? Do they need some constants? Another question: what are the steps to reproduce the issue you're fixing? Can you please write them in the PR description (or create an issue)?

RicoElectrico (Migrated from github.com) reviewed 2024-09-02 11:20:30 +00:00

android/app/src/main/java/app/organicmaps/downloader/DownloaderNotifier.java

RicoElectrico (Migrated from github.com) commented

2024-09-02 11:20:30 +00:00

I updated the PR description. These numbers are pretty much arbitrary and identify pending intents. In our case the requestCode is not used elsewhere in the app as pending intents are not further manipulated. If they're set to the same number and the target intent is the same, they will be overwritten by each other. This change makes the 3 pending intents take separate identities instead.
If you think that using constants will be more clear, that's a valid concern and I'm going to update the PR.

I updated the PR description. These numbers are pretty much arbitrary and identify pending intents. In our case the requestCode is not used elsewhere in the app as pending intents are not further manipulated. If they're set to the same number and the target intent is the same, they will be overwritten by each other. This change makes the 3 pending intents take separate identities instead. If you think that using constants will be more clear, that's a valid concern and I'm going to update the PR.

biodranik (Migrated from github.com) reviewed 2024-09-02 13:00:34 +00:00

android/app/src/main/java/app/organicmaps/downloader/DownloaderNotifier.java

biodranik (Migrated from github.com) commented

2024-09-02 13:00:34 +00:00

Should they each time be unique/random? Or the same for the same use-case (to override only itself)? If the latter, then something like PendingIntentId.DownloadNotification would be clearer.

biodranik (Migrated from github.com) reviewed 2024-09-02 22:13:21 +00:00

android/app/src/main/java/app/organicmaps/routing/NavigationService.java

					
				@ -46,6 +46,7 @@ public class NavigationService extends Service implements LocationListener

				{

				  private static final String TAG = NavigationService.class.getSimpleName();

				  private static final String STOP_NAVIGATION = "STOP_NAVIGATION";

				  private static final int REQ_CODE_STOP_NAVIGATION = 1;

biodranik (Migrated from github.com) commented

2024-09-02 22:13:21 +00:00

Should these codes be unique compared to MwmActivity ones? If yes, then it would be safer to keep all such codes in a single file/place, to avoid accidentally reusing the same code for different notifications. WDYT?

RicoElectrico (Migrated from github.com) reviewed 2024-09-02 22:29:33 +00:00

android/app/src/main/java/app/organicmaps/routing/NavigationService.java

					
				@ -46,6 +46,7 @@ public class NavigationService extends Service implements LocationListener

				{

				  private static final String TAG = NavigationService.class.getSimpleName();

				  private static final String STOP_NAVIGATION = "STOP_NAVIGATION";

				  private static final int REQ_CODE_STOP_NAVIGATION = 1;

RicoElectrico (Migrated from github.com) commented

2024-09-02 22:29:33 +00:00

I found another cleaner way to deal with the issue. As all the pending intents do the same thing, one can use FLAG_UPDATE_CURRENT which will not cancel the pending intent from the previous invocation. As a result all the services hold a reference to the same pending intent doing the desired thing i.e. opening OM in the foreground.
I think we need an Android expert to comment on what is considered best practice.

I found another cleaner way to deal with the issue. As all the pending intents do the same thing, one can use FLAG_UPDATE_CURRENT which will not cancel the pending intent from the previous invocation. As a result all the services hold a reference to the same pending intent doing the desired thing i.e. opening OM in the foreground. I think we need an Android expert to comment on what is considered best practice.

kavikhalique (Migrated from github.com) reviewed 2024-09-05 22:09:00 +00:00

android/app/src/main/java/app/organicmaps/routing/NavigationService.java

					
				@ -46,6 +46,7 @@ public class NavigationService extends Service implements LocationListener

				{

				  private static final String TAG = NavigationService.class.getSimpleName();

				  private static final String STOP_NAVIGATION = "STOP_NAVIGATION";

				  private static final int REQ_CODE_STOP_NAVIGATION = 1;

kavikhalique (Migrated from github.com) commented

2024-09-05 22:09:00 +00:00

I will go with FLAG_UPDATE_CURRENT

As android suggests to use FLAG_CANCEL_CURRENT only if you want to invalidate all other instances of pending intent by any service but here our logic is not to invalidate previous instances and also using it will remove the problem of similar pending intent creation by mistake.

I will go with FLAG_UPDATE_CURRENT As android suggests to use FLAG_CANCEL_CURRENT only if you want to invalidate all other instances of pending intent by any service but here our logic is not to invalidate previous instances and also using it will remove the problem of similar pending intent creation by mistake.