Skip to content

Conversation

@muhrifqii
Copy link

No description provided.

@felixdivo
Copy link
Contributor

@gotev Will this eventually be merged?

@gotev
Copy link
Owner

gotev commented Jan 19, 2019

Hi @muhrifqii and thanks for your PR 😎! Finally had some time to review it!

Thanks to your contribution I thought about one of the design decisions I made when the support for Notification Channels has been added. At that time none of my projects used notification channels and it was a feature needed for the app to just run on Android 8.

In particular, the decision to create the notification channel if it doesn't exist inside the library has been proven to be wrong, because in that way you are tempted to wrap the whole composition logic of the notification channels and groups inside the library. In fact, your PR proves it and I have to thank you for this.

Time has passed since then and I have to say that in all the apps I implemented during this period, I've always created notification channels and groups by composition using standard SDK APIs inside every app and then just passed the channel names to the android upload service library.

Just to make an example. If you look at https://github.com/gotev/android-upload-service/pull/382/files#diff-0dd55c8313c82ef1d5070fbe5d18e718R138 and the following lines added by this PR, I can tell you it's the almost exact code I use in utility methods to create notification channels and groups if needed. Furthermore, every app I made had different requirements about groups and priorities.

I know it's tempting to include this utility code in the library so you don't have to deal with it in every app you make and simply "Drop some strings to the library et voilà, everything works", but I think it will be better to not deal with the whole notification channels and groups logic inside the library itself, because this is going to limit library users when Google comes up with new features and additions, it increases maintenance effort and overall brings some serious limitations and unwanted side effects.

So, to conclude this review It think it will be better to not merge this PR and to also remove notification channel creation in the library, launching an exception like "notification channel does not exist" and providing better guidance in the Wiki about this decision and how to make a correct implementation.

Your point of view it's also important, so feel free to comment 😃

@muhrifqii
Copy link
Author

IMHO it will be convenient for faster implementation (without having subclassing this and that, or submodule the lib) then have a well configured upload notification channel (if the app needed to do so).

Yes it will definitely bring more maintenance effort if notification API changes. But it's all up to you.
Btw, @gotev thank you for returning, and happy new year 👍

@gotev gotev mentioned this pull request Aug 14, 2019
38 tasks
@gotev gotev closed this Aug 14, 2019
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

Successfully merging this pull request may close these issues.

3 participants