fix: install button not appearing on android 8 - #3333
Conversation
| createNotificationChannel() // Safe to call multiple times | ||
| setForeground(getForegroundInfo()) | ||
| } catch (e: Exception) { | ||
| Log.d(tag, "Failed to set foreground info:", e) | ||
| Log.e(tag, "Failed to set foreground info:", e) | ||
| // On Android 8, if this fails, the job is likely doomed | ||
| return Result.failure() | ||
| } | ||
|
|
||
| val wakeLock: PowerManager.WakeLock = | ||
| (applicationContext.getSystemService(Context.POWER_SERVICE) as PowerManager) | ||
| .newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "$tag::Patcher") | ||
| .apply { | ||
| acquire(10 * 60 * 1000L) | ||
| Log.d(tag, "Acquired wakelock.") | ||
| } | ||
|
|
||
| val args = workerRepository.claimInput(this) | ||
|
|
||
| return try { | ||
| runPatcher(args) | ||
| } finally { | ||
| wakeLock.release() | ||
| runPatcher(args) // WorkManager holds its own WakeLock internally |
There was a problem hiding this comment.
| createNotificationChannel() // Safe to call multiple times | |
| setForeground(getForegroundInfo()) | |
| } catch (e: Exception) { | |
| Log.d(tag, "Failed to set foreground info:", e) | |
| Log.e(tag, "Failed to set foreground info:", e) | |
| // On Android 8, if this fails, the job is likely doomed | |
| return Result.failure() | |
| } | |
| val wakeLock: PowerManager.WakeLock = | |
| (applicationContext.getSystemService(Context.POWER_SERVICE) as PowerManager) | |
| .newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "$tag::Patcher") | |
| .apply { | |
| acquire(10 * 60 * 1000L) | |
| Log.d(tag, "Acquired wakelock.") | |
| } | |
| val args = workerRepository.claimInput(this) | |
| return try { | |
| runPatcher(args) | |
| } finally { | |
| wakeLock.release() | |
| runPatcher(args) // WorkManager holds its own WakeLock internally | |
| createNotificationChannel() // Safe to call multiple times. | |
| setForeground(getForegroundInfo()) | |
| } catch (e: Exception) { | |
| Log.e(tag, "Failed to set foreground info:", e) | |
| // On Android 8, if this fails, the job is likely doomed. | |
| return Result.failure() | |
| } | |
| val args = workerRepository.claimInput(this) | |
| return try { | |
| runPatcher(args) // WorkManager holds its own WakeLock internally. |
There was a problem hiding this comment.
What is the change here? The code looks exactly the same.
There was a problem hiding this comment.
Yeah the suggestion does look kinda weird.
Axelen123
left a comment
There was a problem hiding this comment.
Seems fine to me but I am not sure about the other suggestion.
Also, the PR title does not match the implemented code.
kitadai31
left a comment
There was a problem hiding this comment.
Overall, I have serious doubts as to whether this pull request will actually fix the issue.
|
|
||
| try { | ||
| // This does not always show up for some reason. | ||
| createNotificationChannel() // Safe to call multiple times |
There was a problem hiding this comment.
Creating the notification channel here should be unnecessary, as it has already been done within the createNotification() method above that is called from getForegroundInfo().
Also, if you have really confirmed that creating a notification channel twice here fixes the problem, use the same method within createNotification().
The ID of the notification channel created by the new method is different from the one that is actually being used.
This change will make a duplicate notification channel.
| runPatcher(args) | ||
| } finally { | ||
| wakeLock.release() | ||
| runPatcher(args) // WorkManager holds its own WakeLock internally |
There was a problem hiding this comment.
iirc, WorkManager doesn't holds WakeLock
Try turning off the screen while patching without WakeLock
It should stop patching while the screen is off
There was a problem hiding this comment.
I assumed the statement in the comment was true. Is it documented anywhere?
There was a problem hiding this comment.
See conversation in #2147
It is not documented, but some reports can be found on StackOverflow
TUSHAR91316
left a comment
There was a problem hiding this comment.
A few observations regarding this PR:
- Notification Channel ID: The new
createNotificationChannel()creates a channel with the ID"patcher_channel", which doesn't match the channel ID used increateNotification(). This will end up creating a duplicate unused channel. - Aborting on Notification Failure: Returning
Result.failure()inside thecatchblock forsetForeground()will cause the entire patch job to fail if the foreground notification fails. It might be better to let the patch process continue even if the notification fails to display. - WakeLock:
CoroutineWorkerin WorkManager doesn't hold a CPU wakelock automatically across the entire job duration. Removing the manualWakeLockcould cause patching to freeze when the device screen turns off, especially on OEMs with aggressive battery management.
|
|
||
| try { | ||
| // This does not always show up for some reason. | ||
| createNotificationChannel() // Safe to call multiple times |
There was a problem hiding this comment.
The channel ID "patcher_channel" used here is hardcoded and differs from the channel ID used in createNotification(). If a channel is needed before setForeground, we should reuse the existing channel ID / method to avoid creating a duplicate notification channel.
| runPatcher(args) // WorkManager holds its own WakeLock internally | ||
| } catch (e: Exception) { | ||
| Log.e(tag, "Patcher encountered an error", e) | ||
| Result.failure() |
There was a problem hiding this comment.
Returning Result.failure() here will abort the entire patching operation if setForeground() throws. We should log the exception and allow patching to proceed instead of failing the job.
| // in the finally-block, and therefore, it will be released regardless (of failure/success eventually). | ||
| acquire() | ||
| Log.d(tag, "Acquired wakelock.") | ||
| } |
There was a problem hiding this comment.
Removing the WakeLock might cause the patching process to stop prematurely when the screen turns off (see PR #2147). WorkManager does not guarantee a CPU wakelock for CoroutineWorker.
#3201
NotificationChannelis created before callingsetForeground. If it isn't created, the notification won't show, and the "Foreground" status will be rejected.WorkManagerautomatically holds aWakeLockwhiledoWork()is running. Manually acquiring one is usually unnecessary and can sometimes conflict with EMUI aggressive power management.