User "Krinkle" changed the status of MediaWiki.r96309. Old Status: fixme New Status: new
User "Krinkle" also posted a comment on MediaWiki.r96309. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/96309#c22322 Commit summary: [ResourceLoader2] Gadget manager (start of ajax editor) * Add column to SpecialGadgetManager front-end for last modified timestamp. * Use getTitleMessage. Logic is now centralized (yay r95970). * Expose gadget internal name to JavaScript via data- attribute. * Expose array of all rights to JavaScript via OutputPage's getJSVars hook. * Only load ajax editor if user is allowed to edit NS_GADGET_DEFINITION. * Introduce three resource modules: ** ext.gadgetmanager.api: Group of (partially stub) function to interact with the API. Getting gadget metadata, list of categories, save gadget, delete gadget etc. ** ext.gadgetmanager.ui: Binds event handlers to the table outputted by PHP and takes care of the form/dialog generation and the autocompletion of all input fields through various apis and arrays. ** jquery.createPropcloud: jQuery plugin written from scratch for the gadget manager. Turns an input field into a properties "cloud" with autocompletion and visual bubbles to remove items. TODO: * ext.gadgetmanager.api ** Finish stub functions for saving and deleting a gadget. * ext.gadgetmanager.ui ** Nofication system ** Add "Create" tab to the special page, which just brings up a regular modify-form except that it will feature a title-input field. ** Add dynamic "(new category)" option in the category dropdown of the gadget editor. * SpecialGadgetManager.php ** generateGadgetView (Follows-up r95818) Comment: See follow-up r96759. Regarding the following: <pre> + // Prevent duplicate values + if ( o.props.indexOf( val ) === -1 ) { + $container.append( newPropHtml( val, o ) ); + o.onAdd( val ); + } </pre> <blockquote>''Doesn't this fail to prevent duplicate added values, because val isn't added to o.props ? The way it looks like to me at a quick glance is that if you've got o.props = ['foo','bar'] it won't let you add foo or bar, but it will let you add baz twice.''</blockquote> I agree it's not optimal. Right now the updating of <code>o.props</code> is expected to be handled by the callback (among other things). However I realize this is not handy, there was a reason for it though, however it turns out to be no longer valid. The reason for this behavior was because the options are (or could be) passed by value, instead of reference. Which means updating it here would not update the real object, hence we should let the callback handle this. However this rationale is no longer valid since the actual check is looking in the options' props, not some unknown 'real object'. The plugin is not receiving the props by value, it's maintaining the reference very well (it does call <code>$.extend</code> on <code>options</code>, but it doesn't do it recursively, it merely copies them over by key from one object to another, possibly overwriting a default but maintaining the reference) I changed plugin in the follow-up to do handle it inside the plugin. This got rid of a fair amount of code duplication as in most (if not all) cases all the callback did was updating the props object. Nice! <pre> +.mw-gadgetmanager-propcontainer {} [...] +.mw-gadgetmanager-prop-label {} </pre> <blockquote>''???''</blockquote> Still a work in progress. Already used in my local copy, accidentally left it in here while I was separating the local copy up into two parts so that I could do this commit first, before continuing. <pre> <legend>Module properties</legend>\</pre> <blockquote>''Hardcoded English. Isn't this jquery.localize-able?''</blockquote> The interface is still changing, so to avoid having to change 3 or 4 files all the time, left it hardcoded until a somewhat final decision is made. Next phase will take care of this. Thanks for the reminder (iirc there are some other untranslated parts as well) <pre> + $definitionTitle = Title::newFromText( $gadget->getName() . '.js', NS_GADGET_DEFINITION );</pre> Aha. Now that I re-read the documentation for <code>Title::newFromText</code> it makes sense. The second argument isn't "namespace" but "defaultNamespace", not obvious imho since it's a pretty generic function name. Anway, what I want here is <code> makeTitleSafe</code> indeed. Fixed. _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
