Skip to content
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

Beautiful Android Notifications #18852

Merged
merged 28 commits into from Aug 20, 2019

Conversation

@MarcoPolo
Copy link
Contributor

commented Aug 9, 2019

Preview

Before

10a02c61-61ca-4505-934e-7dd043e4e17f

  • Gross, a new notification for every message!
  • "In another conversation" doesn't even look like a different conversation
  • Cecile said something! What's the context? Is this in keybase#breaking or keybase#random?
  • Who's this marcopolo character? Where's his lovely face?

After

Screen Shot 2019-08-08 at 3 38 00 PM

* Wow, all the notifications are grouped! * "In Another conversation" is clearly a different conversation. I can see it's in `marcopolo,pkt0`. * Ah, I can see cecile was responding to my "Testing Avatars in Notifs" message * Looking good marco!

Other Niceties

  • Device change notifications look better before after
  • Proper support for notification channels on android:

Screen Shot 2019-08-08 at 6 46 54 PM

* Greatly simplified notification handling on android * Pass more metadata for chat notifications (Can make iOS notifs nicer too)

@mmaxim for the Go/objc stuff
@jzila for Android stuff

Please let me know if anything looks off, I haven't done much with our Go stack.

Future work

Reply / Mark Read on Android (I wanted to get this shipped since it's such a big improvement over what we have now)

@@ -51,8 +55,37 @@ var kbfsConfig libkbfs.Config
var initMutex sync.Mutex
var initComplete bool

type Person struct {

This comment has been minimized.

Copy link
@mmaxim

mmaxim Aug 9, 2019

Member

Let's move all this stuff to a new file in bind, call it notifications.go or something.

return fmt.Sprintf("%s: %s", msg.Valid().SenderUsername, msg.Valid().MessageBody.Text().Body), nil
}
}
func (h *MobilePush) GetConvInfo(ctx context.Context, uid gregor1.UID, convID chat1.ConversationID) (info chat1.ConversationInfoLocal, err error) {

This comment has been minimized.

Copy link
@mmaxim

mmaxim Aug 9, 2019

Member

Can delete this function, just use utils.GetVerifiedConv instead and call it from bind

}
age := time.Since(time.Unix(int64(unixTime), 0))
if age >= 2*time.Minute {

This comment has been minimized.

Copy link
@mmaxim

mmaxim Aug 9, 2019

Member

Restore like this mentioned in chat

return res, errors.New("invalid message type for plaintext")
func GetUserAvatar(username string, srvInfo *keybase1.HttpSrvInfo) string {
if srvInfo != nil {
return fmt.Sprintf("http://%v/av?typ=user&name=%v&format=square_192&token=%v", srvInfo.Address, username, srvInfo.Token)

This comment has been minimized.

Copy link
@mmaxim

mmaxim Aug 9, 2019

Member

Should make this a function of the avatars package, probably by making that NewSrv thing get saved somewhere and be accessible. Not a huge deal though, so if it is annoying just skip it.

Updated!

MarcoPolo added some commits Aug 10, 2019

@@ -0,0 +1,29 @@
package keybase

This comment has been minimized.

Copy link
@mmaxim

mmaxim Aug 12, 2019

Member

I meant bring over HandleBackgroundNotification too

default:
h.Debug(ctx, "FormatPushText: unknown message type: %v", msg.GetMessageType())
return res, errors.New("invalid message type for plaintext")
func GetUserAvatar(username string, srvInfo *keybase1.HttpSrvInfo) string {

This comment has been minimized.

Copy link
@mmaxim

mmaxim Aug 12, 2019

Member

How come this is still here? I see you have it in avatar land too

This comment has been minimized.

Copy link
@MarcoPolo

MarcoPolo Aug 19, 2019

Author Contributor

I forgot to remove this!

@jzila
Copy link
Contributor

left a comment

A bunch of nits and suggestions, but overall this looks excellent.

})

// TODO: move this out of this file.
// FIXME: sometimes this doubles up on a cold start--we've already executed the previous code.
// TODO: fixme this is buggy

This comment has been minimized.

Copy link
@jzila

jzila Aug 12, 2019

Contributor

FYI I've created TRIAGE-462--you can reference that ticket here.

.setAutoCancel(true);

Message msg = chatNotification.getMessage();
Person.Builder personBuilder = new Person.Builder().setName(msg.getFrom().getKeybaseUsername()).setBot(msg.getFrom().getIsBot());

This comment has been minimized.

Copy link
@jzila

jzila Aug 12, 2019

Contributor

Please whitespace this so it's more easily readable.

This comment has been minimized.

Copy link
@MarcoPolo

MarcoPolo Aug 19, 2019

Author Contributor

We need to fix the formatting of the java code in general... 😅

This comment has been minimized.

Copy link
@jzila

jzila Aug 19, 2019

Contributor

Cool, what command did you run to generate those formatting fixes?


void deviceNotification() {
Bundle bundle = (Bundle) this.bundle.clone();
bundle.putBoolean("userInteraction", true);

This comment has been minimized.

Copy link
@jzila

jzila Aug 12, 2019

Contributor

can we just set this in genericNotification?

}

public List<MessagingStyle.Message> summary() {
return buffer.subList(0, buffer.size() >= 5 ? 5 : buffer.size());

This comment has been minimized.

Copy link
@jzila

jzila Aug 12, 2019

Contributor

Assuming you do the loop thing above, then you don't need the buffer.size() >= 5 check here.

Integer badgeCount = Integer.parseInt(bundle.getString("b"));
Integer unixTime = Integer.parseInt(bundle.getString("x"));
int badgeCount = Integer.parseInt(bundle.getString("b"));
int unixTime = Integer.parseInt(bundle.getString("x"));

This comment has been minimized.

Copy link
@jzila

jzila Aug 12, 2019

Contributor

why unbox these?

This comment has been minimized.

Copy link
@MarcoPolo

MarcoPolo Aug 19, 2019

Author Contributor

Android Studio's lint

This comment has been minimized.

Copy link
@MarcoPolo

MarcoPolo Aug 19, 2019

Author Contributor

We were boxing them originally. Integer.parseInt returns an int. https://docs.oracle.com/javase/7/docs/api/java/lang/Integer.html#parseInt(java.lang.String)

public static String GENERAL_CHANNEL_ID = "kb_rest_channel";

// This keeps a small (5 msg) ring buffer cache of the last messages the user was notified about,
// so we can give context to future notifications

This comment has been minimized.

Copy link
@jzila

jzila Aug 12, 2019

Contributor

5 msg per conversation

@joshblum
Copy link
Member

left a comment

go/objc side lgtm assuming manual testing

return err
}

// TODO how to figure out if it's a bot?

This comment has been minimized.

Copy link
@joshblum

joshblum Aug 19, 2019

Member

isBot := gregor1.UITPtrEq(msg.Valid().ClientHeader.BotUID, msg.Valid().ClientHeader.Sender)

you can put this as a helper in chat1/extras.go if it will be useful elsewhere..

username := msgUnboxed.Valid().SenderUsername

chatNotification := ChatNotification{
IsPlaintext: false,

This comment has been minimized.

Copy link
@joshblum

joshblum Aug 19, 2019

Member

why not displayPlaintext?

@MarcoPolo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

Manual Testing Strategy:

iOS Android Test
[x] [x] 1 on 1 message - plaintext
[x] [x] 1 on 1 message - not plain
[x] [x] Exploding Message
[X] [X] Big Team pushes with Channel Name
[x] [x] Small Team
@joshblum

This comment has been minimized.

Copy link
Member

commented on go/protocol/chat1/extras.go in 1fd354f Aug 19, 2019

should check IsValid() first or this can panic. Would rename to something like SenderIsBot

}
}

chatNotification.IsPlaintext = true

This comment has been minimized.

Copy link
@joshblum

joshblum Aug 19, 2019

Member

can rm now

MarcoPolo added some commits Aug 19, 2019

@MarcoPolo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

I fixed exploding messages since I didn't realize we only get those in chat.newmessage. I think we should fix up that flow because it will basically always error out of handleBackgroundNotification on ios with a stale notification because the unix time is 0. It would be nice to get more meta data too so we can show that on the android side (instead of falling back to the unstyled generic type message)

@jzila
Copy link
Contributor

left a comment

A few more nits

private IconCompat getKeybaseAvatar(String avatarUri) {
IconCompat icon = null;

if (!avatarUri.isEmpty()) {

This comment has been minimized.

Copy link
@jzila

jzila Aug 19, 2019

Contributor

To reduce nesting, I'd prefer:

Suggested change
if (!avatarUri.isEmpty()) {
if (avatarUri.isEmpty()) return null;
...

Then you don't even need the icon declaration above, instead returning directly where it's needed.

Keybase.setAppStateForeground();
}
// Emits the intent to JS
private void emitIntent(Intent intent) {

This comment has been minimized.

Copy link
@jzila

jzila Aug 19, 2019

Contributor

Why was this moved into MainActivity instead of being a separate class as before? I think this makes it harder to read and understand what's going on. This method doesn't use anything in this class, so I don't see why it can't be separated out.

This comment has been minimized.

Copy link
@MarcoPolo

MarcoPolo Aug 19, 2019

Author Contributor

Because this is only used by this class. That's why it's private. I can move it to it's own class but you don't get the same guarantees.

Handler handler = new Handler(Looper.getMainLooper());
handler.post(() -> {
// Construct and load our normal React JS code bundle
ReactInstanceManager mReactInstanceManager = ((ReactApplication) getApplication()).getReactNativeHost().getReactInstanceManager();

This comment has been minimized.

Copy link
@jzila

jzila Aug 19, 2019

Contributor

Why is this called mReactInstanceManager? It's not a member variable, it's a local variable...

This comment has been minimized.

Copy link
@MarcoPolo

MarcoPolo Aug 19, 2019

Author Contributor

copy/pasted, but good point 👍

Updated

@jzila

jzila approved these changes Aug 20, 2019

@jzila

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Very excited to use this!

@MarcoPolo MarcoPolo merged commit 8f06603 into master Aug 20, 2019

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@MarcoPolo MarcoPolo deleted the marco/better-android-notifs branch Aug 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.