X-Git-Url: https://sipb.mit.edu/gitweb.cgi/ikiwiki.git/blobdiff_plain/a9585d231e7bd57bb4da2e962198e59bbbb3d2d1..06dc69946fc87ec750693636b94990edd8c899ff:/doc/plugins/contrib/po.mdwn diff --git a/doc/plugins/contrib/po.mdwn b/doc/plugins/contrib/po.mdwn index af748a8d3..61ec53ea8 100644 --- a/doc/plugins/contrib/po.mdwn +++ b/doc/plugins/contrib/po.mdwn @@ -198,3 +198,189 @@ finish it at some point in the first quarter of 2009. --[[intrigeri]] > subsequent adjustments. > > --[[intrigeri]] + +> I've looked it over and updated my branch with some (untested) +> changes. +> +>> I've merged your changes into my branch. Only one was buggy. +> +> Sorry, I'd forgotten about your cansave hook.. sorry for the duplicate +> work there. +> +> Reviewing the changes, mostly outside of `po.pm`, I have +> the following issues. +> +> * renamepage to renamelink change would break the ikiwiki +> 3.x API, which I've promised not to do, so needs to be avoided +> somehow. (Sorry, I guess I dropped the ball on not getting this +> API change in before cutting 3.0..) +>> +>> Fixed, see [[todo/need_global_renamepage_hook]]. +>> +> * I don't understand the parentlinks code change and need to figure it +> out. Can you explain what is going on there? +>> +>> I'm calling `bestlink` there so that po's injected `bestlink` is +>> run. This way, the parent links of a page link to the parent page +>> version in the proper language, depending on the +>> `po_link_to=current` and `po_link_to=negotiated` settings. +>> Moreover, when using my meta branch enhancements plus meta title to +>> make pages titles translatable, this small patch is needed to get +>> the translated titles into parentlinks. +>> +> * canrename's mix of positional and named parameters is way too +> ugly to get into an ikiwiki API. Use named parameters +> entirely. Also probably should just use named parameters +> for canremove. +> * `skeleton.pm.example`'s canrename needs fixing to use either +> the current or my suggested parameters. +>> +>> Done. +>> +> * I don't like the exporting of `%backlinks` and `$backlinks_calculated` +> (the latter is exported but not used). +>> +>> The commit message for 85f865b5d98e0122934d11e3f3eb6703e4f4c620 +>> contains the rationale for this change. I guess I don't understand +>> the subtleties of `our` use, and perldoc does not help me a lot. +>> IIRC, I actually did not use `our` to "export" these variables, but +>> rather to have them shared between `Render.pm` uses. +>> +>>> My wording was unclear, I meant exposing. --[[Joey]] +>>> +>>>> I guess I still don't know Perl's `our` enough to understand clearly. +>>>> No matter whether these variables are declared with `my` or `our`, +>>>> any plugin can `use IkiWiki::Render` and then access +>>>> `$IkiWiki::backlinks`, as already does e.g. the pagestat plugin. +>>>> So I guess your problem is not with letting plugins use these +>>>> variables, but with them being visible for every piece of +>>>> (possibly external) code called from `Render.pm`. Am I right? +>>>> If I understand clearly, using a brace block to lexically enclose +>>>> these two `our` declarations, alongside with the `calculate_backlinks` +>>>> and `backlinks` subs definitions, would be a proper solution, wouldn't +>>>> it? --[[intrigeri]] +>>>> +>>>>> No, %backlinks and the backlinks() function are not the same thing. +>>>>> The variable is lexically scoped; only accessible from inside +>>>>> `Render.pm` --[[Joey]] +>>>> +> * What is this `IkiWiki::nicepagetitle` and why are you +> injecting it into that namespace when only your module uses it? +> Actually, I can't even find a caller of it in your module. +>> +>> I guess you should have a look to my `meta` branch and to +>> [[bugs/pagetitle_function_does_not_respect_meta_titles]] in order +>> to understand this :) +>> +>>> It would probably be good if I could merge this branch without +>>> having to worry about also immediatly merging that one. --[[Joey]] +>>> +>>>> I removed all dependencies on my `meta` branch from the `po` one. +>>>> This implied removing the `po_translation_status_in_links` and +>>>> `po_strictly_refresh_backlinks` features, and every link text is now +>>>> displayed in the master language. I believe the removed features really +>>>> enhance user experience of a translatable wiki, that's why I was +>>>> initially supposing the `meta` branch would be merged first. +>>>> IMHO, we'll need to come back to this quite soon after `po` is merged. +>>>> --[[intrigeri]] +>>>> +>>>> Maybe you should keep those features in a meta-po branch? +>>>> I did a cursory review of your meta last night, have some issues with it, +>>>> but this page isn't the place for a detailed review. --[[Joey]] +>>>> +>>>>> Done. --[[intrigeri]] +>>> +> * I'm very fearful of the `add_depends` in `postscan`. +> Does this make every page depend on every page that links +> to it? Won't this absurdly bloat the dependency pagespecs +> and slow everything down? And since nicepagetitle is given +> as the reason for doing it, and nicepagetitle isn't used, +> why do it? +>> +>> As explained in the 85f865b5d98e0122934d11e3f3eb6703e4f4c620 log: +>> this feature hits performance a bit. Its cost was quite small in my +>> real-world use-cases (a few percents bigger refresh time), but +>> could be bigger in worst cases. When using the po plugin with my +>> meta branch changes (i.e. the `nicepagetitle` thing), and having +>> enabled the option to display translation status in links, this +>> maintains the translation status up-to-date in backlinks. Same when +>> using meta title to make the pages titles translatable. It does +>> help having a nice and consistent translated wiki, but as it can +>> also involve problems, I just turned it into an option. +>> +>>> This has been completely removed for now due to the removal of +>>> the dependency on my `meta` branch. --[[intrigeri]] +>> +> * The po4a Suggests should be versioned to the first version +> that can be used safely, and that version documented in +> `plugins/po.mdwn`. +>> +>> Done. +>> +>> --[[intrigeri]] +> +> --[[Joey]] + +I reverted the `%backlinks` and `$backlinks_calculated` exposing. +The issue they were solving probably will arise again when I'll work +on my meta branch again (i.e. when the simplified po one is merged), +but the po thing is supposed to work without these ugly `our`. +Seems like it was the last unaddressed item from Joey's review, so I'm +daring a timid "please pull"... or rather, please review again :) +--[[intrigeri]] + +> Ok, I've reviewed and merged into my own po branch. It's looking very +> mergeable. +> +> * Is it worth trying to fix compatability with `indexpages`? +>> +>> Supporting `usedirs` being enabled or disabled was already quite +>> hard IIRC, so supporting all four combinations of `usedirs` and +>> `indexpages` settings will probably be painful. I propose we forget +>> about it until someone reports he/she badly needs it, and then +>> we'll see what can be done. +>> +> * Would it make sense to go ahead and modify `page.tmpl` to use +> OTHERLANGUAGES and PERCENTTRANSLATED, instead of documenting how to modify it? +>> +>> Done in my branch. +>> +> * Would it be better to disable po support for pages that use unsupported +> or poorly-supported markup languages? +> +>> I prefer keeping it enabled, as: +>> +>> * most wiki markups "almost work" +>> * when someone needs one of these to be fully supported, it's not +>> that hard to add dedicated support for it to po4a; if it were +>> disabled, I fear the ones who could do this would maybe think +>> it's blandly impossible and give up. +>> + +> * What's the reasoning behind checking that the link plugin +> is enabled? AFAICS, the same code in the scan hook should +> also work when other link plugins like camelcase are used. +> * In `pagetemplate` there is a comment that claims the code +> relies on `genpage`, but I don't see how it does; it seems +> to always add a discussion link? +> * Is there any real reason not to allow removing a translation? +> I'm imagining a spammy translation, which an admin might not +> be able to fix, but could remove. +> * Re the meta title escaping issue worked around by `change`. +> I suppose this does not only affect meta, but other things +> at scan time too. Also, handling it only on rebuild feels +> suspicious -- a refresh could involve changes to multiple +> pages and trigger the same problem, I think. Also, exposing +> this rebuild to the user seems really ugly, not confidence inducing. +> +> So I wonder if there's a better way. Such as making po, at scan time, +> re-run the scan hooks, passing them modified content (either converted +> from po to mdwn or with the escaped stuff cheaply de-escaped). (Of +> course the scan hook would need to avoid calling itself!) +> +> (This doesn't need to block the merge, but I hope it can be addressed +> eventually..) +> +> --[[Joey]] +>> +>> --[[intrigeri]]