happy.melon.wiki wrote:

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

attachment diff.txt ignored as obsolete

happy.melon.wiki wrote:

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.

Attached:

Why are you turning addWikiMsg and wrapWikiMsg calls to something horrible?

happy.melon.wiki wrote:

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' );

happy.melon.wiki wrote:

(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.

sumanah wrote:

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.

sumanah wrote:

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.