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

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

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

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

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

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?

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.

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.

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?

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.

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.

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.