New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wearable reply #3244

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@chr1shaefn3r
Contributor

chr1shaefn3r commented May 23, 2015

Added wearable reply action to notifications, so now you can answer an incoming message right from your wrist :)

Tested for the past few days with my own Pebble and I also did check with an actual Android Wear Device from a colleague to test the voice reply.

Fixes #1699
Follow up to #3010

Thanks to @Karalix for sharing his code in SMSSecure#135, gave me a great headstart.

Intent intent = new Intent(WearReplyReceiver.REPLY_ACTION);
intent.putExtra("thread_ids", threadArray);
intent.putExtra("master_secret", masterSecret);

This comment has been minimized.

@moxie0

moxie0 May 28, 2015

Member

We can't put this into intents that are leaving the app. We don't know where they're going or where they're stored, but more importantly, if someone locks TS there shouldn't be random copies of this floating around somewhere else.

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r Jun 2, 2015

Contributor

I think I get your point. I'm wondering why the "getMarkAsReadIntent" is allowed to do so?

@@ -63,4 +63,28 @@ public PendingIntent getMarkAsReadIntent(Context context, MasterSecret masterSec
return PendingIntent.getBroadcast(context, 0, intent, PendingIntent.FLAG_UPDATE_CURRENT);
}
public PendingIntent getReplyIntent(Context context, MasterSecret masterSecret, long recipientId) {

This comment has been minimized.

@moxie0

moxie0 May 28, 2015

Member

this all feels a little copy and pastey

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r Jun 2, 2015

Contributor

Refactored

}
OutgoingTextMessage reply = new OutgoingTextMessage(recipient, responseText.toString());
MessageSender.send(context, masterSecret, reply, -1, false);

This comment has been minimized.

@moxie0

moxie0 May 28, 2015

Member

you've got the threadid right here...

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r Jun 2, 2015

Contributor

Fixed.

OutgoingTextMessage reply = new OutgoingTextMessage(recipient, responseText.toString());
MessageSender.send(context, masterSecret, reply, -1, false);
MessageNotifier.updateNotification(context, masterSecret);

This comment has been minimized.

@moxie0

moxie0 May 28, 2015

Member

What is this for? Looks like you canceled all TS notifications just before the async task.

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r Jun 2, 2015

Contributor

In fact it does :/ I took that to blindly from the MarkReadReceiver (https://github.com/WhisperSystems/TextSecure/blob/master/src/org/thoughtcrime/securesms/notifications/MarkReadReceiver.java). I will take a closer look and fix it.
Didn't catch that in my tests, as my Pebble keeps the multiple notifications even though the WearReplyReceiver cancels them on phone. Sorry for that.

@NickGeek

This comment has been minimized.

NickGeek commented Jun 18, 2015

Would love to see this added

@moxie0

This comment has been minimized.

Member

moxie0 commented Jun 18, 2015

I sent @chr1shaefn3r a PR with some changes. Need him to approve them, since I don't have a wear device to test with (donations welcome).

@chr1shaefn3r

This comment has been minimized.

Contributor

chr1shaefn3r commented Jun 21, 2015

I've been running and testing it the whole week, worked flawlessly 👍
I merged your commit, rebased everything, should be good to go now.

@moxie0

View changes

res/values/strings.xml Outdated
<item>Yes</item>
<item>No</item>
<item>OK</item>
<item>Thanks</item>

This comment has been minimized.

@moxie0

moxie0 Jun 21, 2015

Member

What would happen if we put emoji unicode points in here? Would they render on the watch? I think it'd be cool to have 👍 and 👎 instead of "yes and "no".

This comment has been minimized.

@McLoo

McLoo Jun 21, 2015

Contributor

If it worked, 👍 would also cover "ok", right?

This comment has been minimized.

@NickGeek

NickGeek Jun 22, 2015

In Android Wear the user can already draw emoji with the 5.1 update.

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r Jun 22, 2015

Contributor

Android is not happy with putting emoji unicode points here (tested with "\ud83d\udc4d"):
JNI DETECTED ERROR IN APPLICATION: input is not valid Modified UTF-8: illegal start byte 0xf0
string: '👍'
in call to NewStringUTF
from java.lang.String[] android.content.res.AssetManager.getArrayStringResource(int)

As @NickGeek already pointet out, when you are running latest android wear, you can answer with an emoji. Also Pebble has a feature to answer with an emoji, in contrast to answering with a prefilled plaintext.

So all, in all, we should stay with plaintext replies here.

This comment has been minimized.

@moxie0

moxie0 Jun 22, 2015

Member

I think they'd probably be in the code directly, rather than in the XML here. Never having seen an Android wear device, I'm not sure I understand the purpose of these strings if you can already quickly respond with arbitrary emoji or other values?

This comment has been minimized.

@NickGeek

NickGeek Jun 22, 2015

Keep the text. Hangouts has all the text replies with a few common emoji at the bottom. Having emoji replace "Yes", and "No" could be frustrating when texting people with phones that don't support emoji.

Drawing emoji is good as well, IIRC it also has commonly used ones available in the drawing screen.

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r Jun 23, 2015

Contributor

Specifiying emojis in code worked, they got rendered correctly on my Pebble Smartwatch.
Like @NickGeek I'm in doubt wether or not it would be the right thing to do.
On my Pebble Smartwatch it makes no sense at all. When I reply to a notification I get confronted with "Emojis" or "Use Templates" and now the templates also have Emojis ...
On Android Wear devices it might make a little more sense, as the option to answer with Emojis is one more swipe away. But I think Google will take great care of their User Experience and when users want to get more possibilities to answer with Emojis Google will add that to Android Wear.

All in all I think we should stay with the text.

This comment has been minimized.

@moxie0

moxie0 Jun 23, 2015

Member

And none of these devices have a way for system-defined templates? It seems a little weird that we're the ones choosing the strings a user can respond with.

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r Jun 24, 2015

Contributor

Good point.
With my Pebble Smartwatch the official Pebble-App lets me save five template strings which I will get as an option whenever "Reply" is available. Additionally I see the options the app chooses to offer via "setChoices".
On Android Wear its a little different. There is no configuration-option in the official Android Wear app. But a quick test with Hangouts and WhatsApp-Notifications showed, that the template strings offered to reply are nearly identical. Which makes me assume that Android Wear itself has some pre-configured template strings. (Thanks to my colleague @adracus who actually has a Android Wear Smartwatch and helps with testing)

I think we would be fine with getting rid of the "MessageNotifier_wear_replies" all together.

This comment has been minimized.

@NickGeek

NickGeek Jun 24, 2015

I see the same reply template for every app on my Sony Smartwatch 3 (5.1.1).

I wouldn't try to mess with the pre-configured Android Wear settings as they're good.

@chr1shaefn3r

This comment has been minimized.

Contributor

chr1shaefn3r commented Jun 23, 2015

Darn! I found a bug: Answering a text from a group doesn't work properly. I will look into that ASAP.

@moxie0

This comment has been minimized.

Member

moxie0 commented Jun 24, 2015

ok then i'm just waiting to merge this on the group reply issue and whatever you want to do with the template strings.

@chr1shaefn3r

This comment has been minimized.

Contributor

chr1shaefn3r commented Jun 25, 2015

I rebased on master and removed the template strings.
The new NotificationItem.getReplyIntent() is conflicting with NotificationState.getReplyIntent(), the first is pointing to ConversationPopupActivity.class the later to WearReplyReceiver.REPLY_ACTION.
I switched back to NotificationState.getReplyIntent during the rebase, master uses NotificationItem.getReplyIntent (thats why the check is failling, I guess).
@moxie0 Could you share some information about the ConversationPopupActivity, so we can resolve the conflicting methods?

@moxie0

This comment has been minimized.

Member

moxie0 commented Jun 25, 2015

@chr1shaefn3r The quick reply intent in master now is for quick reply on-device. You need a different intent that's just for reply from Wear devices. Might be easier to leave the PR based off its original branch and let me do the rebase.

What was up with the group reply issue?

@chr1shaefn3r

This comment has been minimized.

Contributor

chr1shaefn3r commented Jun 25, 2015

I did a debugging-session yesterday and failed to pin down the problem.
I know for sure, that with the initial implementation the group reply worked. To me, it looks like the internals of MessageSender.send changed. But a quick check with git blame didn't bring up any recent changes which looked suspicious. I will try to take another look on it tomorrow.

@chr1shaefn3r

This comment has been minimized.

Contributor

chr1shaefn3r commented Jun 27, 2015

I'm puzzled, with the initial implementation (new OutgoingTextMessage and MessageSender.send) it worked just fine (replying to both group and "normal" messages), at least thats what I had in mind.
Now I can't see any scenario in which MessageSender.send could have ever send a group message (with an OutgoingTextMessage)!?!?

Anyway, I fixed the issue with a similiar implementation as seen in QuickResponseService.

For the sake of overall architecture it might be a good idea to refactor WearReplyReceiver and QuickResponseService (and possible other classes I'm not aware of yet) and centralize the distinction between Group and "normal" message in MessageSender.send.

@chr1shaefn3r

This comment has been minimized.

Contributor

chr1shaefn3r commented Jul 1, 2015

Streamlined the commits, rebased on master and tested reply with TextSecure-, Non-TextSecure aka. SMS- and Group-Messages. All right now :)

@moxie0

This comment has been minimized.

Member

moxie0 commented Jul 1, 2015

@chr1shaefn3r I can't follow what's going on if you modify the history. I need the unrebased original series of commits, along with the commits for changes you've made since.

chr1shaefn3r and others added some commits May 19, 2015

Fix up wearable reply stuff a little.
1) Don't include MasterSecret in PendingIntents.

2) Correctly reply to non-push group threads, rather than
   just an individual in that group.

// FREEBIE
@chr1shaefn3r

This comment has been minimized.

Contributor

chr1shaefn3r commented Jul 1, 2015

@moxie0 Sorry for messing with the history, restored the unrebased original commits from my dangling-commits. Tell me if I need to restore anything else!

@NickGeek

This comment has been minimized.

NickGeek commented Jul 3, 2015

Awesome! Can't wait for this to get merged!

@moxie0

This comment has been minimized.

Member

moxie0 commented Jul 8, 2015

in 2.22.0, thanks!

@moxie0 moxie0 closed this Jul 8, 2015

@chr1shaefn3r chr1shaefn3r deleted the chr1shaefn3r:WearableReply branch Jul 9, 2015

@joeykrim joeykrim referenced this pull request Apr 24, 2017

Closed

Add Android Wear support #1699

@Webstas

This comment has been minimized.

Webstas commented May 16, 2017

I would say no since it only added notification replies and not a native app

@bogorad

This comment has been minimized.

bogorad commented Nov 22, 2017

What about buzzing when for incoming calls and option to answer?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment