Hi Brian, Thanks for the review. With the updated spec attached and patches.
Brian Cameron wrote: > Alfred: > > Shouldn't the two patches go upstream? Even if Songbird wants to > support non-GNOME applications, I'd think the configure script could > still detect if there is a /usr/share/applications directory and > add the desktop file if so. The taglib patch will be up-streamed for sure. About the menu item patch, I think it's better to get Takao's L10N evaluation for the file first and then I'll post the patch to Songbird community for review. Also add patch-03 which was developed by Ginn and still under review. > Also, shouldn't this have Requies and BuildRequires on SUNWgnome-media > since it depends on GStreamer? Updated. >> Please help me review the spec file for Songbird. SUNWsongbird.spec >> will be in spec-files-other/core. > > > %if %with_debug > > %define build_type debug > > %else > > %define build_type release > > %endif > > > > %ifarch sparc > > %define arch sparc > > %else > > %define arch i386 > > %endif > > That seems odd to me. Shouldn't this kind of stuff be handled by the > Solaris.inc include? The debug and release stuff is specific for Songbird. Could we add this to Solaris.inc? If yes, I'll patch Solaris.inc for this. Remove the arch define and replace it with base_arch in Solaris.inc. > > Summary: The desktop media player mashed-up with the Web. > > "mashed-up with the Web" doesn't make sense to me. >From wikipedia(http://en.wikipedia.org/wiki/Mashup_(web_application_hybrid)), a mashup is a web application that combines data from more than one source into a single integrated tool. Songbird isn't only a media player. It's embedded with a web browser. - The Mozilla extension system applies to Songbird. There are lots of extensions available to aggregate different data sources into Songbird, for example, the mashTape extension(http://addons.songbirdnest.com/addon/73) grabs data from flickr, youtube, wikipedia, lyricWiki... on the Internet. - The Webpage API allows the media player to be addressed via websites, a demo is available here: http://whacked.net/explore. and the music store and music blog integration with the media player: Sample Music Store: http://developer.songbirdnest.com/webpage-api/examples/musicstore/ Sample Music Blog: http://developer.songbirdnest.com/webpage-api/examples/musicblog/ Does this make sense to describe Songbird "the desktop media player mashed-up with the Web"? > > Version: 0.6 > > %define tarball_version 0.6.1 > > Why not just define Version to be 0.6.1 and avoid using tarball_version? When I unpack the tarball provided by Songbird, the top level directory is Songbird0.6 instead of 0.6.1. However, the link to the tarball contains the string 0.6.1. That's the reason I keep two version numbers here. Anyway, the updated spec removes the 0.6 version number and adds one line "mv Songbird* Songbird%{version}" to get around this. > > Songbird provides a public playground for Web media mash-ups by > > I'm not sure what "public playground" means. Sorry for confusion here. I think the basic idea is that Songbird is not only a media player. It tends to be a platform also for developers to develop different stuff on. The mission is "to catalyze and champion a diverse, open Media Web." Anyway, update the description here to reflect that it's a media player. > > providing developers with both desktop and Web APIs, developer > > resources and fostering Open Web media standards. > > This description doesn't seem to do a great job of explaining that this > is a media player. > > > ac_add_options --disable-dtrace > > ac_add_options --disable-dbus > > Why don't we want dtrace and D-Bus support? This might be to walk around some build error previously. Enable them in the updated spec. Best Regards. -Alfred -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: SUNWsongbird.spec URL: <http://mail.opensolaris.org/pipermail/jds-review/attachments/20080722/8925998a/attachment.ksh> -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: songbird-01-menu-item.diff URL: <http://mail.opensolaris.org/pipermail/jds-review/attachments/20080722/8925998a/attachment-0001.ksh> -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: songbird-02-taglib.diff URL: <http://mail.opensolaris.org/pipermail/jds-review/attachments/20080722/8925998a/attachment-0002.ksh> -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: songbird-03-remap-pixman-functions.diff URL: <http://mail.opensolaris.org/pipermail/jds-review/attachments/20080722/8925998a/attachment-0003.ksh>
