GNOME Bugzilla – Bug 636550
[Context] Add tab with links to websites to context pane
Last modified: 2010-12-10 22:22:51 UTC
Created attachment 175894 [details] [review] Additional tab with links to music websites in context pane plugin The idea is adding another tab to the Context Pane showing useful links, about the current track. This originally is a GCI task, for additional information, please see here: http://socghop.appspot.com/gci/task/show/google/gci2010/gnome/t129019719841 Please, review this initial patch, and tell let me know what I need to change, fix, etc.
Review of attachment 175894 [details] [review]: This is a reasonable start, but I think it can be simplified a lot. I'm working on fixing the way links are opened so they'll always go to the default browser rather. ::: plugins/context/context/LinksTab.py @@ +129,3 @@ + + def connect_signals (self): + self.ds.connect ('links-ready', self.links_ready) shouldn't need this, as the link creation process is (currently?) completely synchronous. if we start doing background wikipedia searches to check if a page exists, then we might need to use a signal to indicate when the link data is ready, but for now LinksView can just call self.ds.get_artist_links() etc. immediately. This also means you don't need to use the 'loading' template, since loading doesn't take any time. Instead, there should just be a single method that renders the template using data from the data source, then loads it into the webview. @@ +154,3 @@ + self.error = None + self.db = db + self.db.connect ('entry-extra-metadata-notify::rb:links', self.links_notify) This bit isn't necessary, nothing outside the plugin is going to be asking for the link data @@ +160,3 @@ + if entry == self.entry: + print "Ready! Shoot!" + self.emit ('links-ready', self.entry, metadata) and since nothing outside the plugin is going to be asking for the link data, this will never be called.. @@ +175,3 @@ + "Wikipedia" : "http://www.wikipedia.org/%s" % self.artist, + "Allmusic" : "http://www.discogs.com/search?type=artist&q=%s&f=html" % self.artist, + "Discogs" : "http://www.allmusic.com/search/artist/%s" % self.artist I think for discogs we should try to link directly to the artist's page, since it's just www.discogs.com/artist/Artist+Name, and if the name is misspelled or doesn't have the right punctuation, it usually gives a link to the correct page. ::: plugins/context/tmpl/artist-tmpl.html @@ +11,3 @@ text = text.replace('\n', '</p><p>') return text + please try to exclude useless/irrelevant changes like this from your patches ::: plugins/context/tmpl/links-tmpl.html @@ +4,3 @@ +<meta http-equiv="content-type" content="text-html; charset=utf-8"> +<%! + import cgi what does this template use the cgi module for? @@ +10,3 @@ +<body> +%if error is None: +<h1><em>${artist | h}</em> links:</h1> This string should be translatable, like the "Unable to get links" string below. @@ +13,3 @@ +<ul> +%for k, v in art_links : + <li><a href="${v}">${k}</a></li> I'd like to see a small image included for each link, probably the favicon for each site @@ +16,3 @@ +%endfor +</ul> +<h1>Links for <em>${album | h}</em>:</h1> this one too, and they should use consistent phrasing
Created attachment 175959 [details] [review] Additional tab with links to music websites in context pane plugin A handful of updates, including the images for the links.
Review of attachment 175959 [details] [review]: This looks much better, thanks. A few minor things left to fix. Could you upload a tar file (or something) with the link images? ::: plugins/context/context/LinksTab.py @@ +102,3 @@ + + def load_view (self): + self.webview.load_string (self.file, 'text/html', 'utf-8', self.basepath) since this is only called directly from load_links below, it doesn't need to be a separate method any more @@ +104,3 @@ + self.webview.load_string (self.file, 'text/html', 'utf-8', self.basepath) + + def load_tmpl (self): I don't see much of a need for this to be a separate method either @@ +180,3 @@ + def get_error (self): + if self.get_artist() is "": + return "No artist specified!" If this is going to be displayed to the user, it needs to be translated. It probably shouldn't have an exclamation mark. ::: plugins/context/tmpl/artist-tmpl.html @@ +11,3 @@ text = text.replace('\n', '</p><p>') return text + still adding an unnecessary line here ::: plugins/context/tmpl/links-tmpl.html @@ +11,3 @@ +<body> +%if error is None: +<h1>Links for <em>${artist | h}</em>:</h1> This still isn't translated; I'm not sure exactly how to do it considering that the artist name needs to be html-escaped, but there should be an example in one of the other templates.
(In reply to comment #3) > Review of attachment 175959 [details] [review]: > > This looks much better, thanks. A few minor things left to fix. Could you > upload a tar file (or something) with the link images? > Sure, I can do that. :-) You mean, instead of including them in the patch? > ::: plugins/context/context/LinksTab.py > @@ +102,3 @@ > + > + def load_view (self): > + self.webview.load_string (self.file, 'text/html', 'utf-8', > self.basepath) > > since this is only called directly from load_links below, it doesn't need to be > a separate method any more > Ok > @@ +104,3 @@ > + self.webview.load_string (self.file, 'text/html', 'utf-8', > self.basepath) > + > + def load_tmpl (self): > > I don't see much of a need for this to be a separate method either > > @@ +180,3 @@ > + def get_error (self): > + if self.get_artist() is "": > + return "No artist specified!" > > If this is going to be displayed to the user, it needs to be translated. It > probably shouldn't have an exclamation mark. Will fix that. thanks. > ::: plugins/context/tmpl/artist-tmpl.html > @@ +11,3 @@ > text = text.replace('\n', '</p><p>') > return text > + > > still adding an unnecessary line here Sorry about that... I will fix that. > ::: plugins/context/tmpl/links-tmpl.html > @@ +11,3 @@ > +<body> > +%if error is None: > +<h1>Links for <em>${artist | h}</em>:</h1> > > This still isn't translated; I'm not sure exactly how to do it considering that > the artist name needs to be html-escaped, but there should be an example in one > of the other templates. Well, the album and the artist don't have to be translated. Solution: -<h1>Links for <em>${album | h}</em>:</h1> +<h1>${ _("Links for") } <em>${album | h}</em>:</h1>
(In reply to comment #4) > (In reply to comment #3) > > Review of attachment 175959 [details] [review] [details]: > > > > This looks much better, thanks. A few minor things left to fix. Could you > > upload a tar file (or something) with the link images? > > > > Sure, I can do that. :-) > > You mean, instead of including them in the patch? Oh, I didn't see that they were already in the patch. Never mind then. > > ::: plugins/context/tmpl/links-tmpl.html > > @@ +11,3 @@ > > +<body> > > +%if error is None: > > +<h1>Links for <em>${artist | h}</em>:</h1> > > > > This still isn't translated; I'm not sure exactly how to do it considering that > > the artist name needs to be html-escaped, but there should be an example in one > > of the other templates. > > Well, the album and the artist don't have to be translated. > > Solution: > > -<h1>Links for <em>${album | h}</em>:</h1> > +<h1>${ _("Links for") } <em>${album | h}</em>:</h1> This won't work for all languages. The translated string really needs to be "Links for %s", with "<em>${album | h}</em>" substituted in for %s.
Created attachment 175962 [details] The images for the link tab They're all of size 16x16, and please leave the names, otherwise webkit won't find them anymore.
Created attachment 175972 [details] [review] Additional tab with links to music websites in context pane plugin Fixed all of the previous errors, plus some other refactorings.
Review of attachment 175972 [details] [review]: I'm going to fix these last few issues and commit this. Thanks for working on it. ::: plugins/context/context/LinksTab.py @@ +154,3 @@ + "Wikipedia" : "http://www.wikipedia.org/%s" % artist, + "Allmusic" : "http://www.discogs.com/search?type=artist&q=%s&f=html" % artist, + "Discogs" : "http://www.allmusic.com/search/artist/%s" % artist the discogs and allmusic URLs are in the wrong order, and the wikipedia URLs should be http://www.wikipedia.org/wiki/Name_Separated_By_Underscores @@ +170,3 @@ + "Wikipedia" : "http://www.wikipedia.org/%s" % album, + "Allmusic" : "http://www.discogs.com/search?type=album&q=%s&f=html" % album, + "Discogs" : "http://allmusic.com/search/album/%s" % album as above ::: plugins/context/tmpl/links-tmpl.html @@ +11,3 @@ +<body> +%if error is None: +<h1>${ _("Links for") } <em>${artist | h}</em>:</h1> The translatable string here should be "Links for %s:", as in some languages the natural place to put the album/artist name may be in the middle or at the start of the text rather than at the end. When writing translatable strings, you really need to give the translators the full string to work with.
pushed as commit 8b30aa8 and the uri opening problems fixed in commit b5c505e.
(In reply to comment #8) > Review of attachment 175972 [details] [review]: > > I'm going to fix these last few issues and commit this. Thanks for working on > it. > > ::: plugins/context/context/LinksTab.py > @@ +154,3 @@ > + "Wikipedia" : "http://www.wikipedia.org/%s" % artist, > + "Allmusic" : > "http://www.discogs.com/search?type=artist&q=%s&f=html" % artist, > + "Discogs" : "http://www.allmusic.com/search/artist/%s" % > artist > > the discogs and allmusic URLs are in the wrong order, and the wikipedia URLs > should be http://www.wikipedia.org/wiki/Name_Separated_By_Underscores Thanks for correcting this! > @@ +170,3 @@ > + "Wikipedia" : "http://www.wikipedia.org/%s" % album, > + "Allmusic" : > "http://www.discogs.com/search?type=album&q=%s&f=html" % album, > + "Discogs" : "http://allmusic.com/search/album/%s" % album > > as above > > ::: plugins/context/tmpl/links-tmpl.html > @@ +11,3 @@ > +<body> > +%if error is None: > +<h1>${ _("Links for") } <em>${artist | h}</em>:</h1> > > The translatable string here should be "Links for %s:", as in some languages > the natural place to put the album/artist name may be in the middle or at the > start of the text rather than at the end. When writing translatable strings, > you really need to give the translators the full string to work with. Oh! Thanks for pointing that out. I am not very experienced with internationalisation. I should have cleaned this up myself, but I somehow missed your previous mail. So thank you for fixing those yourself.