Update PdService.java #120

rafaelgustavo123 · 2022-05-15T19:49:58Z

PendingIntent bug on android 12. Please, add this changes.

tkirshboim

Thanks for the contribution!

I assume that this PR addresses this issue that was raised on stackoverflow.
In the future, please provide more information on why you wish to implement a certain change.

I'd like to get @joebowbeer approval before merging this PR.

PdCore/src/main/java/org/puredata/android/service/PdService.java

tkirshboim · 2022-05-17T07:49:47Z

PdCore/src/main/java/org/puredata/android/service/PdService.java

+                	pi = PendingIntent.getActivity(getApplicationContext(), 0, intent, PendingIntent.FLAG_UPDATE_CURRENT | PendingIntent.FLAG_IMMUTABLE);
+            	}
+            	else {
+                	pi = PendingIntent.getActivity(getApplicationContext(), 0, intent, PendingIntent.FLAG_UPDATE_CURRENT);


What is the reason for changing the value that was previously used (0) to PendingIntent.FLAG_UPDATE_CURRENT?

Suggested change

pi = PendingIntent.getActivity(getApplicationContext(), 0, intent, PendingIntent.FLAG_UPDATE_CURRENT | PendingIntent.FLAG_IMMUTABLE);

}

else {

pi = PendingIntent.getActivity(getApplicationContext(), 0, intent, PendingIntent.FLAG_UPDATE_CURRENT);

pi = PendingIntent.getActivity(getApplicationContext(), 0, intent, PendingIntent.FLAG_IMMUTABLE);

}

else {

pi = PendingIntent.getActivity(getApplicationContext(), 0, intent, 0);

In my experience it doesn't change much, but we can keep 0 to be more like the original code.
https://stackoverflow.com/questions/50128188/what-are-different-between-pendingintent-flag-update-current-and-0-in-android

Let's keep 0 then for now.

This might be clearer with a helper to compose the flag(s)

joebowbeer · 2022-05-17T10:28:34Z

Fixes #121 ?

joebowbeer · 2022-05-17T10:51:29Z

See SO for background and link to article: https://stackoverflow.com/q/67045607/901597

joebowbeer · 2022-05-17T11:20:50Z

@tkirshboim our git submodules are not syncing due to deprecated protocol as reported #118

Deprecation: https://github.blog/2021-09-01-improving-git-protocol-security-github/

I created a test PR #122 but it is failing further along; we also need to update our link to libpd in order to pickup the .gitmodules changes there.

libpd/libpd@df45c75

Still working on fixing the build. Now stuck on some recently added double precision support...

Co-authored-by: Tal Kirshboim <[email protected]>

rafaelgustavo123 · 2022-05-17T12:09:13Z

Fixes #121 ?

yes!

cerupcat · 2022-05-20T19:41:38Z

Is there any additional work to be done here? I'm aiming to get this up and running on 31+ so would like to help get this merged in if it's ready.

tkirshboim · 2022-05-20T20:20:29Z

@cerupcat This still needs to be done.

FunkyMuse · 2022-06-08T15:11:14Z

@cerupcat This still needs to be done.

As I can see that the question is whether to keep the zero or not, in this case it's smarter to keep the zero because you do not have any custom request code meaning zero is no request codes at all, unless the service does something specific and obtains it through that request code then and only then it makes sense to have it custom one.

residuum

Works for me here with latest Android Studio and Android 32.

FunkyMuse · 2023-06-13T18:41:57Z

hey guys, any chance on getting this merged?

cerupcat · 2023-06-13T21:23:14Z

@joebowbeer @tkirshboim Any chance we can get this merged? We're like to integrate this change.

joebowbeer · 2023-06-14T04:33:21Z

@cerupcat This PR looks good to me, but the build fails.

@tkirshboim I think we're still stuck on nettoyeurny/opensl_stream#5 via libpd/libpd#357

Ideas?

FunkyMuse · 2023-06-14T07:05:11Z

@cerupcat This PR looks good to me, but the build fails.

@tkirshboim I think we're still stuck on nettoyeurny/opensl_stream#5 via libpd/libpd#357

Ideas?

I'm not sure how they're related 🤔

This is only a flag to prevent crashes when it comes to the service

tkirshboim · 2023-06-14T07:25:39Z

This also still needs to be done. @rafaelgustavo123 Could you take a look?

joebowbeer · 2023-06-14T08:26:59Z

@FunkyMuse writes

This is only a flag to prevent crashes when it comes to the service

Until the build is fixed, I guess the other option would be to make a point release based on an older tag.

FunkyMuse · 2023-06-14T11:45:23Z

@FunkyMuse writes

This is only a flag to prevent crashes when it comes to the service

Until the build is fixed, I guess the other option would be to make a point release based on an older tag.

It'll be of a great help, since targeting newer SDKs is a requirement from Google nowadays

FunkyMuse · 2023-06-21T20:13:38Z

@FunkyMuse writes

This is only a flag to prevent crashes when it comes to the service

Until the build is fixed, I guess the other option would be to make a point release based on an older tag.

can we get a release, I can see the PR is merged for #126

joebowbeer · 2023-06-22T06:35:57Z

@FunkyMuse writes

This is only a flag to prevent crashes when it comes to the service

Until the build is fixed, I guess the other option would be to make a point release based on an older tag.

can we get a release, I can see the PR is merged for #126

It may be a while before we release to maven central, but there is a recent snapshot build

https://s01.oss.sonatype.org/content/repositories/snapshots/io/github/libpd/android/pd-core/1.2.1-SNAPSHOT/

And the aar artifact can be downloaded from the GitHub Action:

https://github.com/libpd/pd-for-android/actions/runs/5283412584

Perhaps one of these will work in the interim?

cerupcat · 2023-07-05T19:26:26Z

It appears that snapshot also has the crash issue.

joebowbeer · 2023-07-07T04:49:24Z

It appears that snapshot also has the crash issue.

What crash issue?

The current code now provides FLAG_IMMUTABLE, right?

Is the error message still the same?

FunkyMuse · 2023-07-07T09:13:59Z

It appears that snapshot also has the crash issue.

What crash issue?

The current code now provides FLAG_IMMUTABLE, right?

Is the error message still the same?

yeap, tried with every version from https://s01.oss.sonatype.org/content/repositories/snapshots/io/github/libpd/android/pd-core/1.2.1-SNAPSHOT/

joebowbeer · 2023-07-07T10:10:41Z

@rafaelgustavo123 can you confirm this PR is no longer needed and close it?

@cerupcat @FunkyMuse can you open an issue for your crash?

Is it one of these crashes?

cerupcat · 2023-07-10T02:46:41Z

@FunkyMuse can you help provide more information on the crash you're seeing?

FunkyMuse · 2023-07-10T08:03:00Z

@rafaelgustavo123 can you confirm this PR is no longer needed and close it?

@cerupcat @FunkyMuse can you open an issue for your crash?

Is it one of these crashes?

It's the FLAG_IMMUTABLE crash

rafaelgustavo123 · 2023-07-10T22:09:16Z

Should I close it?

residuum · 2023-07-13T10:13:19Z

PdCore/src/main/java/org/puredata/android/service/PdService.java

+            	if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
+                	pi = PendingIntent.getActivity(getApplicationContext(), 0, intent, PendingIntent.FLAG_UPDATE_CURRENT | PendingIntent.FLAG_IMMUTABLE);
+            	} else {
+                	pi = PendingIntent.getActivity(getApplicationContext(), 0, intent, PendingIntent.FLAG_UPDATE_CURRENT);


I am against FLAG_UPDATE_CURRENT here. This may change the behaviour on older Android versions, and the existing code with using 0 was working in many apps.

residuum · 2023-07-13T10:22:35Z

PdCore/src/main/java/org/puredata/android/service/PdService.java

+
+            	PendingIntent pi = null;
+            	if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
+                	pi = PendingIntent.getActivity(getApplicationContext(), 0, intent, PendingIntent.FLAG_UPDATE_CURRENT | PendingIntent.FLAG_IMMUTABLE);


I am not sure about setting FLAG_UPDATE_CURRENT here. On the one hand, this will just replace an existing notification so it will not show multiple notifications, if startAudio() is called multiple times. On the other hand, this should have been prevented in startAudio().

What's the solution here? We need to set one or the other per the original discussion on SO.

What's the solution here? We need to set one or the other per the original discussion on SO.

I would pick FLAG_IMMUTABLE, but I am unsure about the FLAG_UPDATE_CURRENT_PART.

joebowbeer · 2023-07-14T23:51:19Z

The immutable flag is in the main branch:

https://github.com/libpd/pd-for-android/blob/master/PdCore/src/main/java/org/puredata/android/service/PdService.java#L244

I've confirmed that this change is in the source code of the latest snapshot.

@rafaelgustavo123 please close this PR or rebase on the main branch

@cerupcat @FunkyMuse Please create a new issue with details

NOTE: Make sure you update the cache

https://stackoverflow.com/questions/13565082/how-can-i-force-gradle-to-redownload-dependencies

Update PdService.java

b054841

PendingIntent bug on android 12. Please, add this changes.

cerupcat approved these changes May 15, 2022

View reviewed changes

tkirshboim reviewed May 17, 2022

View reviewed changes

Update PdCore/src/main/java/org/puredata/android/service/PdService.java

f2c645c

Co-authored-by: Tal Kirshboim <[email protected]>

tkirshboim linked an issue May 17, 2022 that may be closed by this pull request

Targeting S+ (version 31 and above) requires that one of FLAG_IMMUTABLE or FLAG_MUTABLE be specified when creating a PendingIntent #121

Closed

residuum approved these changes Jan 21, 2023

View reviewed changes

joebowbeer requested review from joebowbeer and tkirshboim June 14, 2023 04:16

tkirshboim mentioned this pull request Jun 15, 2023

Raise target SDK to 33 for PdCore and PdTest #126

Merged

joebowbeer requested a review from residuum July 7, 2023 10:11

rafaelgustavo123 closed this Jul 10, 2023

rafaelgustavo123 reopened this Jul 10, 2023

residuum reviewed Jul 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update PdService.java #120

Update PdService.java #120

rafaelgustavo123 commented May 15, 2022

tkirshboim left a comment •

edited

Loading

tkirshboim May 17, 2022

rafaelgustavo123 May 17, 2022

tkirshboim May 17, 2022

joebowbeer May 18, 2022

joebowbeer commented May 17, 2022

joebowbeer commented May 17, 2022

joebowbeer commented May 17, 2022 •

edited

Loading

rafaelgustavo123 commented May 17, 2022

cerupcat commented May 20, 2022

tkirshboim commented May 20, 2022

FunkyMuse commented Jun 8, 2022

residuum left a comment

FunkyMuse commented Jun 13, 2023

cerupcat commented Jun 13, 2023

joebowbeer commented Jun 14, 2023 •

edited

Loading

FunkyMuse commented Jun 14, 2023

tkirshboim commented Jun 14, 2023

joebowbeer commented Jun 14, 2023

FunkyMuse commented Jun 14, 2023

FunkyMuse commented Jun 21, 2023 •

edited

Loading

joebowbeer commented Jun 22, 2023 •

edited

Loading

cerupcat commented Jul 5, 2023

joebowbeer commented Jul 7, 2023

FunkyMuse commented Jul 7, 2023 •

edited

Loading

joebowbeer commented Jul 7, 2023

cerupcat commented Jul 10, 2023

FunkyMuse commented Jul 10, 2023

rafaelgustavo123 commented Jul 10, 2023

residuum Jul 13, 2023

residuum Jul 13, 2023

cerupcat Jul 14, 2023

residuum Jul 14, 2023

joebowbeer commented Jul 14, 2023 •

edited

Loading

Update PdService.java #120

Are you sure you want to change the base?

Update PdService.java #120

Conversation

rafaelgustavo123 commented May 15, 2022

tkirshboim left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joebowbeer commented May 17, 2022

joebowbeer commented May 17, 2022

joebowbeer commented May 17, 2022 • edited Loading

rafaelgustavo123 commented May 17, 2022

cerupcat commented May 20, 2022

tkirshboim commented May 20, 2022

FunkyMuse commented Jun 8, 2022

residuum left a comment

Choose a reason for hiding this comment

FunkyMuse commented Jun 13, 2023

cerupcat commented Jun 13, 2023

joebowbeer commented Jun 14, 2023 • edited Loading

FunkyMuse commented Jun 14, 2023

tkirshboim commented Jun 14, 2023

joebowbeer commented Jun 14, 2023

FunkyMuse commented Jun 14, 2023

FunkyMuse commented Jun 21, 2023 • edited Loading

joebowbeer commented Jun 22, 2023 • edited Loading

cerupcat commented Jul 5, 2023

joebowbeer commented Jul 7, 2023

FunkyMuse commented Jul 7, 2023 • edited Loading

joebowbeer commented Jul 7, 2023

cerupcat commented Jul 10, 2023

FunkyMuse commented Jul 10, 2023

rafaelgustavo123 commented Jul 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joebowbeer commented Jul 14, 2023 • edited Loading

tkirshboim left a comment •

edited

Loading

joebowbeer commented May 17, 2022 •

edited

Loading

joebowbeer commented Jun 14, 2023 •

edited

Loading

FunkyMuse commented Jun 21, 2023 •

edited

Loading

joebowbeer commented Jun 22, 2023 •

edited

Loading

FunkyMuse commented Jul 7, 2023 •

edited

Loading

joebowbeer commented Jul 14, 2023 •

edited

Loading