Merge lp:~jonas-drange/account-polld/gmail-missing-notifications into lp:~ubuntu-push-hackers/account-polld/trunk

[Jonas G. Drange]

- Force parsing of email address even if it violates RFC 5322.
- Encode icon file paths since unity-notifications require it.
- Summarize notification bubbles when > 10 in a batch, but never summarize in the indicator.

[John Lenton (chipaca)]

Given the level of insanity of this codepath, this does not seem worse than what we have :-)

+1

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:150
http://jenkins.qa.ubuntu.com/job/account-polld-ci/94/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-vivid-amd64-ci/9
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-vivid-armhf-ci/9
        deb: http://jenkins.qa.ubuntu.com/job/account-polld-vivid-armhf-ci/9/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-vivid-i386-ci/9

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/account-polld-ci/94/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:159
http://jenkins.qa.ubuntu.com/job/account-polld-ci/95/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-vivid-amd64-ci/10
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-vivid-armhf-ci/10
        deb: http://jenkins.qa.ubuntu.com/job/account-polld-vivid-armhf-ci/10/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-vivid-i386-ci/10

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/account-polld-ci/95/rebuild

review: Approve (continuous-integration)

[Bill Filler (bfiller)]

tested, works well!

review: Approve

[John Lenton (chipaca)]

My 'approve' stands.

review: Approve

=== modified file 'cmd/account-polld/main.go'
--- cmd/account-polld/main.go	2015-11-03 13:40:45 +0000
+++ cmd/account-polld/main.go	2015-12-21 11:51:14 +0000
@@ -44,8 +44,8 @@
 const (
 	SERVICETYPE_WEBAPPS = "webapps"
 
-	SERVICENAME_GMAIL    = "com.ubuntu.developer.webapps.webapp-gmail_webapp-gmail"
-	SERVICENAME_TWITTER  = "com.ubuntu.developer.webapps.webapp-twitter_webapp-twitter"
+	SERVICENAME_GMAIL   = "com.ubuntu.developer.webapps.webapp-gmail_webapp-gmail"
+	SERVICENAME_TWITTER = "com.ubuntu.developer.webapps.webapp-twitter_webapp-twitter"
 )
 
 const (
@@ -169,50 +169,23 @@
 func postOffice(bus *dbus.Connection, postWatch chan *PostWatch) {
 	for post := range postWatch {
 		obj := bus.Object(POSTAL_SERVICE, pushObjectPath(post.appId))
-		pers, err := obj.Call(POSTAL_INTERFACE, "ListPersistent", post.appId)
-		if err != nil {
-			log.Println("Could not list previous messages:", err)
-			continue
-		}
-		var tags []string
-		tagmap := make(map[string]int)
-		if err := pers.Args(&tags); err != nil {
-			log.Println("Could not get tags:", err)
-			continue
-		}
-		log.Printf("Previous messages: %#v\n", tags)
-		for _, tag := range tags {
-			tagmap[tag]++
-		}
 
 		for _, batch := range post.batches {
-			// add individual notifications upto the batch limit
-			// (minus currently presented notifications). If
-			// overflowed and no overflow present, present that.
-			var notifs []*plugins.PushMessage
-			add := batch.Limit - tagmap[batch.Tag]
-			if add > 0 {
-				// there are less notifications presented than
-				// the limit; we can show some
-				if len(batch.Messages) < add {
-					notifs = batch.Messages
-				} else {
-					notifs = batch.Messages[:add]
-				}
-			}
+
+			notifs := batch.Messages
+			overflowing := len(notifs) > batch.Limit
+
 			for _, n := range notifs {
-				n.Notification.Tag = batch.Tag
+				// We're overflowing, so no popups.
+				// See LP: #1527171
+				if overflowing {
+					n.Notification.Card.Popup = false
+				}
 			}
-			ofTag := batch.Tag + "-overflow"
-			if len(notifs) < len(batch.Messages) {
-				// overflow
-				n := batch.OverflowHandler(batch.Messages[len(notifs):])
-				n.Notification.Tag = ofTag
-				if tagmap[ofTag] != 0 {
-					// sneakily, don't replace the overflow card,
-					// but do play the sound & etc
-					n.Notification.Card = nil
-				}
+
+			if overflowing {
+				n := batch.OverflowHandler(notifs)
+				n.Notification.Card.Persist = false
 				notifs = append(notifs, n)
 			}
 
 
=== modified file 'plugins/gmail/gmail.go'
--- plugins/gmail/gmail.go	2015-03-27 11:40:37 +0000
+++ plugins/gmail/gmail.go	2015-12-21 11:51:14 +0000
@@ -23,7 +23,9 @@
 	"net/mail"
 	"net/url"
 	"os"
+	"regexp"
 	"sort"
+	"strings"
 	"time"
 
 	"log"
@@ -37,8 +39,10 @@
 const (
 	APP_ID           = "com.ubuntu.developer.webapps.webapp-gmail_webapp-gmail"
 	gmailDispatchUrl = "https://mail.google.com/mail/mu/mp/#cv/priority/^smartlabel_%s/%s"
-	// this means 3 individual messages + 1 bundled notification.
-	individualNotificationsLimit = 2
+	// If there's more than 10 emails in one batch, we don't show 10 notification
+	// bubbles, but instead show one summary. We always show all notifications in the
+	// indicator.
+	individualNotificationsLimit = 10
 	pluginName                   = "gmail"
 )
 
@@ -52,6 +56,9 @@
 // trackDelta defines how old messages can be before removed from tracking
 var trackDelta = time.Duration(time.Hour * 24 * 7)
 
+// regexp for identifying non-ascii characters
+var nonAsciiChars, _ = regexp.Compile("[^\x00-\x7F]")
+
 type GmailPlugin struct {
 	// reportedIds holds the messages that have already been notified. This
 	// approach is taken against timestamps as it avoids needing to call
@@ -154,12 +161,34 @@
 		from := hdr[hdrFROM]
 		var avatarPath string
 
-		if emailAddress, err := mail.ParseAddress(hdr[hdrFROM]); err == nil {
-			if emailAddress.Name != "" {
-				from = emailAddress.Name
-				avatarPath = qtcontact.GetAvatar(emailAddress.Address)
-			}
-		}
+		emailAddress, err := mail.ParseAddress(from)
+		if err != nil {
+			// If the email address contains non-ascii characters, we get an
+			// error so we're going to try again, this time mangling the name
+			// by removing all non-ascii characters. We only care about the email
+			// address here anyway.
+			// XXX: We can't check the error message due to [1]: the error
+			// message is different in go < 1.3 and > 1.5.
+			// [1] https://github.com/golang/go/issues/12492
+			mangledAddr := nonAsciiChars.ReplaceAllString(from, "")
+			mangledEmail, _ := mail.ParseAddress(mangledAddr)
+			if err != nil {
+				emailAddress = mangledEmail
+			}
+		} else {
+			from = emailAddress.Name
+		}
+
+		if emailAddress != nil {
+			avatarPath = qtcontact.GetAvatar(emailAddress.Address)
+			// If icon path starts with a path separator, assume local file path,
+			// encode it and prepend file scheme defined in RFC 1738.
+			if strings.HasPrefix(avatarPath, string(os.PathSeparator)) {
+				avatarPath = url.QueryEscape(avatarPath)
+				avatarPath = "file://" + avatarPath
+			}
+		}
+
 		msgStamp := hdr.getTimestamp()
 
 		if _, ok := pushMsgMap[msg.ThreadId]; ok {
@@ -186,12 +215,14 @@
 
 }
 func (p *GmailPlugin) handleOverflow(pushMsg []*plugins.PushMessage) *plugins.PushMessage {
-	// TRANSLATORS: This represents a notification summary about more unread emails
-	summary := gettext.Gettext("More unread emails available")
 	// TODO it would probably be better to grab the estimate that google returns in the message list.
 	approxUnreadMessages := len(pushMsg)
-	// TRANSLATORS: the first %d refers to approximate additional email message count
-	body := fmt.Sprintf(gettext.Gettext("You have about %d more unread messages"), approxUnreadMessages)
+
+	// TRANSLATORS: the %d refers to the number of new email messages.
+	summary := fmt.Sprintf(gettext.Gettext("You have %d new messages"), approxUnreadMessages)
+
+	body := ""
+
 	// fmt with label personal and no threadId
 	action := fmt.Sprintf(gmailDispatchUrl, "personal")
 	epoch := time.Now().Unix()