From: Amitai Schlair Date: Sun, 21 Jul 2013 23:01:15 +0000 (-0400) Subject: Move design and code review to discussion subpage. X-Git-Url: https://sipb.mit.edu/gitweb.cgi/ikiwiki.git/commitdiff_plain/b4a41c0595c83a33760ed6d19cac651cbd19c956?ds=sidebyside Move design and code review to discussion subpage. --- diff --git a/doc/todo/fancypodcast.mdwn b/doc/todo/fancypodcast.mdwn index 04d86823c..bff509325 100644 --- a/doc/todo/fancypodcast.mdwn +++ b/doc/todo/fancypodcast.mdwn @@ -10,10 +10,7 @@ also have lots more metadata. [[!template id=gitbranch branch=schmonz/fancypodcast author="[[schmonz]]"]] [[!tag patch]] -In summary, the branch preserves ikiwiki's existing podcast behavior, -adds more featureful behavior, and has been tested to work well in -some common podcatchers. I believe it is ready for integration. ---[[schmonz]] +Nothing new on the branch since 2013/07/21 merge to `master`. ## Features @@ -34,53 +31,6 @@ Episode description|(./) |(./) |(./) | Episode enclosure |(./) |(./) |(./) |(./) """]] -## Design - -7. For each fancy podcast episode, write a blog post containing - `\[[!meta enclosure="WikiLink/to/media.mp3"]]`. (Don't specify - more than one enclosure -- but if you do, last one wins.) -7. When rendering to HTML (single-page or inlined), append a link - to the media file. -7. When rendering to RSS/Atom, the text is the entry's content and - the media file is its enclosure. -7. Don't break simple podcasts in pursuit of fancy podcasts. - -## Implementation - -### Completed - -* Cover the existing simple podcast behavior with tests. -* Add an `enclosure` field to [[plugins/meta]] that expands the - given [[ikiwiki/WikiLink]] to an absolute URL (feed enclosures - pretty much need to be, and the reference feeds I've looked at - all do this). -* Write failing tests for the desired single-page and inlined - HTML behavior, then make them pass by adding enclosure stanzas - to `{,inline}page.tmpl`. -* Write failing tests for the desired RSS/Atom behavior, then make - them pass via changes to `{atom,rss}item.tmpl` and [[plugins/inline]]. -* Match feature-for-feature with - [tru_podcast](http://www.rainskit.com/blog/542/tru_podcast-a-podcasting-plugin-for-textpattern) - (what [[schmonz]] will be migrating from). -* Enrich [feed metadata](http://cyber.law.harvard.edu/rss/rss.html) - by catching up `rsspage.tmpl` to `atompage.tmpl`. -* Verify that [[plugins/more]] plays well with fancy podcasts. -* Verify that the feeds validate. -* Subscribe to a fancy feed in some common podcatchers and verify - display details against a reference podcast. -* Verify smooth transitions for two common use cases (see testing - details below). -* Code review: don't add enclosure divs unless we have enclosures. -* Code review: genericize download link for more use cases. -* Code review: don't confuse old readers with Atom names in RSS. -* Code review: instead of hacking back to `$link`, just provide it. -* Code review: show author in addition to feedname, if different. - -### Must-have (for [[schmonz]], anyway) - -* Think carefully about UTF-8. -* Verify that _all_ the tests pass (not just my new ones). - ## Migration ### Upgrading within ikiwiki: from simple to fancy @@ -228,125 +178,6 @@ it with ikiwiki instead. iTunes) alongside the RSS/Atom ones in [[plugins/inline]]. * Support Apple's "enhanced podcasts" (if they're still relevant). -### code review - - + # XXX better way to compute relative to srcdir? - + my $file = $absurl; - + $file =~ s|^$config{url}/||; - -I don't think ikiwiki offers a better way to do that, because there is -normally no reason to do that. Why does it need an url of this form here? ---[[Joey]] - -> In all the popular, production-quality podcast feeds I've looked -> at, enclosure URLs are always absolute (even when they could be -> expressed concisely as relative). [Apple's -> example](http://www.apple.com/itunes/podcasts/specs.html#example) -> does too. So I told \[[!meta]] to call `urlto()` with the third -> parameter true, which means the \[[!inline]] code here gets an -> absolute URL in `$pagestate{$p}{meta}{enclosure}`. To compute the -> enclosure's metadata, though, we of course need it as a local path. -> I didn't see a less -> [ongepotchket](http://www.jewish-languages.org/jewish-english-lexicon/words/1402) -> way at the time. If you have a better idea, I'm happy to hear it; -> if not, I'll add an explanatory comment. --[[schmonz]] - ->> I would be more comfortable with this if two two different forms of url ->> you need were both generated by calling urlto. It'd be fine to call ->> it more than once. --[[Joey]] - ->>> Heh, it was even easier than that! (Hooray for tests.) Done. ->>> --[[schmonz]] - - +
- + - -Can't we avoid adding this div when there's no enclosure? --[[Joey]] - -> Sure, I've moved the `` check to outside the -> section-and-div block for `{,inline}page.tmpl`. --[[schmonz]] - - +Download this episode - -"Download this episode" is pretty specific to particular use cases. -Can this be made more generic, perhaps just "Download"? --[[Joey]] - -> Yep, I got a little carried away. Done. --[[schmonz]] - - - - - <TMPL_VAR AUTHOR ESCAPE=HTML>: <TMPL_VAR TITLE> - - - -This change removes the author name from the title of the rss feed, which -does not seem necessary for fancy podcasts. And it is a change that -could negatively impact eg, Planet style aggregators using ikiwiki. --[[Joey]] - -> While comparing how feeds render in podcatchers, I noticed that -> RSS and Atom were inconsistent in a couple ways, of which this was -> one. The way I noticed it: with RSS, valuable title space was being -> spent to display the author. I figured Atom's display was the one -> worth matching. You're right, of course, that planets using the -> default template and somehow relying on the current author-in-the-title -> rendering for RSS feeds (but not Atom feeds!) would be broken by -> this change. I'm having trouble imagining exactly what would break, -> though, since guids and timestamps are unaffected. Would it suffice -> to provide a note in the changelog warning people to be careful -> upgrading their planets, and to customize `rssitem.tmpl` if they -> really prefer the old behavior (or don't want to take any chances)? -> --[[schmonz]] - ->> A specific example I know of is updo.debian.net, when used with ->> rss2email. Without the author name there, one cannot see who posted ->> an item. It's worth noting that planet.debian.org does the same thing ->> with its rss feed. (That's probably what I copied.) Atom feeds may ->> not have this problem, don't know. --[[Joey]] - ->>> Okay, that's easy to reproduce. It looks like this _might_ be ->>> a simple matter of getting \[[!aggregate]] to populate author in ->>> `add_page()`. I'll see what I can figure out. --[[schmonz]] - ->>>> Yep, that was mostly it. If the feed entry defines an author, ->>>> and the author is distinct from the feed name, we now show `NAME: ->>>> AUTHOR`, else just show `NAME` (same as always). In addition, ->>>> the W3 feed validator says `` is invalid, so ->>>> I replaced it with ``, and all of a sudden `r2e` ->>>> gives me better `From:` headers. With the latest on my branch, ->>>> when I generate the same planet as updo and run `r2e` over it, ->>>> the names I get in `From:` look like so: - -* `"updo: Junio C Hamano"` -* `"updo: Greg Kroah-Hartman"` -* `"updo: Eric Raymond: esr"` (article author != feed name, so we get both) -* `"updo: Jannis Pohlman: Jannis Pohlmann"` (oops! I tweaked the real updo) - ->>>> --[[schmonz]] - - +++ b/templates/rsspage.tmpl - + xmlns:atom="http://www.w3.org/2005/Atom" - + - -Why is it using atom namespace inside an rss feed? What are the chances -every crummy rss reader on earth is going to understand this? I'd put it at -about 0%; I doubt ikiwiki's own rss reader understands such a mashup. ---[[Joey]] - -> The validator I used () told me to. -> Pretty sure it doesn't make anything work better in the podcatchers -> I tried. Hadn't considered that it might break some readers. -> Removed. --[[schmonz]] - - +ikiwiki - -Does this added tag provide any benefits? --[[Joey]] - -> Consistency with the Atom feed, and of course it trumpets ikiwiki -> to software and/or curious humans who inspect their feeds. The tag -> arrived only in RSS 2.0, but that's already the version we're -> claiming to be, and it's over a decade old. Seems much less risky -> than the atom namespace bits. --[[schmonz]] - ->> Sounds ok then. --[[Joey]] - ---- [[merged|done]] --[[Joey]] diff --git a/doc/todo/fancypodcast/discussion.mdwn b/doc/todo/fancypodcast/discussion.mdwn new file mode 100644 index 000000000..bb225f89c --- /dev/null +++ b/doc/todo/fancypodcast/discussion.mdwn @@ -0,0 +1,162 @@ +# Round 1 + +## Design + +7. For each fancy podcast episode, write a blog post containing + `\[[!meta enclosure="WikiLink/to/media.mp3"]]`. (Don't specify + more than one enclosure -- but if you do, last one wins.) +7. When rendering to HTML (single-page or inlined), append a link + to the media file. +7. When rendering to RSS/Atom, the text is the entry's content and + the media file is its enclosure. +7. Don't break simple podcasts in pursuit of fancy podcasts. + +## Implementation + +### Completed + +* Cover the existing simple podcast behavior with tests. +* Add an `enclosure` field to [[plugins/meta]] that expands the + given [[ikiwiki/WikiLink]] to an absolute URL (feed enclosures + pretty much need to be, and the reference feeds I've looked at + all do this). +* Write failing tests for the desired single-page and inlined + HTML behavior, then make them pass by adding enclosure stanzas + to `{,inline}page.tmpl`. +* Write failing tests for the desired RSS/Atom behavior, then make + them pass via changes to `{atom,rss}item.tmpl` and [[plugins/inline]]. +* Match feature-for-feature with + [tru_podcast](http://www.rainskit.com/blog/542/tru_podcast-a-podcasting-plugin-for-textpattern) + (what [[schmonz]] will be migrating from). +* Enrich [feed metadata](http://cyber.law.harvard.edu/rss/rss.html) + by catching up `rsspage.tmpl` to `atompage.tmpl`. +* Verify that [[plugins/more]] plays well with fancy podcasts. +* Verify that the feeds validate. +* Subscribe to a fancy feed in some common podcatchers and verify + display details against a reference podcast. +* Verify smooth transitions for two common use cases (see testing + details below). +* Code review: don't add enclosure divs unless we have enclosures. +* Code review: genericize download link for more use cases. +* Code review: don't confuse old readers with Atom names in RSS. +* Code review: instead of hacking back to `$link`, just provide it. +* Code review: show author in addition to feedname, if different. + +### Code review + + + # XXX better way to compute relative to srcdir? + + my $file = $absurl; + + $file =~ s|^$config{url}/||; + +I don't think ikiwiki offers a better way to do that, because there is +normally no reason to do that. Why does it need an url of this form here? +--[[Joey]] + +> In all the popular, production-quality podcast feeds I've looked +> at, enclosure URLs are always absolute (even when they could be +> expressed concisely as relative). [Apple's +> example](http://www.apple.com/itunes/podcasts/specs.html#example) +> does too. So I told \[[!meta]] to call `urlto()` with the third +> parameter true, which means the \[[!inline]] code here gets an +> absolute URL in `$pagestate{$p}{meta}{enclosure}`. To compute the +> enclosure's metadata, though, we of course need it as a local path. +> I didn't see a less +> [ongepotchket](http://www.jewish-languages.org/jewish-english-lexicon/words/1402) +> way at the time. If you have a better idea, I'm happy to hear it; +> if not, I'll add an explanatory comment. --[[schmonz]] + +>> I would be more comfortable with this if two two different forms of url +>> you need were both generated by calling urlto. It'd be fine to call +>> it more than once. --[[Joey]] + +>>> Heh, it was even easier than that! (Hooray for tests.) Done. +>>> --[[schmonz]] + + +
+ + + +Can't we avoid adding this div when there's no enclosure? --[[Joey]] + +> Sure, I've moved the `` check to outside the +> section-and-div block for `{,inline}page.tmpl`. --[[schmonz]] + + +Download this episode + +"Download this episode" is pretty specific to particular use cases. +Can this be made more generic, perhaps just "Download"? --[[Joey]] + +> Yep, I got a little carried away. Done. --[[schmonz]] + + - + - <TMPL_VAR AUTHOR ESCAPE=HTML>: <TMPL_VAR TITLE> + - + +This change removes the author name from the title of the rss feed, which +does not seem necessary for fancy podcasts. And it is a change that +could negatively impact eg, Planet style aggregators using ikiwiki. --[[Joey]] + +> While comparing how feeds render in podcatchers, I noticed that +> RSS and Atom were inconsistent in a couple ways, of which this was +> one. The way I noticed it: with RSS, valuable title space was being +> spent to display the author. I figured Atom's display was the one +> worth matching. You're right, of course, that planets using the +> default template and somehow relying on the current author-in-the-title +> rendering for RSS feeds (but not Atom feeds!) would be broken by +> this change. I'm having trouble imagining exactly what would break, +> though, since guids and timestamps are unaffected. Would it suffice +> to provide a note in the changelog warning people to be careful +> upgrading their planets, and to customize `rssitem.tmpl` if they +> really prefer the old behavior (or don't want to take any chances)? +> --[[schmonz]] + +>> A specific example I know of is updo.debian.net, when used with +>> rss2email. Without the author name there, one cannot see who posted +>> an item. It's worth noting that planet.debian.org does the same thing +>> with its rss feed. (That's probably what I copied.) Atom feeds may +>> not have this problem, don't know. --[[Joey]] + +>>> Okay, that's easy to reproduce. It looks like this _might_ be +>>> a simple matter of getting \[[!aggregate]] to populate author in +>>> `add_page()`. I'll see what I can figure out. --[[schmonz]] + +>>>> Yep, that was mostly it. If the feed entry defines an author, +>>>> and the author is distinct from the feed name, we now show `NAME: +>>>> AUTHOR`, else just show `NAME` (same as always). In addition, +>>>> the W3 feed validator says `` is invalid, so +>>>> I replaced it with ``, and all of a sudden `r2e` +>>>> gives me better `From:` headers. With the latest on my branch, +>>>> when I generate the same planet as updo and run `r2e` over it, +>>>> the names I get in `From:` look like so: + +* `"updo: Junio C Hamano"` +* `"updo: Greg Kroah-Hartman"` +* `"updo: Eric Raymond: esr"` (article author != feed name, so we get both) +* `"updo: Jannis Pohlman: Jannis Pohlmann"` (oops! I tweaked the real updo) + +>>>> --[[schmonz]] + + +++ b/templates/rsspage.tmpl + + xmlns:atom="http://www.w3.org/2005/Atom" + + + +Why is it using atom namespace inside an rss feed? What are the chances +every crummy rss reader on earth is going to understand this? I'd put it at +about 0%; I doubt ikiwiki's own rss reader understands such a mashup. +--[[Joey]] + +> The validator I used () told me to. +> Pretty sure it doesn't make anything work better in the podcatchers +> I tried. Hadn't considered that it might break some readers. +> Removed. --[[schmonz]] + + +ikiwiki + +Does this added tag provide any benefits? --[[Joey]] + +> Consistency with the Atom feed, and of course it trumpets ikiwiki +> to software and/or curious humans who inspect their feeds. The tag +> arrived only in RSS 2.0, but that's already the version we're +> claiming to be, and it's over a decade old. Seems much less risky +> than the atom namespace bits. --[[schmonz]] + +>> Sounds ok then. --[[Joey]]