r82343 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r82342‎ | r82343 | r82344 >
Date:18:41, 17 February 2011
Author:mah
Status:reverted (Comments)
Tags:
Comment:
Followup bug #27447 and r82308 and r82328 with patch supplied by
Carsten Nielsen to take care of the warning and supply some
documentation. Also fixed up language in the comments.
Modified paths:
  • /trunk/phase3/includes/EditPage.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/EditPage.php
@@ -2256,16 +2256,19 @@
22572257 $imagesAvailable = $wgEnableUploads || count( $wgForeignFileRepos );
22582258
22592259 /**
2260 -
2261 - * toolarray an array of arrays which each include the filename of
2262 - * the button image (without path), the opening tag, the closing tag,
2263 - * and optionally a sample text that is inserted between the two when no
2264 - * selection is highlighted.
2265 - * The tip text is shown when the user moves the mouse over the button.
 2260+ * $toolarray is an array of arrays each of which includes the
 2261+ * filename of the button image (without path), the opening
 2262+ * tag, the closing tag, optionally a sample text that is
 2263+ * inserted between the two when no selection is highlighted
 2264+ * and an option to select which switches the automatic
 2265+ * selection of inserted text (default is true, see
 2266+ * mw-editbutton-image). The tip text is shown when the user
 2267+ * moves the mouse over the button.
22662268 *
2267 - * Already here are accesskeys (key), which are not used yet until someone
2268 - * can figure out a way to make them work in IE. However, we should make
2269 - * sure these keys are not defined on the edit page.
 2269+ * Also here: accesskeys (key), which are not used yet until
 2270+ * someone can figure out a way to make them work in
 2271+ * IE. However, we should make sure these keys are not defined
 2272+ * on the edit page.
22702273 */
22712274 $toolarray = array(
22722275 array(
@@ -2320,7 +2323,8 @@
23212324 'close' => ']]',
23222325 'sample' => wfMsg( 'image_sample' ),
23232326 'tip' => wfMsg( 'image_tip' ),
2324 - 'key' => 'D'
 2327+ 'key' => 'D',
 2328+ 'select' => true
23252329 ) : false,
23262330 $imagesAvailable ? array(
23272331 'image' => $wgLang->getImageFile( 'button-media' ),
@@ -2376,6 +2380,10 @@
23772381 continue;
23782382 }
23792383
 2384+ if( !isset( $tool['select'] ) ) {
 2385+ $tool['select'] = true;
 2386+ }
 2387+
23802388 $params = array(
23812389 $image = $wgStylePath . '/common/images/' . $tool['image'],
23822390 // Note that we use the tip both for the ALT tag and the TITLE tag of the image.

Follow-up revisions

RevisionCommit summaryAuthorDate
r92715Revert non-functional parts of r82343 -- 'select' parameter that's never actu...brion23:51, 20 July 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r82308(Bug #27447) Added option to toolbar-item to switch off text-selection...mah23:05, 16 February 2011
r82328Rv part of r82308 that causes warningsnikerabbit10:12, 17 February 2011

Comments

#Comment by Krinkle (talk | contribs)   00:26, 2 March 2011

Untested but from what I see this won't work.

Context:

2378                foreach ( $toolarray as $tool ) {
2379	                        if ( !$tool ) {
2380	                                continue;
2381	                        }
2382	
2383	                        if( !isset( $tool['select'] ) ) {
2384	                          $tool['select'] = true;
2385	                        }
2386	
2387	                        $params = array(
2388	                                $image = $wgStylePath . '/common/images/' . $tool['image'],
2389	                                // Note that we use the tip both for the ALT tag and the TITLE tag of the image.
2390	                                // Older browsers show a "speedtip" type message only for ALT.
2391	                                // Ideally these should be different, realistically they
2392	                                // probably don't need to be.
2393	                                $tip = $tool['tip'],
2394	                                $open = $tool['open'],
2395	                                $close = $tool['close'],
2396	                                $sample = $tool['sample'],
2397	                                $cssId = $tool['id'],
2398	                        );
2399	
2400	                        $paramList = implode( ',',
2401	                                array_map( array( 'Xml', 'encodeJsVar' ), $params ) );
2402	                        $script .= "addButton($paramList);\n";
2403	                }
2404	

It needs to be passed to $params as well:

+	                              $select = $tool['select],
</ore>
#Comment by Brion VIBBER (talk | contribs)   23:53, 20 July 2011

As noted above the code does nothing:

  • $tool['sample'] always ends up as true
  • the value then never gets output or used anyway

Since this doesn't seem like something that even makes sense to be switchable here, just reverting everything except some of the code comment cleanup.

#Comment by Brion VIBBER (talk | contribs)   23:53, 20 July 2011

revert done in r92715.

Status & tagging log