Implement Persistent Notification #26
Conversation
Add google maven repository, to allow importing compatibility libs
TODO: Change icon color/details for connected vs. disconnected status
Update the SDK version to match the version in gradle.
| @@ -174,7 +174,7 @@ public void run() | |||
| { | |||
| final TextView addressText = (TextView) findViewById(R.id.address); | |||
|
|
|||
| final WifiManager wifiManager = (WifiManager) getSystemService(WIFI_SERVICE); | |||
There was a problem hiding this comment.
Can you explain or provide a link which describes this type of leak? Is the handle to the WifiManager, coming from the Activity's context, causing the WifiManager to not be garbage collected?
There was a problem hiding this comment.
Issue was spotted by Android studio. The following link seems to explain it for another project:
pwittchen/NetworkEvents#106
There was a problem hiding this comment.
Oh, OK. From this bug report against Android 5.1 and below it is possible for a WifiManager to not be garbage collected based on a bug in WifiManager itself. I think that getting an instance via the application context just limits the maximum number of leaked objects to 1, as the application context is fixed for the life of the entire app.
As this app supports down to Android 4.1, it is fine to use this workaround to avoid the bug in WifiManager. Can you add a comment, reminding why using the application context is important?
| NotificationCompat.Builder mBuilder = | ||
| new NotificationCompat.Builder(_mParentContext) | ||
| .setSmallIcon(R.drawable.ic_launcher) | ||
| .setContentTitle("Baby Monitor Listener") |
There was a problem hiding this comment.
Can these strings be added to the strings.xml file instead? That way others can provide translations for other languages.
There was a problem hiding this comment.
Sure, hijacked the existing application name and connection status string. Will refresh the pull-request with the fixes.
|
|
||
|
|
||
| // Sets an ID for the notification | ||
| int mNotificationId = 1; |
There was a problem hiding this comment.
Why not make this ID a final and static class member, as it is the same for both notifications. That way it is not a magic number.
| @@ -37,6 +40,8 @@ | |||
| String _address; | |||
| int _port; | |||
| String _name; | |||
| Context _mParentContext = this; | |||
There was a problem hiding this comment.
What is the reason for capturing the context here? For the lifetime of the Activity the context will not change. Is it to allow the Runnables to access the context? Why not use ListenActivity.this instead:
NotificationCompat.Builder mBuilder =
new NotificationCompat.Builder(ListenActivity.this)
There was a problem hiding this comment.
Yep, that is cleaner way for doing this. Updated the code to this.
|
I took a look at the notifications, and it looks good. Thanks for the improvement! |
Show a notification when the app is connected and when connection is dropped.
Pull request also refreshes the gradle version and fixes a minor leak
Fixing issue #18