Pandora applet needs url update and behavior modifications

[Kevin W.]

The Pandora applet keeps on having to have it's url changed because Pandora.com keeps on updating the player. It seems silly to have to change it every time they update, so you should be able to just use the version on their website currently. Also, the applet's behavior is not to the standards on the wiki.

I have made a patch to fix these issues.

[Mark Lee (malept)]

Just a quick note on the patch:

You should probably get rid of any commented out lines of code that you did, for a cleaner patch.

[Kevin W. (eyecreate)]

Fixed, took out comments related to material not changed in patch.

[Mark Lee (malept)]

In the future, could you mark your patches by checking "this attachment is a patch"? I prefer to view the patches in the browser, rather than download them. I've modified both of your current patches in this way.

Some style comments:

While I don't like the fact that the applet is currently using two-spaces-per-indentation (it should be following PEP8, which requires four spaces per indentation), that's what the convention is. Make sure your indentations are consistent. I think there are three inconsistencies.

You should probably rename the second parameter of GetPandoraUrl.__init__() because it's the same name as a module that you import.

Other than that, I'm going to defer review of the patch to someone who's more familiar with writing Python Awn applets. (Testers are also welcome to comment.)

[Michael Rooney (mrooney)]

Thanks for taking the initiative Kevin! I have talked about improving this before and I think that your patch addresses half the issue. Since the HTML code itself could change, it is possible that the same issue could happen with this approach. Also, I think page requests to get the URL every time are unnecessary bandwidth for users and could slow things down. I think the optimal way to address this issue is to ship the applet with the current known address as we do, but instead as an awnconfig key (whatever the proper name is for that, that goes to gconf in gnome). Then, wrap the fetching of the flash in a try, and if it can't be found, THEN try your new approach. If successful, put that new value in gconf and continue. This way, you only need to fetch the URL once per change, and even if they completely change HTML, we can at least tell users the new value to put in gconf as a relatively simple fix.

[Kevin W. (eyecreate)]

You are right, your suggestion would be the best way to handle the
situation. I will look into putting that in soon. One question I have, what
would be the best way to "stop" the player. I can't constantly have the
player applet up because it makes some of my other audio software not run
because they want full control of the sound. I tried to make it change the
url to about:blank to make it stop but I couldn't get it to work(maybe I was
doing something wrong), but is there a better way? That way a user can right
click and select "stop" when they don't want the player running, do what
they want, and then click the applet to show the dialog and start the player
again.

On Sat, Oct 4, 2008 at 11:24 PM, Mike Rooney <email address hidden> wrote:

> Thanks for taking the initiative Kevin! I have talked about improving
> this before and I think that your patch addresses half the issue. Since
> the HTML code itself could change, it is possible that the same issue
> could happen with this approach. Also, I think page requests to get the
> URL every time are unnecessary bandwidth for users and could slow things
> down. I think the optimal way to address this issue is to ship the
> applet with the current known address as we do, but instead as an
> awnconfig key (whatever the proper name is for that, that goes to gconf
> in gnome). Then, wrap the fetching of the flash in a try, and if it
> can't be found, THEN try your new approach. If successful, put that new
> value in gconf and continue. This way, you only need to fetch the URL
> once per change, and even if they completely change HTML, we can at
> least tell users the new value to put in gconf as a relatively simple
> fix.
>
> --
> Pandora applet needs url update and behavior modifications
> https://bugs.launchpad.net/bugs/278388
> You received this bug notification because you are a direct subscriber
> of the bug.
>

[Kevin W. (eyecreate)]

Alright, while waiting for feedback on my question above, I have another patch to replace the first. It includes usage of gconf now. This is my first try implementing something with gconf, so if there is a "better " way to do something i'm doing, please tell me.

Mike:I tired an approach similar to what you said. It uses a value in gconf. If there is none there, it fetches it like I wrote before. But I also added in fallback. If the file url is found to not be a flash file, it will also refetch the url so updates to the player will not been seen by the applet user, while not querying the website for a new value each time it starts.

[Michael Rooney (mrooney)]

Hi Kevin! I am not aware of a way to interface into Flash to stop it,
although that sounds like a separate issue which should perhaps be
separated into another bug report. In regards to your patch, it is
looking good. However gconf shouldn't be used directly as we want to
be desktop agnostic, so you probably want to use the settings class of
AWNLib to do this (it shouldn't be too much of a code change). You may
want to look at the weather applet for an example of how to read/write
to it, with a default. Check out fetchSettings around line 73 at
http://bazaar.launchpad.net/~awn-extras/awn-extras/trunk/annotate/891?file_id=weather.py-20071003215714-m4pm77bq1yz118ju-6.

[Mark Lee (malept)]

You should use awn.Config instead: http://wiki.awn-project.org/Blueprint:DesktopAgnosticConfig

[Kevin W. (eyecreate)]

Thanks Mark for your helpful link. I have mad the small changes needed for the patch.

[Kevin W. (eyecreate)]

Okay, I've made what might be the "final" patch for now. This patch incorporates a fixed version of a player stop. If you need to use audio in other software or want flash to stop, right click on the icon and select "stop". This will browse to about:blank so no activity will happen and the player will stop. As soon as you click on the icon to bring up the dialog again, the player will reload.

[Mark Lee (malept)]

Oh, I see why I keep seeing an inconsistent amount of whitespace per indentation - replace your hard tabs with spaces.

[Michael Rooney (mrooney)]

4 spaces each, to be precise :) (http://www.python.org/dev/peps/pep-0008/)

[Mark Lee (malept)]

Well...currently, the file is at 2 spaces per tab, so the patch might as well adhere to that. We can change it to be PEP8-compliant in a later patch. It makes it easier to figure out what changed.

[Michael Rooney (mrooney)]

Yes, good thinking!

[Andrew Starr-Bochicchio (andrewsomething)]

The ever changing Pandora url continues to be an issue, and Kevin W. hasn't replied in awhile, I'll volunteer to review and fix his patch. I'll make sure it still functions and fix the issue with inconsistent whitespace.

[Andrew Starr-Bochicchio (andrewsomething)]

Alright, here's a cleaned up patch. It fixes the the white space issues and now uses AWNlib as the applet had been moved to it since Kevin W. made his patch. This fixes the gconf key as well. Previously it was being set at avant-window-navigator>PandoraPlayer, but now using AWNlib it correctly goes to avant-window-navigator>applets>pandora. I also added a play button to the context menu since it just seemed to me if there was a stop button there should be a play button as well.

I'll commit to trunk if there are no objections.

[Andrew Starr-Bochicchio (andrewsomething)]

On further testing, I found that the key wasn't actually getting changed when the url was wrong. It would find the correct url if it didn't exist, but couldn't handle a 404. This should fix that...

[Kevin W. (eyecreate)]

Thank you for fixing the issues with my patch. I haven't had the time
recently to go back over my patch to fix it.

Kevin

On Thu, Mar 5, 2009 at 3:18 AM, Andrew Starr-Bochicchio <<email address hidden>
> wrote:

> On further testing, I found that the key wasn't actually getting changed
> when the url was wrong. It would find the correct url if it didn't
> exist, but couldn't handle a 404. This should fix that...
>
> ** Attachment added: "pandora2.patch"
> http://launchpadlibrarian.net/23489306/pandora2.patch
>
> --
> Pandora applet needs url update and behavior modifications
> https://bugs.launchpad.net/bugs/278388
> You received this bug notification because you are a direct subscriber
> of the bug.
>

[Michael Rooney (mrooney)]

I was going to mention that it isn't really doing anything Andrew, but
you beat me to it :) Anyway two parts of the patch confuse me:

+ try:
+ pandurl=self.applet.settings["url"]
+ except:
+ pandurl=self.returnurl()
+ self.applet.settings["url"] = pandurl

Aren't we shipping a default "url"? If so, this seems like it would
only help if someone manually deleted that gconf key or something
weird.

+ try:
+ site = urllib2.urlopen(pandurl)
+ meta=site.info()
+ if meta['Content-Type'] == 'application/x-shockwave-flash':
+ pandurl=self.returnurl()
+ self.applet.settings["url"] = pandurl
+ self.moz.load_url(pandurl)
+ except:
+ pandurl=self.returnurl()
+ self.applet.settings["url"] = pandurl
+ self.moz.load_url(pandurl)
         self.dialog.add(self.moz)

There are three duplicated lines in the try and the except. Shouldn't
that be in a finally, or after the try/except? Also I am not quite
sure what is going on here, you try to load the URL from the config,
but then you parse it from the web anyway? In other words, it doesn't
seem like the value that is persisted is ever really used except to
get some meta content type.

Is there a branch for this? I'd be happy to make some improvements if so.

[Andrew Starr-Bochicchio (andrewsomething)]

>Aren't we shipping a default "url"? If so, this seems like it would
>only help if someone manually deleted that gconf key or something
>weird.

Well, in theory we will, but until there is a schema file shipping the url, this is needed.

> There are three duplicated lines in the try and the except. Shouldn't
> that be in a finally, or after the try/except?

Right you are...

> Also I am not quite sure what is going on here, you try to load the URL
> from the config, but then you parse it from the web anyway? In other
> words, it doesn't seem like the value that is persisted is ever really used
> except to get some meta content type.

I also wondered about this, but was mostly concerned with getting the patch into
shape that worked with current trunk, not changing the approach. I added the try/except
after testing to see if this would actually work with a wrong url and getting a HTMLError
from urllib2.urlopen(pandurl).

I assume that the reason for getting the meta content is to make sure that the
page being opened is in fact the flash player and not something else.

Any way, as this does work for me. I'm going to fix it up a little more, and then push
to trunk so others can work on it as well.

Thanks for the patch Kevin W!

[Andrew Starr-Bochicchio (andrewsomething)]

Here's what I'm going to go with, at least for now:

Open the url in the gconf key and check to make sure it's the flash player:

+ try:
+ site = urllib2.urlopen(pandurl)
+ meta=site.info()
+ if meta['Content-Type'] == 'application/x-shockwave-flash':

If it is, load it. If it isn't return an error:

+ self.moz.load_url(pandurl)
+ else:
+ return Error

If the url can't be opened or isn't a flash application, get the url and set the key:

+ except:
+ pandurl=self.returnurl()
+ self.applet.settings["url"] = pandurl
+ self.moz.load_url(pandurl)

I'm going to go ahead and push to trunk. Feel free to hack on it Mike.

[Kevin W. (eyecreate)]

okay, I just wanted to state that the reason to use the gconf to store the
url is that I had some complaints that it didn't seem wise to always check
the site for the existence of the file. I didn't know anything about needing
a schema file to put the url value in, so I didn't know why to keep it in
gconf since the page itself is accessed every time the applet loads anyways,
but I put it in so that it could be changed without modding the source code.
I used the meta info for exactly what was described, because in my testing
of the code, I found that even if the file changed, it would load the 404
page still, so I put in the check to make sure it was a flash file and if it
wasn't, then it should go find where it moved to. So in short, it should
work like:

if (gconf key exists && flash file from key exists)
load url
elseif (gconf key exists && flash file not exist)
read main pandora page to find new url
put new url in gconf
load url
elseif (gconf key doesn't exist)
read main pandora page to find new url
put new url in gconf
load url

I think this is what you are trying to figure out and implement. I hope that
makes sense.

Kevin

On Thu, Mar 5, 2009 at 2:44 PM, Andrew Starr-Bochicchio <<email address hidden>
> wrote:

> ** Changed in: awn-extras
> Status: In Progress => Fix Committed
>
> --
> Pandora applet needs url update and behavior modifications
> https://bugs.launchpad.net/bugs/278388
> You received this bug notification because you are a direct subscriber
> of the bug.
>

[Michael Rooney (mrooney)]

Right, except that in either the case of the gconf key not existing,
or the flash file not existing, we want to do the same thing. The
logic is really:

if (gconf key exists && flash file from key exists)
....load url
else
....read main pandora page to find new url
....put new url in gconf
....load url

and I think as you said it is important not to load the URL every time
if the one in gconf works. This will save one request on almost every
startup making the applet quicker and be more responsive, and will
save pandora bandwidth as well :)