[Desktop] SMS plugin no longer creates desktop notifications -- Old functionality still supported by notifications plugin
ClosedPublic
Actions

Authored by sredman on Sep 15 2018, 8:51 PM.

Details

Reviewers

albertvaka
nicolasfella

Group Reviewers

KDE Connect

Commits

R224:64062b3cc9be: [Desktop] SMS plugin no longer creates desktop notifications -- Old…

Summary

Drop support for creating notifications from the SMS plugin. The old usecase is better handled by the notifications plugin reply box anyway

Rename packets defined in SMS plugin from PACKET_TYPE_TELEPHONY_* to PACKET_TYPE_SMS_*

Update TELEPHONY plugin packet description to point to SMS plugin

Define TELEPHONY_REQUEST_MUTE packet to replace old TELEPHONY_REQUEST with mute event field

Please see Android-side patch here: D15544

Test Plan

I see four test cases, based on the version of the app being used, where "old" means any version built with sources not containing this revision and "new" means any version built with sources using this revision:

New Android vs. New Desktop
- Supported and works for me
New Android vs. Old Desktop
- Supported and works for me
Old Android vs. New Desktop
- Not supported - Download a new app from the Play store or F-Droid
Old Android vs. Old Desktop
- If this is broken, it is not my fault :)

In the supported use cases:

Test SMS reply
- Receive SMS (or MMS!) message
- Verify that the notification plugin forwards a replyable notification and that replying works
Test incoming call ringer mute
- Enable ringer volume (not vibrate)
- Receive phone call
- Verify a desktop notification with a Mute button appears
- Verify that clicking the mute button causes the phone to stop making the ringer noise (vibration not affected)

Diff Detail

Repository

R224 KDE Connect

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

sredman created this revision.Sep 15 2018, 8:51 PM

Restricted Application added a project: KDE Connect. · View Herald TranscriptSep 15 2018, 8:51 PM

Restricted Application added a subscriber: kdeconnect. · View Herald Transcript

sredman requested review of this revision.Sep 15 2018, 8:51 PM

Harbormaster completed remote builds in B2894: Diff 41714.Sep 15 2018, 8:51 PM

sredman mentioned this in D15544: [Android] Cleanup SMS plugin and document deprecated SMS-relevant Telephony packet types.Sep 15 2018, 8:52 PM

sredman edited the summary of this revision. (Show Details)Sep 15 2018, 8:53 PM

sredman added a reviewer: KDE Connect.

One disadvantage of this approach is that we loose the ability to reply to all SMS from the notification. Only notifications from apps that support quick reply will have a reply button. Since have the SMS app this is nowhere near as bad as it would be without. I would say it's an acceptable price for solving the double SMS dilemma, but I'm not a regular user of SMS, so take that with a grain of salt

Also, the notificationsplugin on Android (AppDatabase.java) blocks some SMS packets. If we make the notificationsplugin the way to deliver SMS notifications those mustn't be blacklistet anymore

In D15543#326667, @nicolasfella wrote:

Also, the notificationsplugin on Android (AppDatabase.java) blocks some SMS packets. If we make the notificationsplugin the way to deliver SMS notifications those mustn't be blacklistet anymore

Good catch! I think I have updated this in the Android-side diff

In D15543#326665, @nicolasfella wrote:

One disadvantage of this approach is that we loose the ability to reply to all SMS from the notification. Only notifications from apps that support quick reply will have a reply button. Since have the SMS app this is nowhere near as bad as it would be without. I would say it's an acceptable price for solving the double SMS dilemma, but I'm not a regular user of SMS, so take that with a grain of salt

This is pretty much my thought... If a particular SMS app does not support the replyable notification, it is up to that app to be updated. Additionally, as you say, the usecase we are trying to encourage is the SMS app. I will personally keep blacklisting notifications from my messages app!

nicolasfella added inline comments.Sep 16 2018, 7:38 PM

plugins/sms/smsplugin.cpp
51–52	I think this can be removed as well. We don't deal with notifications anymore and it was never implemented anyway
52	Theoretically the check is not necessary since this is the only packet type that is supported

Looks good to me, but I'd like another opinion about the reply thing

albertvaka accepted this revision.Sep 16 2018, 8:49 PM

This revision is now accepted and ready to land.Sep 16 2018, 8:49 PM

nicolasfella accepted this revision.Sep 16 2018, 9:38 PM

Remove useless notification cancel check

Harbormaster completed remote builds in B2940: Diff 41780.Sep 16 2018, 10:01 PM

sredman added inline comments.Sep 16 2018, 10:02 PM

plugins/sms/smsplugin.cpp
52	True, but not checking packet type is part of how we got into the mess with the Telephony plugin 😬

nicolasfella accepted this revision.Sep 16 2018, 10:05 PM

Closed by commit R224:64062b3cc9be: [Desktop] SMS plugin no longer creates desktop notifications -- Old… (authored by sredman). · Explain WhySep 16 2018, 10:07 PM

This revision was automatically updated to reflect the committed changes.

sredman mentioned this in R225:0cc3639aa172: [Android] Cleanup SMS plugin and document deprecated SMS-relevant Telephony….Sep 16 2018, 11:12 PM

Revision Contents
Changeset List

			Path	Packages
M			plugins/sms/kdeconnect_sms.json (7 lines)
M			plugins/sms/smsplugin.h (43 lines)
M			plugins/sms/smsplugin.cpp (90 lines)
M			plugins/telephony/kdeconnect_telephony.json (2 lines)
M			plugins/telephony/telephonyplugin.h (7 lines)
M			plugins/telephony/telephonyplugin.cpp (2 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	41714	707a4bd		Sep 15 2018, 8:51 PM	★	★
Diff 2	41780	d742684	Remove useless notification cancel check	Sep 16 2018, 10:01 PM	★	★
Diff 3	41782	d742684	R224:64062b3cc9bea4b0dcd13a1a0cd3ade5dcbdad83	Sep 16 2018, 10:06 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
326be7fc1c42	cbc2e313d927	f662c351b0bd	Simon Redman	Remove useless notification cancel check	Sep 16 2018, 10:00 PM
f662c351b0bd	d0dc03b22415	d02a4f332bbb	Simon Redman	[Desktop] SMS plugin no longer creates desktop notifications -- Old… (Show More…)	Sep 15 2018, 8:02 PM
d02a4f332bbb	a1832d815c4b	480ffac1d265	Simon Redman	Reword SMS packet descriptions for precision	Sep 15 2018, 7:40 PM
480ffac1d265	76ddb6fb99ef	1ec1b36f44d6	Simon Redman	Update TELEPHONY plugin packet description to point to SMS plugin	Sep 15 2018, 7:40 PM
1ec1b36f44d6	da2a958c4755	3b1b6598abb3	Simon Redman	Rename packet types to reflect sms nature	Sep 12 2018, 11:24 PM
3b1b6598abb3	c198487e47e6	d742684d2147	Simon Redman	SMS plugin no longer creates desktop notifications -- Old functionality could… (Show More…)	Sep 12 2018, 11:21 PM