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

Proposed by Sergio Schvezov
Status: Merged
Approved by: Sergio Schvezov
Approved revision: 25
Merged at revision: 24
Proposed branch: lp:~sergiusens/account-polld/twitter_avatar
Merge into: lp:~phablet-team/account-polld/trunk
Prerequisite: lp:~sergiusens/account-polld/google_credentials
Diff against target: 135 lines (+70/-3)
2 files modified
plugins/plugins.go (+56/-0)
plugins/twitter/twitter.go (+14/-3)
To merge this branch: bzr merge lp:~sergiusens/account-polld/twitter_avatar
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve
Roberto Alsina (community) Approve
Manuel de la Peña Pending
Review via email: mp+228405@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
25. By Sergio Schvezov

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

Revision history for this message
Roberto Alsina (ralsina) wrote :

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

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Sergio Schvezov (sergiusens) wrote :

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
>> + }
>> +
> ...
>
>

Revision history for this message
James Henstridge (jamesh) 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?

Revision history for this message
Sergio Schvezov (sergiusens) 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.

Revision history for this message
Sergio Schvezov (sergiusens) wrote :

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.

Revision history for this message
James Henstridge (jamesh) 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.

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

Revision history for this message
Sergio Schvezov (sergiusens) wrote :

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.

Revision history for this message
James Henstridge (jamesh) 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.

Revision history for this message
Sergio Schvezov (sergiusens) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/plugins.go'
2--- plugins/plugins.go 2014-07-25 20:09:20 +0000
3+++ plugins/plugins.go 2014-07-26 21:03:48 +0000
4@@ -19,10 +19,20 @@
5
6 import (
7 "errors"
8+ "io"
9+ "net/http"
10+ "os"
11+ "path"
12+ "path/filepath"
13
14 "launchpad.net/account-polld/accounts"
15+ "launchpad.net/go-xdg/v0"
16 )
17
18+func init() {
19+ cmdName = os.Args[0]
20+}
21+
22 // Plugin is an interface which the plugins will adhere to for the poll
23 // daemon to interact with.
24 //
25@@ -131,6 +141,8 @@
26 // the web service reported that the authentication token has expired.
27 var ErrTokenExpired = errors.New("Token expired")
28
29+var cmdName string
30+
31 // DefaultSound returns the path to the default sound for a Notification
32 func DefaultSound() string {
33 return "/usr/share/sounds/ubuntu/notifications/Slick.ogg"
34@@ -140,3 +152,47 @@
35 func DefaultVibration() *Vibrate {
36 return &Vibrate{Duration: 200}
37 }
38+
39+func DownloadAvatar(pluginName, url string) (string, error) {
40+ filePart := filepath.Join(cmdName, "avatars", pluginName, path.Base(url))
41+ if file, err := xdg.Cache.Find(filePart); err == nil {
42+ return file, nil
43+ }
44+
45+ file, err := xdg.Cache.Ensure(filePart)
46+ if err != nil {
47+ return "", err
48+ }
49+
50+ if err := download(file, url); err != nil {
51+ return "", err
52+ }
53+
54+ return file, nil
55+}
56+
57+func download(file, url string) (err error) {
58+ defer func() {
59+ if err != nil {
60+ os.Remove(file)
61+ }
62+ }()
63+
64+ out, err := os.Create(file)
65+ if err != nil {
66+ return err
67+ }
68+ defer out.Close()
69+
70+ resp, err := http.Get(url)
71+ if err != nil {
72+ return err
73+ }
74+ defer resp.Body.Close()
75+
76+ if _, err := io.Copy(out, resp.Body); err != nil {
77+ return err
78+ }
79+
80+ return nil
81+}
82
83=== modified file 'plugins/twitter/twitter.go'
84--- plugins/twitter/twitter.go 2014-07-25 19:01:28 +0000
85+++ plugins/twitter/twitter.go 2014-07-26 21:03:48 +0000
86@@ -32,7 +32,10 @@
87
88 var baseUrl, _ = url.Parse("https://api.twitter.com/1.1/")
89
90-const twitterIcon = "/usr/share/click/preinstalled/.click/users/@all/com.ubuntu.developer.webapps.webapp-twitter/twitter.png"
91+const (
92+ twitterIcon = "/usr/share/click/preinstalled/.click/users/@all/com.ubuntu.developer.webapps.webapp-twitter/twitter.png"
93+ pluginName = "twitter"
94+)
95
96 const (
97 maxIndividualStatuses = 2
98@@ -107,13 +110,17 @@
99
100 pushMsg := []plugins.PushMessage{}
101 for _, s := range statuses {
102+ icon, err := plugins.DownloadAvatar(pluginName, s.User.Image)
103+ if err != nil {
104+ icon = twitterIcon
105+ }
106 pushMsg = append(pushMsg, plugins.PushMessage{
107 Notification: plugins.Notification{
108 Card: &plugins.Card{
109 Summary: fmt.Sprintf("@%s mentioned you", s.User.ScreenName),
110 Body: s.Text,
111 Actions: []string{fmt.Sprintf("http://mobile.twitter.com/%s/statuses/%d", s.User.ScreenName, s.Id)},
112- Icon: twitterIcon,
113+ Icon: icon,
114 Persist: true,
115 Popup: true,
116 },
117@@ -180,13 +187,17 @@
118
119 pushMsg := []plugins.PushMessage{}
120 for _, m := range dms {
121+ icon, err := plugins.DownloadAvatar(pluginName, m.Sender.Image)
122+ if err != nil {
123+ icon = twitterIcon
124+ }
125 pushMsg = append(pushMsg, plugins.PushMessage{
126 Notification: plugins.Notification{
127 Card: &plugins.Card{
128 Summary: fmt.Sprintf("@%s sent you a DM", m.Sender.ScreenName),
129 Body: m.Text,
130 Actions: []string{fmt.Sprintf("http://mobile.twitter.com/%s/messages", m.Sender.ScreenName)},
131- Icon: twitterIcon,
132+ Icon: icon,
133 Persist: true,
134 Popup: true,
135 },

Subscribers

People subscribed via source and target branches