Skip to content

Android Support Library + NotificationChannels #299

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

Merged
merged 5 commits into from
Aug 2, 2017
Merged

Android Support Library + NotificationChannels #299

merged 5 commits into from
Aug 2, 2017

Conversation

gamer3dx
Copy link

Android Support Library is stable now, with compatible NotificationChannels from Android O... So I guess it's time to use them.

Added support of NotificationChannels, with possibility to override him through protected createNotificationChannel of UploadTask class
@gotev
Copy link
Owner

gotev commented Jul 31, 2017

Thanks for the PR! For those not familiar with the new Channels concept, here's the video. I have some questions and notes about your PR:

  • Why have you switched the minSdkVersion to 14?
  • In UploadNotificationStatusConfig you added the possibility to setup the channel name explicitly, so I imagine you want to be able to specify a different channel for every notification status, to allow maximum customization, which is good, but then when you are constructing the default value in UploadNotificationConfig, I see an hardcoded channel name, which is not good, because the default setting will always point to net.gotev.uploadservice and not to the library user's namespace. I propose you the following changes:
    • rename the channel field in UploadNotificationStatusConfig to notificationChannel which is more expressive
    • remove the channel parameter from the UploadNotificationStatusConfig costructor and make it like this:
    public UploadNotificationStatusConfig() {
        notificationChannel = UploadService.NAMESPACE;
    }
    In this way the default channel will reflect the user setting.
    • add the method setNotificationChannelForAllStatuses to UploadNotificationConfig to be able to set the same channel for all the statuses easily
  • here you are creating a single notification channel. What happens if I set four different channel names for the four notification statuses?
  • here all the channels are given NotificationManager.IMPORTANCE_DEFAULT value. I think the best will be to make this configurable, by adding an additional setting called notificationChannelImportance (which defaults to NotificationManager.IMPORTANCE_DEFAULT) in UploadNotificationStatusConfig
  • Also, you can get rid of the createNotificationChannel method and create the channel directly here

@gamer3dx
Copy link
Author

gamer3dx commented Jul 31, 2017

  • MinSDK: Starting with Support Library version 26.0.0, the minimum supported API level has changed to API v14 for all support library packages. And work with channel ID completely implemented in 26.0.0-beta1.
  • Channel name: yep, i forgot about the NAMESPACE property. My bad. Name net.gotev.uploadservice was selected only to provide default channel for the library, with minimal functionality. Main idea was to give access to channelId, so developer can create notification channels somewhere outside the library, if he will decide to use his own channels. And that's also related to

set four different channel names for the four notification statuses.

  • Single notification channel: Yes, we can do it, but all notifications posted to a notification channel should behave the same. So I thought that because all four notification channels related to the same process - we can use single channel for of them.
  • I've added protected createNotificationChannel on purpose, so any user of this library could override it while implementing his custom UploadTask class and set all required properties on his own, cause importance is not only property - we also can change sound, lights, vibration, behavior on lockscreen, DND settings. So, if we gonna pass notification channel importance - we also should pass light (boolean), color (Color), vibration (boolean), vibration pattern (long[]) and so on.

@gotev
Copy link
Owner

gotev commented Aug 1, 2017

Ok for the minSdk. Android < 4.0 (2011) is rarely used today so it's time to move forward to have the latest features.

Regarding notification channels, I see that the user can still set four different channels for the four states. After some thinking, maybe the best would be to have a single notification channel for all the states, so I would move notificationChannelId from UploadNotificationStatusConfig to UploadNotificationConfig, so it's clearer at API level that all the states shares the same notification channel and it's not error-prone when you write the configuration code.

To allow custom notification channel creation, with all the customizations, I think the best would be to let the user create it outside the library and then just pass the notificationChannelId to the notification config. At this point, here we can do this:

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
    String notificationChannelId = params.notificationConfig.notificationChannelId;
    if (notificationChannelId == null) {
        notificationChannelId = UploadService.NAMESPACE;
    }
    if (!notificationManager.getNotificationChannels().contains(notificationChannelId)) {
        NotificationChannel channel = new NotificationChannel(notificationChannelId, "Upload Service channel", NotificationManager.IMPORTANCE_DEFAULT);
        notificationManager.createNotificationChannel(channel);
    }
}

and remove createNotificationChannel method. In this way:

  • if the user creates a custom channel somewhere and provides its ID, the library will simply use it without creating a new one
  • the user does not have to override a task just to implement a custom channel
  • if the user makes multiple upload requests, the new channel will be created only the first time
  • if the user does not create a notification channel or if it provides a null or non-existent channel name, it will be automatically created with default settings.

Forgot to tell you in previous comments that for the CI build to work you have to modify this line in .travis.yml to:

    - build-tools-26.0.1

And this line to:

    - android-26

@gotev gotev added the enhancement Library enhancement label Aug 1, 2017
@gotev gotev added this to the 3.4 milestone Aug 1, 2017
@gamer3dx
Copy link
Author

gamer3dx commented Aug 1, 2017

Seems fine for me. I will apply all necessary changes tomorrow.

@gotev gotev modified the milestones: 3.3.1, 3.4 Aug 2, 2017
@gotev gotev merged commit cb7789d into gotev:master Aug 2, 2017
@gotev
Copy link
Owner

gotev commented Aug 2, 2017

Great! Thanks for the contribution 😃

gotev added a commit that referenced this pull request Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Library enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants