Distinguish between public and private notifications.

[dennisguse]

Add a public notification (which does not show the current title and artist). If the lock screen is configured to hide sensitive information, this notification will be shown instead.

This allows to use interact with Odyssey from the lock screen while preventing others to see the currently playing song.

[gnome17]

Hi, interesting proposal. We think it could be argued that these information should be private but we don't share this opinion. So maybe we could add an option for this but not as a default.

[dennisguse]

Sounds reasonable. I will update the change request in the upcoming days.

On January 31, 2018 5:25:29 PM UTC, "Frederik Lütkes" notifications@github.com wrote:

Hi, interesting proposal. We think it could be argued that these information should be private but we don't share this opinion. So maybe we could add an option for this but not as a default.

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/gateship-one/odyssey/pull/120#issuecomment-362006692

-- Dennis Guse

[dennisguse]

IMPORTANT: the preference patch has a serious limitation!

It requires restarting the PlaybackService if the "private notification"-preference is changed. I could figure out, how this is done correctly... Any suggestions?

[gnome17]

Hi, the problem are the SharedPreferences (see https://developer.android.com/reference/android/content/SharedPreferences.html) :

Note: This class does not support use across multiple processes.

So you have to do something similar to the method hideArtwork in the PlaybackService. This will require more changes including extending the aidl file.

[dennisguse]

Works now correctly :)

I amended the changes to last commit.

Just one more thing: if a notification exists, I updated this notification using mNotification.visibility(...). I tried rebuilding it using the NotificationBuilder, but this removed the controls when going private. I don't know why, but I think it would be the better suited option.

I included both version (NotificationBuilder commented out) - It is marked with a comment. Just search for "ATTENTION".

What do you say?

[dennisguse]

From my side is code ready for inclusion.

I revisited again the issue on how to update the notification visibility (a: create a new one notification or b: update the visibility). I updated the current notification as the NotificationBuilder cannot be reused later. This will remove the playback controls, which would be quite annoying.

Whenever you have time.

[djselbeck]

I'll test it, when it compiles again ;)

[dennisguse]

Works again. And removed the modifier change of gradlew. It was for my convenience only :)

And removed the useless NPE comment - it was work in progress and I missed to remove it. I should not do code reviews to late at night...

[djselbeck]

A few things:

• I think the "Content hidden" text is superfluous. I checked with several other applications: FB Messenger Lite, AOSP SMS, Telegram, K9-mail all show just nothing.

• The setShowWhen call needs to be called BEFORE the first notification is build. Otherwise it is useless.

[dennisguse]

SetShowWhen got it - fixed it.

About "Contents hidden" I actually copied the behavior from K9Mail (version 5.403) and the default messaging app (com.android.messaging // version 1.0.001), which I use on LineageOS 7.1.2. I attached some screenshots.

I just checked briefly the code of K9Mail and I don't see that they explicitly set "Contents hidden" - might happen as part of LineageOS. I will do some more digging here....

Anyhow, if we don't show "Contents hidden", some user might mistake this as a bug.

[dennisguse]

"Contents hidden" is set automatically by Android if no public notification is set. However, then also the playbacks controls are not shown.

Next step is to find, how to apply this hidden-style for custom notifications...

[dennisguse]

So, it's done. A friend (@frahaase) helped me to discover how to import Android system-defined strings. The implemented code now uses the Android's default string - it should be thus consistent on each device.

Tested on LineageOS 7.1 only.

[frahaase]

You can find the documentation here: https://developer.android.com/reference/android/content/res/Resources.html Get systems returns: Return a global shared Resources object that provides access to only system resources (no application resources), and is not configured for the current screen (can not use dimension units, does not change based on orientation, etc).

[djselbeck]

I've also found this but this provides no documentation for the strings used for this method.

https://developer.android.com/reference/android/content/res/Resources.html#getIdentifier(java.lang.String,%20java.lang.String,%20java.lang.String)

I mean is there some documentation for the use of package "android"?

[dennisguse]

Sadly, there is not documentation about this anywhere to find. We only found that the strings are actually part of the AndroidSDK.

@djselbeck What do you think about using this approach, but check if the resource is available (check getIdentifer() != 0) and if it is not available, just use an empty string?

[gnome17]

We don't think it is a good solution to simply use some strings that are part of the SDK but not directly accessible. So please switch back to your previous solution with an additional string in Odyssey's resources.

[dennisguse]

I reverted the last change and now Odysseys strings.xml is used again.

[djselbeck]

I'm currently somewhat disconnected from the internet. I'll check and merge it as soon as I got a stable connection again

[dennisguse]

@djselbeck Any news?

[gnome17]

Hi, sorry for the delay we have currently not much time but I will try to get back to this pr in the next weeks.

[gnome17]

Hi, I finally tested your pr, works well on Android 6 and 8 and code looks fine. But I noticed that in the lockscreen you will still have the cover of the current album as the background. Was this intended? It seems a bit weird to hide the notification if you can gather most of the information from the cover anyway.

[dennisguse]

You are definitely right: it absolutely makes sense to also hide the artwork on the lock screen. I overlooked this completely.

Do you know if the lock screen art can be hidden while it is shown everywhere else? If the metaDataBuilder is used only for the lock screen, then it might be sufficient to adjust it in PlaybackServiceHelper, or?

https://github.com/gateship-one/odyssey/blob/dee3531c3c2fe35fab49408ac7ac0164cee2e1b6/app/src/main/java/org/gateshipone/odyssey/playbackservice/managers/PlaybackServiceStatusHelper.java#L370

https://github.com/gateship-one/odyssey/blob/dee3531c3c2fe35fab49408ac7ac0164cee2e1b6/app/src/main/java/org/gateshipone/odyssey/playbackservice/managers/PlaybackServiceStatusHelper.java#L222

[gnome17]

I think it should be sufficient to adjust the two lines in the PlaybackServiceHelper that you mentioned.

[dennisguse]

It is working, but I didnt have the time to refactor the patch, so the names of variables/methods etc. matches the updated functionality.

[dennisguse]

I changed the pref, methods and variables to hideMediaOnLockscreen (more or less). All other requests are addressed. What do you think?

[gnome17]

Hi, looks good now, thanks for your contribution. If you want we will add you to the list of contributors. In this case we need to know if it is fine and if we should use your nickname or fullname provided by your github account or something different.

[dennisguse]

Great and thanks for the offer. Feel free to decide it on your own - it was just one patch and nothing too big. If you decide to then you can use my full name, which is also my Github username :)

PS Thanks for your feedback to the patch and for creating Odysseus in the first place.