r73030 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r73029‎ | r73030 | r73031 >
Date:00:36, 15 September 2010
Author:tparscal
Status:deferred (Comments)
Tags:
Comment:
Intitial import. Ported from UsabilityInitiative/Vector
Modified paths:
  • /trunk/extensions/Vector (added) (history)
  • /trunk/extensions/Vector/Vector.hooks.php (added) (history)
  • /trunk/extensions/Vector/Vector.i18n.php (added) (history)
  • /trunk/extensions/Vector/Vector.php (added) (history)
  • /trunk/extensions/Vector/modules (added) (history)

Diff [purge]

Index: trunk/extensions/Vector/Vector.i18n.php
——@@ -0,0 +1,24 @@
 2+<?php
 3+/**
 4+ * Internationalisation for Vector extension
 5+ *
 6+ * @file
 7+ * @ingroup Extensions
 8+ */
 9+
 10+$messages = array();
 11+
 12+/** English
 13+ * @author Trevor Parscal
 14+ */
 15+$messages['en'] = array(
 16+ 'vector' => 'UI improvements for Vector',
 17+ 'vector-desc' => 'Improves on the user interface elements of the Vector skin.',
 18+ 'vector-collapsiblenav-preference' => 'Enable collapsing of items in the navigation menu in Vector skin',
 19+ 'vector-collapsiblenav-more' => 'More languages',
 20+ 'vector-editwarning-warning' => 'Leaving this page may cause you to lose any changes you have made.
 21+If you are logged in, you can disable this warning in the "Editing" section of your preferences.',
 22+ 'vector-editwarning-preference' => 'Warn me when I leave an edit page with unsaved changes',
 23+ 'vector-simplesearch-search' => 'Search',
 24+ 'vector-simplesearch-containing' => 'containing...',
 25+);
Property changes on: trunk/extensions/Vector/Vector.i18n.php
___________________________________________________________________
Added: svn:eol-style
126 + native
Index: trunk/extensions/Vector/Vector.php
——@@ -0,0 +1,51 @@
 2+<?php
 3+/**
 4+ * Vector extension
 5+ *
 6+ * @file
 7+ * @ingroup Extensions
 8+ *
 9+ * @author Trevor Parscal <trevor@wikimedia.org>
 10+ * @author Roan Kattouw <roan.kattouw@gmail.com>
 11+ * @author Nimish Gautam <nimish@wikimedia.org>
 12+ * @author Adam Miller <amiller@wikimedia.org>
 13+ * @license GPL v2 or later
 14+ * @version 0.2.0
 15+ */
 16+
 17+/* Configuration */
 18+
 19+// Each module may be configured individually to be globally on/off or user preference based
 20+$wgVectorModules = array(
 21+ 'collapsiblenav' => array( 'global' => true, 'user' => true ),
 22+ 'collapsibletabs' => array( 'global' => true, 'user' => false ),
 23+ 'editwarning' => array( 'global' => false, 'user' => true ),
 24+ 'expandablesearch' => array( 'global' => false, 'user' => true ),
 25+ 'footercleanup' => array( 'global' => false, 'user' => false ),
 26+ 'simplesearch' => array( 'global' => false, 'user' => true ),
 27+);
 28+
 29+// The Vector skin has a basic version of simple search, which is a prerequisite for the enhanced one
 30+$wgDefaultUserOptions['vector-simplesearch'] = 1;
 31+
 32+// Enable bucket testing for new version of collapsible nav
 33+$wgCollapsibleNavBucketTest = false;
 34+// Force the new version
 35+$wgCollapsibleNavForceNewVersion = false;
 36+
 37+/* Setup */
 38+
 39+$wgExtensionCredits['other'][] = array(
 40+ 'path' => __FILE__,
 41+ 'name' => 'Vector',
 42+ 'author' => array( 'Trevor Parscal', 'Roan Kattouw', 'Nimish Gautam', 'Adam Miller' ),
 43+ 'version' => '0.3.0',
 44+ 'url' => 'http://www.mediawiki.org/wiki/Extension:Vector',
 45+ 'descriptionmsg' => 'vector-desc',
 46+);
 47+$wgAutoloadClasses['VectorHooks'] = dirname( __FILE__ ) . '/Vector.hooks.php';
 48+$wgExtensionMessagesFiles['Vector'] = dirname( __FILE__ ) . '/Vector.i18n.php';
 49+$wgHooks['BeforePageDisplay'][] = 'VectorHooks::beforePageDisplay';
 50+$wgHooks['GetPreferences'][] = 'VectorHooks::getPreferences';
 51+$wgHooks['MakeGlobalVariablesScript'][] = 'VectorHooks::MakeGlobalVariablesScript';
 52+$wgHooks['ResourceLoaderRegisterModules'][] = 'VectorHooks::resourceLoaderRegisterModules';
\ No newline at end of file
Property changes on: trunk/extensions/Vector/Vector.php
___________________________________________________________________
Added: svn:eol-style
153 + native
Index: trunk/extensions/Vector/Vector.hooks.php
——@@ -0,0 +1,195 @@
 2+<?php
 3+/**
 4+ * Hooks for Vector extension
 5+ *
 6+ * @file
 7+ * @ingroup Extensions
 8+ */
 9+
 10+class VectorHooks {
 11+
 12+ /* Static Members */
 13+
 14+ static $modules = array(
 15+ 'collapsiblenav' => array(
 16+ 'name' => 'vector.collapsibleNav',
 17+ 'resources' => array(
 18+ 'scripts' => '',
 19+ 'styles' => '',
 20+ 'messages' => array(
 21+ 'vector-collapsiblenav-more',
 22+ ),
 23+ )
 24+ 'preferences' => array(
 25+ 'key' => 'vector-collapsiblenav',
 26+ 'ui' => array(
 27+ 'type' => 'toggle',
 28+ 'label-message' => 'vector-collapsiblenav-preference',
 29+ 'section' => 'rendering/advancedrendering',
 30+ ),
 31+ ),
 32+ 'configurations' => array(
 33+ 'wgCollapsibleNavBucketTest',
 34+ 'wgCollapsibleNavForceNewVersion',
 35+ ),
 36+ ),
 37+ 'collapsibletabs' => array(
 38+ 'name' => 'vector.collapsibleTabs',
 39+ 'resources' => array(
 40+ 'scripts' => '',
 41+ 'styles' => '',
 42+ )
 43+ ),
 44+ 'editwarning' => array(
 45+ 'name' => 'vector.editWarning',
 46+ 'resources' => array(
 47+ 'scripts' => '',
 48+ 'styles' => '',
 49+ 'messages' => array(
 50+ 'vector-editwarning-warning',
 51+ ),
 52+ ),
 53+ 'preferences' => array(
 54+ // Ideally this would be 'vector-editwarning'
 55+ 'key' => 'useeditwarning',
 56+ 'ui' => array(
 57+ 'type' => 'toggle',
 58+ 'label-message' => 'vector-editwarning-preference',
 59+ 'section' => 'editing/advancedediting',
 60+ ),
 61+ ),
 62+ ),
 63+ 'expandablesearch' => array(
 64+ 'name' => 'vector.expandableSearch',
 65+ 'resources' => array(
 66+ 'scripts' => '',
 67+ 'styles' => '',
 68+ ),
 69+ 'preferences' => array(
 70+ 'requirements' = array( 'vector-simplesearch', 'disablesuggest' ),
 71+ ),
 72+ ),
 73+ 'footercleanup' => array(
 74+ 'name' => 'vector.footerCleanup',
 75+ 'resources' => array(
 76+ 'scripts' => '',
 77+ 'styles' => '',
 78+ ),
 79+ ),
 80+ 'simplesearch' => array(
 81+ 'name' => 'vector.simpleSearch',
 82+ 'resources' => array(
 83+ 'scripts' => '',
 84+ 'styles' => '',
 85+ 'messages' => array(
 86+ 'vector-simplesearch-search',
 87+ 'vector-simplesearch-containing',
 88+ ),
 89+ ),
 90+ 'preferences' => array(
 91+ 'requirements' = array( 'vector-simplesearch', 'disablesuggest' ),
 92+ ),
 93+ ),
 94+ );
 95+
 96+ /* Protected Static Methods */
 97+
 98+ protected static isEnabled( $module ) {
 99+ global $wgVectorModules, $wgUser;
 100+
 101+ $enabled =
 102+ $wgVectorModules[$module]['global'] ||
 103+ (
 104+ $wgVectorModules[$module]['user'] &&
 105+ isset( self::$modules[$module]['preferences']['key'] ) &&
 106+ $wgUser->getOption( self::$modules[$module]['preferences']['key']
 107+ );
 108+ if ( !$enabled ) {
 109+ return false;
 110+ }
 111+ foreach ( self::$modules[$module]['preferences']['requirements'] as $requirement ) {
 112+ if ( !$wgUser->getOption( $requirement ) ) {
 113+ return false;
 114+ }
 115+ }
 116+ return true;
 117+ }
 118+
 119+ /* Static Methods */
 120+
 121+ /**
 122+ * BeforePageDisplay hook
 123+ *
 124+ * Adds the modules to the edit form
 125+ *
 126+ * @param $out OutputPage output page
 127+ * @param $skin Skin current skin
 128+ */
 129+ public static function beforePageDisplay( $out, $skin ) {
 130+ global $wgVectorModules;
 131+
 132+ // Don't load Vector modules for non-Vector skins
 133+ if ( !( $skin instanceof SkinVector ) ) {
 134+ return true;
 135+ }
 136+
 137+ // Add enabled modules
 138+ foreach ( $wgVectorModules as $module => $enable ) {
 139+ if ( self::isEnabled( $module ) ) {
 140+ $out->addModules( self::$modules[$module]['name'] );
 141+ }
 142+ }
 143+ }
 144+
 145+ /**
 146+ * GetPreferences hook
 147+ *
 148+ * Adds Vector-releated items to the preferences
 149+ *
 150+ * @param $out User current user
 151+ * @param $skin array list of default user preference controls
 152+ */
 153+ public static function getPreferences( $user, &$defaultPreferences ) {
 154+ global $wgVectorModules;
 155+
 156+ foreach ( $wgVectorModules as $module => $enable ) {
 157+ if ( $enable['user'] ) && isset( self::$modules['preferences'][$module]['ui'] ) ) {
 158+ $defaultPreferences[self::$modules['preferences'][$module]['key']] =
 159+ self::$modules['preferences'][$module]['ui'];
 160+ }
 161+ }
 162+ }
 163+
 164+ /**
 165+ * MakeGlobalVariablesScript hook
 166+ *
 167+ * Adds enabled/disabled switches for Vector modules
 168+ */
 169+ public static function makeGlobalVariablesScript( &$vars ) {
 170+ $configurations = array();
 171+ foreach ( $wgVectorModules as $module => $enable ) {
 172+ if (
 173+ isset( self::$modules[$module]['configurations'] ) &&
 174+ is_array( self::$modules[$module]['configurations'] )
 175+ ) {
 176+ foreach ( self::$modules[$module]['configurations'] as $configuration ) {
 177+ global $$configuration;
 178+ $configurations[$configuration] = $$configuration;
 179+ }
 180+ }
 181+ }
 182+ if ( count( $configurations ) ) {
 183+ $vars = array_merge( $vars, $configurations );
 184+ }
 185+ return true;
 186+ }
 187+
 188+ /*
 189+ * ResourceLoaderRegisterModules hook
 190+ *
 191+ * Adds modules to ResourceLoader
 192+ */
 193+ public static function resourceLoaderRegisterModules() {
 194+ ResourceLoader::register( );
 195+ }
 196+}
\ No newline at end of file
Property changes on: trunk/extensions/Vector/Vector.hooks.php
___________________________________________________________________
Added: svn:eol-style
1197 + native

Follow-up revisions

RevisionCommit summaryAuthorDate
r73208Follow-up r73030: Add Vector extension to Translatewikiraymond12:51, 17 September 2010

Comments

#Comment by Platonides (talk | contribs)   12:40, 15 September 2010

An extension with the same name as the skin is confusing. What about calling this BetterVector, VectorPlus, or something like that?

#Comment by Trevor Parscal (WMF) (talk | contribs)   18:35, 15 September 2010

Is it really? I think the context of it being in the extensions directory rather than the skins directory hints at the fact that is an extension to the Vector skin.

#Comment by MZMcBride (talk | contribs)   18:40, 15 September 2010

I agree that the naming is confusing. People come into #mediawiki all the time asking about how to enable the Vector skin on their site. I think telling them they have Vector already (but possibly not the separate extension of the same name that provides some sort of extra functionality) is needlessly confusing.

Personally, I don't understand why this is in an extension at all. If it's UI improvements, it should be in core, surely....

#Comment by Trevor Parscal (WMF) (talk | contribs)   18:42, 15 September 2010

There's no problem with moving them to core, and that's been suggested many times. At this point however, ResourceLoader and the new installer are quite enough new stuff for 1.17.

#Comment by MZMcBride (talk | contribs)   18:47, 15 September 2010

New extensions quickly gather a lot of users. If the eventual plan is to make this obsolete, it might be better to keep it where it was for now. It seems like a clearer/cleaner path forward to me, at least.

#Comment by Trevor Parscal (WMF) (talk | contribs)   18:48, 15 September 2010

Thank you for your input. I think we're flogging a dead horse here.

#Comment by 😂 (talk | contribs)   19:48, 15 September 2010

If the code is finalized and targeted for trunk anyway, there's no problem with going ahead and moving it to core...we don't feature freeze.

I'm with Max on thinking it should just go straight to core and skip being its own extension.

#Comment by 😂 (talk | contribs)   22:38, 20 September 2010

Setting to fixme per original complaint.

#Comment by Mr.Z-man (talk | contribs)   22:42, 20 September 2010

So basically, for 1.16 its part of the UsabilityIntiative extension, for 1.17 it will be its own extension, then it might be in core for 1.18? That sounds needlessly confusing to me. If we're not going to put it in core for 1.17 (which IMO we should), putting it into a different extension for a single release seems rather pointless.

#Comment by Trevor Parscal (WMF) (talk | contribs)   23:14, 20 September 2010

What is the actual process for deciding when an extension should be pushed into core, or a part of core should be pulled out as an extension? My guess is that there is none, but if there is one, please let's use it. Otherwise, maybe we should consider having one.

I think there is some confusion as to what I meant to do / was doing with this commit (and others like it). Please let me try and clarify.

Q: Why is Trevor moving these extensions into their own folders? A: UsabilityInitiative contained some shared code and a bunch of extensions, most of which used the shared code. Trevor regrets having structured things this way, and is moving these extensions into their own extension folders to finally fix this mistake.

Q: Why are some of these extensions dependent on 1.17? A: Dependence on the shared UsabilityInitiative code has now been removed in favor of ResourceLoader functionality, which is only present in 1.17. The old way of doing things was very costly for client-side performance, and the new way is much better.

Q: Why did the shared code ever exist in the first place? A: MediaWiki was not originally designed for the kind of front-end heavy feature that was being done by the usability team, and the shared code was a way to make this work a little easier and more performant. Now, considering what's running in head at the time of this post, MediaWiki is pretty darn good for front-end heavy features, so the shared code is no longer needed.

/Q&A

One of my reasons for not just pushing this stuff into core in the first place was that I wanted to be cautious about bloating core with this stuff without giving it allot of thought and community discussion. What I am getting here so far seems like open arms, embracing Vector and WikiEditor, welcoming them into core, which is awesome. I'm just wondering the words of MZ and demon, even as the important contributors that they are, are really all that's needed to start pushing stuff into core without risk of a revert by Tim when he has to get a release out.

In short - I'm very happy that my code is being welcomed into core by someone, but none of the people welcoming it into core are the ones who will be making a final say on the matter afaik. If I am wrong about this, please correct me - I think I would be happier to be wrong in this matter that right.

Should this discussion move to a mailing list? It seems to be growing in scope.

#Comment by MZMcBride (talk | contribs)   23:54, 20 September 2010

Regarding when to merge into core and when to keep a feature (or set of features) as part of an extension, I can only really point to List of extensions to be merged to the core. The general litmus test there seems to be made up of two questions: (1) "will it be useful to a majority of MediaWiki installations?" and (2) "is the code in an acceptable state?"

As far as I'm aware, WikiEditor is the successor to CharInsert. It seems fairly well-established that CharInsert (or its successor) should be in core (see in the comments from Brion and Simetrical in the link). WikiEditor undoubtedly improves the MediaWiki experience. Vector (the skin) is already in core; if the Vector extension provides clear improvements to the MediaWiki experience, it should be in core as well.

Regarding delays to releases and Tim's authority, I think it's safe to say that if the code is currently running on Wikimedia wikis, it's generally safe to be in the next MediaWiki release. That's the theory, at least. I understand your concern about moving the code around and possibly breaking things in the process, but I think, as Mr.Z-man said, the current path forward (in which this will be in a separate extension for only one MediaWiki release) isn't ideal.

#Comment by Mr.Z-man (talk | contribs)   23:57, 20 September 2010

It may have been a mistake to structure things as they are, but that's in the past. Changing things like this now just creates more of a hassle for 3rd party users with no clear benefit for anyone.

#Comment by Trevor Parscal (WMF) (talk | contribs)   00:02, 21 September 2010

Given that these extensions are jumping from 1.16 compatible (which is what those 3rd party users you are defending need) to 1.17 - it's more likely they will update and have everything explode on them with the extensions in the same place.

I've been trying to mitigate the hassle with lots of clear communication on the mailing list, but it's going to be a hassle one way or another.

In fact, this exact trap caused things to explode on the prototype servers - someone starts running svn up on the entire extension folder, and everything goes to hell. The primary reason I did not leave the WikiEditor/Vector extensions as they were in the old locations is to do with a conversation I had with the translatewiki folks, about having two extensions with the same messages keys, and the issues that would follow.

I'm just trying to find a balance here, perfection has waved bye bye long ago.

#Comment by Nikerabbit (talk | contribs)   08:48, 21 September 2010

Heck, even we were confused at the twn. We used @include for the migration time to prevent errors, but then we forgot to add those extensions to the live site and remove the @. And then I was wondering why my testwiki works and live site not, when they were using exactly same config...

#Comment by Trevor Parscal (WMF) (talk | contribs)   21:23, 12 October 2010

This should be a bug, not a code review comment, this revision isn't actually broken.

#Comment by MZMcBride (talk | contribs)   22:02, 12 October 2010

What should be a bug?

If this revision introduced a problem (like confusing naming), it should be marked as "fixme".

#Comment by Trevor Parscal (WMF) (talk | contribs)   22:05, 12 October 2010

A feature request bug, that says, this should be named something else. This code is not broken, so marking it as fixme is inappropriate. The change being suggested is not being agreed on so bugzilla is a better place to continue the conversation (it's the tool we use to track feature requests, bugs, etc.)

#Comment by MZMcBride (talk | contribs)   22:09, 12 October 2010

This specific revision is causing problems. It should be marked "fixme" until there's a subsequent revision (whether that's a revert or something else) that fixes this revision. It isn't a "feature request" to fix a revision that introduced problems.

#Comment by Platonides (talk | contribs)   23:14, 12 October 2010

Since naming it "Vector" is wrong, this is a problem with this revision.

We can revert it and continue the discussion as an enhacement bug, though.

#Comment by Guillom (talk | contribs)   02:44, 13 October 2010

Follow-up discussion on wikitech-l: http://thread.gmane.org/gmane.science.linguistics.wikipedia.technical/50223

Status & tagging log