Merge lp:~sergiusens/account-polld/twitter_avatar into lp:~phablet-team/account-polld/trunk

[Sergio Schvezov]

If the twitter user's avatar is available use it in the notification card's icon

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[lp:~sergiusens/account-polld/twitter_avatar]

25. By Sergio Schvezov on 2014-07-26

Download into cache instead and make DownloadAvatar worth it's name.

[Roberto Alsina (ralsina)]

Other than the inline comment which is just a doubt, +1

review: Approve

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Sergio Schvezov (sergiusens)]

On sábado 26 de julio de 2014 18h'17:55 ART, Roberto Alsina wrote:
> Review: Approve
>
> Other than the inline comment which is just a doubt, +1
>
> Diff comments:
>
>> === modified file 'plugins/plugins.go'
>> --- plugins/plugins.go 2014-07-25 20:09:20 +0000
>> +++ plugins/plugins.go 2014-07-26 21:03:48 +0000
>> @@ -19,10 +19,20 @@
>>
>> import (
> ...
>
> I keep getting confused as to whether to return err, result or
> result, err in go.
> If we want to check the error, it makes sense to return error
> first so it's harder to ignore.

By convention errors are the last return value.
I can't ignore it either unless I ignore everything or ignore it explicitly
as seen here:
http://play.golang.org/p/6kCrwVCzEt

>
>> + filePart := filepath.Join(cmdName, "avatars", pluginName,
>> path.Base(url))
>> + if file, err := xdg.Cache.Find(filePart); err == nil {
>> + return file, nil
>> + }
>> +
> ...
>
>

[James Henstridge (jamesh)]

Is it possible to pass the HTTP URL as part of the notification rather than downloading the image explicitly? There is already efforts to add a disk cache for downloaded images to the shell, so do we need a separate one here?

[Sergio Schvezov (sergiusens)]

On lunes 28 de julio de 2014 04h'33:13 ART, James Henstridge wrote:
> Is it possible to pass the HTTP URL as part of the notification
> rather than downloading the image explicitly? There is already
> efforts to add a disk cache for downloaded images to the shell,
> so do we need a separate one here?

This will need to be circled back to the push people, AFAIK, that is not
yet supported.

[Sergio Schvezov (sergiusens)]

On lunes 28 de julio de 2014 09h'03:21 ART, Sergio Schvezov wrote:
> On lunes 28 de julio de 2014 04h'33:13 ART, James Henstridge wrote:
>> Is it possible to pass the HTTP URL as part of the notification
>> rather than downloading the image explicitly? There is already
>> efforts to add a disk cache for downloaded images to the shell,
>> so do we need a separate one here?
>
> This will need to be circled back to the push people, AFAIK, that is not
> yet supported.

Just tried (with gdbus), it does't work.

[James Henstridge (jamesh)]

My main concerns with this are:

1. there is no cache eviction, so it will continue to grow unchecked. There is nothing in the push notifications model to tell account-polld when the image isn't needed any more either, so it's not clear how this can be handled reliably.

2. there is no cache validation, so if new avatar images are served from the same URL we will never notice that change.

And if using URIs for icons doesn't work, how exactly will real push notifications provide icons once we have them?

[Sergio Schvezov (sergiusens)]

On martes 29 de julio de 2014 00h'47:50 ART, James Henstridge wrote:
> My main concerns with this are:
>
> 1. there is no cache eviction, so it will continue to grow
> unchecked. There is nothing in the push notifications model to
> tell account-polld when the image isn't needed any more either,
> so it's not clear how this can be handled reliably.
>
> 2. there is no cache validation, so if new avatar images are
> served from the same URL we will never notice that change.

I thought of adding this as a next step.

> And if using URIs for icons doesn't work, how exactly will real
> push notifications provide icons once we have them?

Chipaca told me that the push-helper would do that.

[James Henstridge (jamesh)]

That still doesn't answer the question of how account-polld will know when a particular image is no longer needed. The post office never tells us when a particular notification has been dismissed. The more I think about it, the more this feels like the wrong way to handle the problem.

[Sergio Schvezov (sergiusens)]

On martes 29 de julio de 2014 07h'28:43 ART, James Henstridge wrote:
> That still doesn't answer the question of how account-polld
> will know when a particular image is no longer needed. The post
> office never tells us when a particular notification has been
> dismissed. The more I think about it, the more this feels like
> the wrong way to handle the problem.

Feel free to rewrite it. I just downloaded persistently to keep it on
across reboots since my gripe with the unity click scope making me spend
more money than I wanted to.

Does it matter for account-polld to know the instant it doesn't need an
image anymore? I'd believe mentions are from the same regular people most
of the time.

=== modified file 'plugins/plugins.go'
--- plugins/plugins.go	2014-07-25 20:09:20 +0000
+++ plugins/plugins.go	2014-07-26 21:03:48 +0000
@@ -19,10 +19,20 @@
 
 import (
 	"errors"
+	"io"
+	"net/http"
+	"os"
+	"path"
+	"path/filepath"
 
 	"launchpad.net/account-polld/accounts"
+	"launchpad.net/go-xdg/v0"
 )
 
+func init() {
+	cmdName = os.Args[0]
+}
+
 // Plugin is an interface which the plugins will adhere to for the poll
 // daemon to interact with.
 //
@@ -131,6 +141,8 @@
 // the web service reported that the authentication token has expired.
 var ErrTokenExpired = errors.New("Token expired")
 
+var cmdName string
+
 // DefaultSound returns the path to the default sound for a Notification
 func DefaultSound() string {
 	return "/usr/share/sounds/ubuntu/notifications/Slick.ogg"
@@ -140,3 +152,47 @@
 func DefaultVibration() *Vibrate {
 	return &Vibrate{Duration: 200}
 }
+
+func DownloadAvatar(pluginName, url string) (string, error) {
+	filePart := filepath.Join(cmdName, "avatars", pluginName, path.Base(url))
+	if file, err := xdg.Cache.Find(filePart); err == nil {
+		return file, nil
+	}
+
+	file, err := xdg.Cache.Ensure(filePart)
+	if err != nil {
+		return "", err
+	}
+
+	if err := download(file, url); err != nil {
+		return "", err
+	}
+
+	return file, nil
+}
+
+func download(file, url string) (err error) {
+	defer func() {
+		if err != nil {
+			os.Remove(file)
+		}
+	}()
+
+	out, err := os.Create(file)
+	if err != nil {
+		return err
+	}
+	defer out.Close()
+
+	resp, err := http.Get(url)
+	if err != nil {
+		return err
+	}
+	defer resp.Body.Close()
+
+	if _, err := io.Copy(out, resp.Body); err != nil {
+		return err
+	}
+
+	return nil
+}
 
=== modified file 'plugins/twitter/twitter.go'
--- plugins/twitter/twitter.go	2014-07-25 19:01:28 +0000
+++ plugins/twitter/twitter.go	2014-07-26 21:03:48 +0000
@@ -32,7 +32,10 @@
 
 var baseUrl, _ = url.Parse("https://api.twitter.com/1.1/")
 
-const twitterIcon = "/usr/share/click/preinstalled/.click/users/@all/com.ubuntu.developer.webapps.webapp-twitter/twitter.png"
+const (
+	twitterIcon = "/usr/share/click/preinstalled/.click/users/@all/com.ubuntu.developer.webapps.webapp-twitter/twitter.png"
+	pluginName  = "twitter"
+)
 
 const (
 	maxIndividualStatuses               = 2
@@ -107,13 +110,17 @@
 
 	pushMsg := []plugins.PushMessage{}
 	for _, s := range statuses {
+		icon, err := plugins.DownloadAvatar(pluginName, s.User.Image)
+		if err != nil {
+			icon = twitterIcon
+		}
 		pushMsg = append(pushMsg, plugins.PushMessage{
 			Notification: plugins.Notification{
 				Card: &plugins.Card{
 					Summary: fmt.Sprintf("@%s mentioned you", s.User.ScreenName),
 					Body:    s.Text,
 					Actions: []string{fmt.Sprintf("http://mobile.twitter.com/%s/statuses/%d", s.User.ScreenName, s.Id)},
-					Icon:    twitterIcon,
+					Icon:    icon,
 					Persist: true,
 					Popup:   true,
 				},
@@ -180,13 +187,17 @@
 
 	pushMsg := []plugins.PushMessage{}
 	for _, m := range dms {
+		icon, err := plugins.DownloadAvatar(pluginName, m.Sender.Image)
+		if err != nil {
+			icon = twitterIcon
+		}
 		pushMsg = append(pushMsg, plugins.PushMessage{
 			Notification: plugins.Notification{
 				Card: &plugins.Card{
 					Summary: fmt.Sprintf("@%s sent you a DM", m.Sender.ScreenName),
 					Body:    m.Text,
 					Actions: []string{fmt.Sprintf("http://mobile.twitter.com/%s/messages", m.Sender.ScreenName)},
-					Icon:    twitterIcon,
+					Icon:    icon,
 					Persist: true,
 					Popup:   true,
 				},