Last modified: 2014-09-23 23:53:15 UTC
Created attachment 5485 [details] Improve system message handling on edit page Patch to do a couple of things to the messages that can be displayed at the top of the edit window when editing, previewing, etc. 1) Make the log extracts displayed underneath [[MediaWiki:Protectedpagewarning]], etc, parameters to the message itself, so that styles and structures can be easily applied to the whole object. I only made the necessary changes to the default messages to the english ones... is that bad? :S 2) Construct the log extract for [[MediaWiki:Recreate-deleted-warn]] in the same way as for the other extracts (seems more elegant, previous version was very similar to LogEventsList::showLogExtract() anyway) 3) Wrap at least most messages in an id'd div for ease of skinning. This seems a very common operation: worth a wrapper function in OutputPage.php? This is my first serious patch, and more to the point I don't have a working version of MediaWiki that I can test changes on, so **THIS NEEDS TESTING** before making it live anywhere... god knows what I've broken this time :D
Created attachment 6354 [details] Updated patch, against r53416 Original patch is now 11,000 revisions out of date. Updated patch no longer does point 1: we have found an elegant way to work around this issue over at enwiki, so it's no longer worth me pushing a breaking change. Tested against r53416. Overhauled EditPage::showLogs() requires the updated LogEventsList::showLogExtract() patched in bug17979
Created attachment 6360 [details] Better patch, still against r53416 Version that actually *uses* the new functionality in LogEventsList::showLogExtract(). That should have been bug17979, which is actually duped to bug18880.
Why are you turning addWikiMsg and wrapWikiMsg calls to something horrible?
Would you rather add *another* wfMsgXxx function, that automatically adds the "mw-<message_name>" id? I'd be equally happy with that, but I was under the impression that we were moving *away* from using zillions of little variations of wfMsg functions? Xml::tags should be used (either directly or through such a wrapper) as good coding practice; plus it'll be useful/necessary for, eg, the transition to HTML5 that's currently being discussed on wikitech-l. It looks like eventually we'll switch, and then we can remove the closing </div> tag and some of the attribute quotes. I'd rather be able to do that just by changing Xml::tags than have to go trawling through the codebase looking for hardcoded divs. This is elementary software encapsulation, no?
You can very well add the id with wrapWikiMsg too, (are you sure classes are not enough? we've got jQuery soon). Don't over engineer, because I have nowhere seen even a suggestion that we would change what Xml-functions would do. The only proposal was to create a new HTML class by Simetrical, which sounds much more sane. What you are suggesting now, is not readable, even ignoring the unsusual indendation. And oh, did you mention that you changed some class names too? And if you really really want it, you can even combine wrapWikiMsg and Xml-functions, something like this: ->wrapWikiMsg( Xml::element( 'div', array( 'class' => 'foo', 'id' => 'daa' ), '$1' ), 'baz' );
(In reply to comment #5) > You can very well add the id with wrapWikiMsg too, (are you sure classes are > not enough? we've got jQuery soon). Don't over engineer, IMO this is just standard encapsulation; in the same way that we are now hunting down and destroying all hardcoded SQL statements to allow proper database abstraction, the codebase would, in an ideal world, not contain any hardcoded HTML for the same reasons. > because I have nowhere > seen even a suggestion that we would change what Xml-functions would do. The > only proposal was to create a new HTML class by Simetrical, which sounds much > more sane. Which would then be deployed by a steady conversion of "Xml::..." to "Html::..." across the codebase. Which can be greped for, unlike hardcoded instances. > > What you are suggesting now, is not readable, even ignoring the unsusual > indendation. And oh, did you mention that you changed some class names too? Where? "error" is added to cascadeprotectedwarning and titleprotectedwarning to be consistent with longpage-error, but I don't see any classes *changed*. I'm not wedded to the "error" class; I can see the distinction between longpage-error and the other notices. I used "id=mw-*edit*-..." to match the one warning (longpage-error) that *does* currently have an ID; I don't like it (they should be the standard "mw-<message_name>" like everywhere else), but it was to *avoid* changing ids. > if you really really want it, you can even combine wrapWikiMsg and > Xml-functions, something like this: > > ->wrapWikiMsg( Xml::element( 'div', array( 'class' => 'foo', 'id' => 'daa' ), > '$1' ), 'baz' ); > What's better about that? The $1 string injection in wrapWikiMsg() is *even more* obfuscated than addHTML(). As I said, I'd be perfecly happy to add another global function, but I was under the impression that we wanted fewer message functions (and global functions generally), not more. Maybe a method in OutputPage would be preferable... $wgOut->outputMsgDiv() or somesuch...?
This bug reminds me about Bug 19848.
Volunteered by Amir during bug triage.
Amir, do you think you will have time to review and resolve this sometime soon, perhaps in your 20% time in the next few weeks? Thanks.
Yep, I actually planned to go over the bugs assigned to me Real Soon Now.
MatmaRex, can you take a look at this?
I could try to once I get some of the 17 pending patchsets I have on gerrit merged.
