[Merge] lp:~sergiusens/account-polld/notif_max into lp:account-polld
Manuel de la Peña
manuel.delapena at canonical.com
Fri Jul 25 16:02:17 UTC 2014
Review: Approve
Approving with a small recommendation and with the knowledge that the duplicated logic is merged in a future branch.
Diff comments:
> === modified file 'plugins/twitter/twitter.go'
> --- plugins/twitter/twitter.go 2014-07-25 15:05:33 +0000
> +++ plugins/twitter/twitter.go 2014-07-25 15:05:33 +0000
> @@ -22,6 +22,7 @@
> "fmt"
> "net/http"
> "net/url"
> + "sort"
> "strings"
>
> "launchpad.net/account-polld/accounts"
> @@ -84,23 +85,54 @@
> if err := decoder.Decode(&statuses); err != nil {
> return nil, err
> }
> +
> + sort.Sort(sort.Reverse(byStatusId(statuses)))
> + if len(statuses) < 1 {
> + return nil, nil
> + }
> + p.lastMentionId = statuses[0].Id
> +
> pushMsg := []plugins.PushMessage{}
> - latestStatus := p.lastMentionId
> for _, s := range statuses {
> pushMsg = append(pushMsg, plugins.PushMessage{
> Notification: plugins.Notification{
> Card: &plugins.Card{
> - Summary: fmt.Sprintf("Mention from @%s", s.User.ScreenName),
> + Summary: fmt.Sprintf("@%s mentioned you", s.User.ScreenName),
> Body: s.Text,
> - Icon: twitterIcon,
> - },
> - },
> - })
> - if s.Id > latestStatus {
> - latestStatus = s.Id
> - }
> - }
> - p.lastMentionId = latestStatus
> + Actions: []string{fmt.Sprintf("http://mobile.twitter.com/%s/statuses/%d", s.User.ScreenName, s.Id)},
> + Icon: twitterIcon,
> + Persist: true,
> + Popup: true,
> + },
> + Sound: plugins.DefaultSound(),
> + Vibrate: plugins.DefaultVibration(),
> + },
> + })
> + if len(pushMsg) == 2 {
Can you add a comment on why we do break when len == 2??
> + break
> + }
> + }
> + // Now we consolidate the remaining statuses
> + if len(statuses) > len(pushMsg) {
> + var screennames []string
> + for _, s := range statuses[2:] {
> + screennames = append(screennames, s.User.ScreenName)
> + }
> + pushMsg = append(pushMsg, plugins.PushMessage{
> + Notification: plugins.Notification{
> + Card: &plugins.Card{
> + Summary: "Multiple more mentions",
> + Body: fmt.Sprintf("From %s", strings.Join(screennames, ", ")),
> + Actions: []string{"http://mobile.twitter.com/i/connect"},
> + Icon: twitterIcon,
> + Persist: true,
> + Popup: true,
> + },
> + Sound: plugins.DefaultSound(),
> + Vibrate: plugins.DefaultVibration(),
> + },
> + })
> + }
> return pushMsg, nil
> }
>
> @@ -120,27 +152,54 @@
> if err := decoder.Decode(&dms); err != nil {
> return nil, err
> }
> +
> + sort.Sort(sort.Reverse(byDMId(dms)))
> + if len(dms) < 1 {
> + return nil, nil
> + }
> + p.lastDirectMessageId = dms[0].Id
> +
> pushMsg := []plugins.PushMessage{}
> - latestDM := p.lastDirectMessageId
> for _, m := range dms {
> pushMsg = append(pushMsg, plugins.PushMessage{
> Notification: plugins.Notification{
> Card: &plugins.Card{
> - Summary: fmt.Sprintf("Direct message from @%s", m.Sender.ScreenName),
> + Summary: fmt.Sprintf("@%s sent you a DM", m.Sender.ScreenName),
> Body: m.Text,
> - Icon: twitterIcon,
> - Persist: true,
> - Popup: true,
> - },
> - Sound: plugins.DefaultSound(),
> - Vibrate: plugins.DefaultVibration(),
> - },
> - })
> - if m.Id > latestDM {
> - latestDM = m.Id
> - }
> - }
> - p.lastDirectMessageId = latestDM
> + Actions: []string{fmt.Sprintf("http://mobile.twitter.com/%s/messages", m.Sender.ScreenName)},
> + Icon: twitterIcon,
> + Persist: true,
> + Popup: true,
> + },
> + Sound: plugins.DefaultSound(),
> + Vibrate: plugins.DefaultVibration(),
> + },
> + })
> + if len(pushMsg) == 2 {
Same as with the above, some context for the if would be nice.
> + break
> + }
> + }
> + // Now we consolidate the remaining messages
> + if len(dms) > len(pushMsg) {
> + var senders []string
> + for _, m := range dms[2:] {
> + senders = append(senders, m.Sender.ScreenName)
> + }
> + pushMsg = append(pushMsg, plugins.PushMessage{
> + Notification: plugins.Notification{
> + Card: &plugins.Card{
> + Summary: "Multiple direct messages available",
> + Body: fmt.Sprintf("From %s", strings.Join(senders, ", ")),
> + Actions: []string{"http://mobile.twitter.com/messages"},
> + Icon: twitterIcon,
> + Persist: true,
> + Popup: true,
> + },
> + Sound: plugins.DefaultSound(),
> + Vibrate: plugins.DefaultVibration(),
> + },
> + })
> + }
> return pushMsg, nil
> }
>
> @@ -183,6 +242,14 @@
> Text string `json:"text"`
> }
>
> +// ByStatusId implements sort.Interface for []status based on
> +// the Id field.
> +type byStatusId []status
> +
> +func (s byStatusId) Len() int { return len(s) }
> +func (s byStatusId) Swap(i, j int) { s[i], s[j] = s[j], s[i] }
> +func (s byStatusId) Less(i, j int) bool { return s[i].Id < s[j].Id }
> +
> // Direct message format is described here:
> // https://dev.twitter.com/docs/api/1.1/get/direct_messages
> type directMessage struct {
> @@ -193,6 +260,14 @@
> Text string `json:"text"`
> }
>
> +// ByStatusId implements sort.Interface for []status based on
> +// the Id field.
> +type byDMId []directMessage
> +
> +func (s byDMId) Len() int { return len(s) }
> +func (s byDMId) Swap(i, j int) { s[i], s[j] = s[j], s[i] }
> +func (s byDMId) Less(i, j int) bool { return s[i].Id < s[j].Id }
> +
> type user struct {
> Id int64 `json:"id"`
> ScreenName string `json:"screen_name"`
>
--
https://code.launchpad.net/~sergiusens/account-polld/notif_max/+merge/228320
Your team Ubuntu Phablet Team is subscribed to branch lp:account-polld.
More information about the Ubuntu-reviews
mailing list