r88111 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r88110‎ | r88111 | r88112 >
Date:17:05, 14 May 2011
Author:freakolowsky
Status:reverted (Comments)
Tags:
Comment:
* added an option to toggle the display of diff size with a warning message.
* in misermode this option is disabled
Modified paths:
  • /trunk/phase3/includes/specials/SpecialContributions.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)
  • /trunk/phase3/maintenance/language/messages.inc (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/language/messages.inc
——@@ -2014,6 +2014,8 @@
20152015 'sp-contributions-explain',
20162016 'sp-contributions-footer',
20172017 'sp-contributions-footer-anon',
 2018+ 'sp-contributions-showsizediff',
 2019+ 'sp-contributions-showsizediff-warn',
20182020 ),
20192021 'whatlinkshere' => array(
20202022 'whatlinkshere',
Index: trunk/phase3/includes/specials/SpecialContributions.php
——@@ -69,6 +69,7 @@
7070 $this->opts['limit'] = $wgRequest->getInt( 'limit', $wgUser->getOption('rclimit') );
7171 $this->opts['target'] = $target;
7272 $this->opts['topOnly'] = $wgRequest->getBool( 'topOnly' );
 73+ $this->opts['showSizeDiff'] = $wgRequest->getBool( 'showSizeDiff' );
7374
7475 $nt = Title::makeTitleSafe( NS_USER, $target );
7576 if( !$nt ) {
——@@ -132,6 +133,7 @@
133134 'month' => $this->opts['month'],
134135 'deletedOnly' => $this->opts['deletedOnly'],
135136 'topOnly' => $this->opts['topOnly'],
 137+ 'showSizeDiff' => $this->opts['showSizeDiff'],
136138 ) );
137139 if( !$pager->getNumRows() ) {
138140 $wgOut->addWikiMsg( 'nocontribs', $target );
——@@ -317,7 +319,7 @@
318320 * @return String: HTML fragment
319321 */
320322 protected function getForm() {
321 - global $wgScript, $wgMiserMode;
 323+ global $wgScript, $wgMiserMode, $wgRCMaxAge, $wgContLang;
322324
323325 $this->opts['title'] = $this->getTitle()->getPrefixedText();
324326 if( !isset( $this->opts['target'] ) ) {
——@@ -354,10 +356,14 @@
355357 $this->opts['topOnly'] = false;
356358 }
357359
 360+ if( !isset( $this->opts['showSizeDiff'] ) ) {
 361+ $this->opts['showSizeDiff'] = !$wgMiserMode;
 362+ }
 363+
358364 $f = Xml::openElement( 'form', array( 'method' => 'get', 'action' => $wgScript, 'class' => 'mw-contributions-form' ) );
359365
360366 # Add hidden params for tracking except for parameters in $skipParameters
361 - $skipParameters = array( 'namespace', 'deletedOnly', 'target', 'contribs', 'year', 'month', 'topOnly' );
 367+ $skipParameters = array( 'namespace', 'deletedOnly', 'target', 'contribs', 'year', 'month', 'topOnly', 'showSizeDiff' );
362368 foreach ( $this->opts as $name => $value ) {
363369 if( in_array( $name, $skipParameters ) ) {
364370 continue;
——@@ -366,13 +372,21 @@
367373 }
368374
369375 $tagFilter = ChangeTags::buildTagFilterSelector( $this->opts['tagFilter'] );
 376+
 377+ $fNS = '';
 378+ $fShowDiff = '';
 379+ if ( !$wgMiserMode ) {
 380+ $fNS = Html::rawElement( 'span', array( 'style' => 'white-space: nowrap' ),
 381+ Xml::label( wfMsg( 'namespace' ), 'namespace' ) . ' ' .
 382+ Xml::namespaceSelector( $this->opts['namespace'], '' )
 383+ );
 384+ $fShowDiff = Xml::infoBox(
 385+ Xml::checkLabel( wfMsg( 'sp-contributions-showsizediff' ), 'showSizeDiff', 'mw-show-size-diff', $this->opts['showSizeDiff'] ) . '<br />'.
 386+ wfMsgReplaceArgs ( wfMsg( 'sp-contributions-showsizediff-warn' ), array( $wgContLang->formatTimePeriod( $wgRCMaxAge ) ) )
 387+ , 'warning-32.png', wfMsg( 'sp-contributions-showsizediff' )
 388+ );
 389+ }
370390
371 - $fNS = ( $wgMiserMode ) ? '' :
372 - Html::rawElement( 'span', array( 'style' => 'white-space: nowrap' ),
373 - Xml::label( wfMsg( 'namespace' ), 'namespace' ) . ' ' .
374 - Xml::namespaceSelector( $this->opts['namespace'], '' )
375 - );
376 -
377391 $f .= Xml::fieldset( wfMsg( 'sp-contributions-search' ) ) .
378392 Xml::radioLabel( wfMsgExt( 'sp-contributions-newbies', array( 'parsemag' ) ),
379393 'contribs', 'newbie' , 'newbie', $this->opts['contribs'] == 'newbie' ) . '<br />' .
——@@ -387,6 +401,7 @@
388402 'deletedOnly', 'mw-show-deleted-only', $this->opts['deletedOnly'] ) . '<br />' .
389403 Xml::tags( 'p', null, Xml::checkLabel( wfMsg( 'sp-contributions-toponly' ),
390404 'topOnly', 'mw-show-top-only', $this->opts['topOnly'] ) ) .
 405+ $fShowDiff.
391406 ( $tagFilter ? Xml::tags( 'p', null, implode( '&#160;', $tagFilter ) ) : '' ) .
392407 Html::rawElement( 'p', array( 'style' => 'white-space: nowrap' ),
393408 Xml::dateMenu( $this->opts['year'], $this->opts['month'] ) . ' ' .
——@@ -436,6 +451,7 @@
437452 'tagFilter' => $this->opts['tagFilter'],
438453 'deletedOnly' => $this->opts['deletedOnly'],
439454 'topOnly' => $this->opts['topOnly'],
 455+ 'showSizeDiff' => $this->opts['showSizeDiff'],
440456 ) );
441457
442458 $pager->mLimit = min( $this->opts['limit'], $wgFeedLimit );
——@@ -523,6 +539,7 @@
524540
525541 $this->deletedOnly = !empty( $options['deletedOnly'] );
526542 $this->topOnly = !empty( $options['topOnly'] );
 543+ $this->showSizeDiff = !empty( $options['showSizeDiff'] );
527544
528545 $year = isset( $options['year'] ) ? $options['year'] : false;
529546 $month = isset( $options['month'] ) ? $options['month'] : false;
——@@ -538,7 +555,7 @@
539556 }
540557
541558 function getQueryInfo() {
542 - global $wgUser;
 559+ global $wgUser, $wgMiserMode;
543560 list( $tables, $index, $userCond, $join_cond ) = $this->getUserCond();
544561
545562 $conds = array_merge( $userCond, $this->getNamespaceCond() );
——@@ -550,20 +567,25 @@
551568 ' != ' . Revision::SUPPRESSED_USER;
552569 }
553570 $join_cond['page'] = array( 'INNER JOIN', 'page_id=rev_page' );
 571+
 572+ $fields = array(
 573+ 'page_namespace', 'page_title', 'page_is_new', 'page_latest', 'page_is_redirect',
 574+ 'page_len','rev_id', 'rev_page', 'rev_text_id', 'rev_timestamp', 'rev_comment',
 575+ 'rev_minor_edit', 'rev_user', 'rev_user_text', 'rev_parent_id', 'rev_deleted'
 576+ );
 577+ if ( $this->showSizeDiff && !$wgMiserMode ) {
 578+ $fields = array_merge( $fields, array( 'rc_old_len', 'rc_new_len' ) );
 579+ array_unshift( $tables, 'recentchanges' );
 580+ $join_cond['recentchanges'] = array( 'INNER JOIN', "rev_id = rc_this_oldid" );
 581+ }
554582
555583 $queryInfo = array(
556584 'tables' => $tables,
557 - 'fields' => array(
558 - 'page_namespace', 'page_title', 'page_is_new', 'page_latest', 'page_is_redirect',
559 - 'page_len','rev_id', 'rev_page', 'rev_text_id', 'rev_timestamp', 'rev_comment',
560 - 'rev_minor_edit', 'rev_user', 'rev_user_text', 'rev_parent_id', 'rev_deleted',
561 - 'rc_old_len', 'rc_new_len'
562 - ),
 585+ 'fields' => $fields,
563586 'conds' => $conds,
564587 'options' => array( 'USE INDEX' => array('revision' => $index) ),
565588 'join_conds' => $join_cond
566589 );
567 -
568590 ChangeTags::modifyDisplayQuery(
569591 $queryInfo['tables'],
570592 $queryInfo['fields'],
——@@ -580,19 +602,18 @@
581603 function getUserCond() {
582604 $condition = array();
583605 $join_conds = array();
 606+
584607 if( $this->target == 'newbies' ) {
585 - $tables = array( 'recentchanges', 'user_groups', 'page', 'revision' );
 608+ $tables = array( 'user_groups', 'page', 'revision' );
586609 $max = $this->mDb->selectField( 'user', 'max(user_id)', false, __METHOD__ );
587610 $condition[] = 'rev_user >' . (int)($max - $max / 100);
588611 $condition[] = 'ug_group IS NULL';
589612 $index = 'user_timestamp';
590613 # FIXME: other groups may have 'bot' rights
591614 $join_conds['user_groups'] = array( 'LEFT JOIN', "ug_user = rev_user AND ug_group = 'bot'" );
592 - $join_conds['recentchanges'] = array( 'INNER JOIN', "rev_id = rc_this_oldid" );
593615 } else {
594 - $tables = array( 'recentchanges', 'page', 'revision' );
 616+ $tables = array( 'page', 'revision' );
595617 $condition['rev_user_text'] = $this->target;
596 - $join_conds['recentchanges'] = array( 'INNER JOIN', "rev_id = rc_this_oldid" );
597618 $index = 'usertext_timestamp';
598619 }
599620 if( $this->deletedOnly ) {
——@@ -739,7 +760,8 @@
740761
741762 $diffHistLinks = '(' . $difftext . $this->messages['pipe-separator'] . $histlink . ')';
742763
743 - $diffOut = ' . . '.ChangesList::showCharacterDifference( $row->rc_old_len, $row->rc_new_len );
 764+
 765+ $diffOut = ( $this->showSizeDiff ) ? ' . . '.ChangesList::showCharacterDifference( $row->rc_old_len, $row->rc_new_len ) : '';
744766
745767 $ret = "{$del}{$d} {$diffHistLinks} {$nflag}{$mflag} {$link}{$diffOut}{$userlink} {$comment} {$topmarktext}";
746768
Index: trunk/phase3/languages/messages/MessagesEn.php
——@@ -2992,6 +2992,8 @@
29932993 'sp-contributions-search' => 'Search for contributions',
29942994 'sp-contributions-username' => 'IP address or username:',
29952995 'sp-contributions-toponly' => 'Only show edits that are latest revisions',
 2996+'sp-contributions-showsizediff' => 'Display difference in page size',
 2997+'sp-contributions-showsizediff-warn' => 'This instance has recent changes log limited to $1 . If you use this option, pages not in this log will not be displayed',
29962998 'sp-contributions-submit' => 'Search',
29972999 'sp-contributions-explain' => '', # only translate this message to other languages if you have to change it
29983000 'sp-contributions-footer' => '-', # do not translate or duplicate this message to other languages

Follow-up revisions

RevisionCommit summaryAuthorDate
r88117* added alternative revison size display if diff size is not selectedfreakolowsky22:06, 14 May 2011
r88252Remove verboseness (image and infobox) in Special:Contributions for diff size...siebrand17:51, 16 May 2011
r95496Revert r8811...reedy17:35, 25 August 2011
r96306Revert r88008 (add size difference to Special:Contributions) and its large gr...catrope21:47, 5 September 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r88008* bug 24037freakolowsky17:11, 13 May 2011

Comments

#Comment by Duplicatebug (talk | contribs)   18:27, 14 May 2011

Can you add the database field rev_len with Linker::formatRevisionSize() like done with r88048 for DeletedContributes, when not showing the byte difference? Thanks.

#Comment by Nikerabbit (talk | contribs)   06:21, 15 May 2011

I think that the checkbox should be removed, all pages should be shown, but display warning only when there are pages where there is pages without information. Is that possible?

#Comment by Aaron Schulz (talk | contribs)   02:24, 24 June 2011

I agree. It can be disable if $wgMiserMode is set.

Either that or all of these should be reverted.

#Comment by Freakolowsky (talk | contribs)   08:54, 15 May 2011

Nike some database guy i will leave unnamed will probably mind if i start doing outer joins. It is possible but after chatting with him about this i think it's just to expensive to do that.

And i don't think if someone just feels something should be done their was warrants the "fixme" status.

#Comment by Nikerabbit (talk | contribs)   09:49, 15 May 2011

If nothing else the notice is in the wrong place and needs some work: 

This instance has recent changes log limited to 43,800h 0m 0s . If you use this option, pages not in this log will not be displayed
#Comment by Nikerabbit (talk | contribs)   09:52, 15 May 2011

Besides, this is disabled in WMF already, so having it a bit slower for others wouldn't be an issue for them?

#Comment by Freakolowsky (talk | contribs)   11:25, 15 May 2011

ok, true for the "needs more work" part. I'm not that good with messages, it took me some effort to not just put "Beware! There be dragons!!" in the message. And we need to extend that method to include days/months/years.

But even if this is disabled on WMF it is still not that great of an idea to do that on a bigger instance running a standard mysql db. wgRCMaxAge has a default of 13 weeks, so for most cases this situation would work anyway. For the edge case ... displaying this diff size is handy ... and i don't think something that has no specific functionality other than being handy deserves to make such expensive operations.

I might be overthinking this, tho ...

#Comment by Freakolowsky (talk | contribs)   20:43, 16 May 2011

This got reverted or 'tleast changed by the following revies, so i'm changing it from fixme to new

#Comment by Bawolff (talk | contribs)   21:16, 29 June 2011

I'm really not a fan of the UI for this feature (just happened to be browsing special:contribs today on trunk). Having that many checkboxes on special:contribs seems confusing and just too much text.

#Comment by Aaron Schulz (talk | contribs)   22:05, 29 June 2011

$join_cond['recentchanges'] = array( 'INNER JOIN', "rev_id = rc_this_oldid" );

At least add a rev_timestamp = rc_timestamp clause to use an index.

#Comment by Bawolff (talk | contribs)   22:08, 29 June 2011

Umm, this (as noted already) will limit the results of special:contribs to things still in RC table (since its an inner join). RC table gets truncated regularly. I don't think we should have a special:contribs feature that breaks for contribs from over 3 months ago. (/me suggests revert)

#Comment by Aaron Schulz (talk | contribs)   22:09, 29 June 2011

I'll have to side with reversion on this one (mostly from a UX standpoint).

Status & tagging log