D8966: Add a foreground notification to the background service

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

> BackgroundService.java:294
> +                .setSmallIcon(R.drawable.ic_notification)
> +                .setColor(getResources().getColor(R.color.primary))
> +                .setContentTitle(getString(R.string.kde_connect))

Why set a explicit color? I would go with the default and remove the line

> BackgroundService.java:314
> +
> +        startForeground(FOREGROUND_NOTIFICATION_ID, notification.build());
>      }

Also I expect a method called createForegroundNotification to do nothing but creating the notification. This has nothing to do with creating but actually using the the notification, thus is a side-effect which should be avoided.

According to the docs this must be called after the service is started. Not sure if this is the case here (especially when createForegroundNotification is called from oncreate(). Maybe it should be moved to onStartCommand()

REPOSITORY
  R225 KDE Connect - Android application

REVISION DETAIL
  https://phabricator.kde.org/D8966

To: mtijink, #kde_connect
Cc: nicolasfella
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20171123/0ff7c634/attachment-0001.html>