Page MenuHomePhabricator

Clean up the rendering of messages displayed at the top of the edit window
Open, LowPublic

Description

Author: happy_melon

Description:
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
  1. 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)
  1. 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


Version: unspecified
Severity: minor

attachment editpage messages.txt ignored as private

Details

Reference
bz16175

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 10:17 PM
bzimport set Reference to bz16175.

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.

Amire80 removed Amire80 as the assignee of this task.Jan 24 2015, 1:59 PM
Amire80 added a subscriber: Amire80.
matmarex removed a subscriber: matmarex.Oct 5 2018, 5:48 PM