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

Initial notification ignores custom notificationHandlerFactory #634

Closed
PhilLab opened this issue Mar 20, 2023 · 7 comments
Closed

Initial notification ignores custom notificationHandlerFactory #634

PhilLab opened this issue Mar 20, 2023 · 7 comments

Comments

@PhilLab
Copy link
Contributor

PhilLab commented Mar 20, 2023

Describe the bug
The initial notification does not use the NotificationHandlerFactory which you can set by UploadServiceConfig.setNotificationHandlerFactory(). Instead it is created standalone:
https://github.com/gotev/android-upload-service/blob/master/uploadservice/src/main/java/net/gotev/uploadservice/UploadService.kt#L206

        val notification = NotificationCompat.Builder(this, UploadServiceConfig.defaultNotificationChannel!!)
            .setSmallIcon(android.R.drawable.ic_menu_upload)
            .setOngoing(true)
            .setGroup(UploadServiceConfig.namespace)
            .build()

        startForeground(UPLOAD_NOTIFICATION_BASE_ID, notification)

This means that if you applied any special handling, it won't work for the very first notification

This is a bigger issue on Samsung devices, because they have a crash bug for which the workaround cannot be injected into the upload service:

To Reproduce
Steps to reproduce the behavior:

  1. Use the upload library on a Samsung device with Android 12 or higher
  2. Start an upload
  3. Eventually, this will result in a silent crash of your app (sometimes, it does not crash on the first try)

Expected behavior
The creation of the very first notification should also use the notification factory, if a custom one was provided.

@stale
Copy link

stale bot commented May 22, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@PhilLab
Copy link
Contributor Author

PhilLab commented May 22, 2023

Bump

@stale
Copy link

stale bot commented Aug 12, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix Something which is not going to happen. Ever. label Aug 12, 2023
@PhilLab
Copy link
Contributor Author

PhilLab commented Aug 14, 2023

@gotev would you consider this being relevant for the library?

@stale stale bot removed the wontfix Something which is not going to happen. Ever. label Sep 13, 2023
@gotev
Copy link
Owner

gotev commented Oct 15, 2023

@PhilLab thank you for this detailed report and proposed solution, much appreciated! Made a PR #648

The very first notification is inteded to be constructed like that, as it serves the sole purpose of guaranteeing service launch.

The NotificationHandler Factory serves a different purpose: to allow you to control how each of the uploads notifications are shown. The very first notification is something which Android requires to be there for the service to be launched. The NotificationHandler does its job after the service is started.

If the docs are not clear about this aspect, every suggestion is welcome to improve them.

@gotev gotev closed this as completed in 02a10c5 Nov 10, 2023
@gotev
Copy link
Owner

gotev commented Nov 10, 2023

Merged the PR and released version 4.9.2

@PhilLab
Copy link
Contributor Author

PhilLab commented Nov 27, 2023

@gotev thank you, very much appreciated!

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

No branches or pull requests

2 participants