[MediaWiki-CodeReview] [MediaWiki r93351]: New comment added, and revision status changed
User "Krinkle" changed the status of MediaWiki.r93351. Old Status: fixme New Status: deferred User "Krinkle" also posted a comment on MediaWiki.r93351. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/93351#c21902 Commit summary: AjaxCategories rewrite: Solving syntax problems, performance improvements and applying code conventions: * Replaced sprite image with separate images and letting ResourceLoader embed them with @embed (@embed means 0 http requests, less maintenance, none of the known limitations with sprites, and more readable code (named files rather than pixel offsets) * Many functions were floating in the global namespace (like window.makeCaseInsensitive). A statement ends after a semi-colon(;). All functions declared after "catUrl" were assigned to the window object. I've instead turned the semi-colons back into comma's, merged some other var statements and moved them to the top of the closure. Changed local function declarations into function expressions for clarity. * fetchSuggestions is called by $.fn.suggestions like ".call( $textbox, $textbox.val() )". So the context (this) isn't the raw element but the jQuery object, no need to re-construct with "$(this)" or "$(that)" which is slow and shouldn't even work. jQuery methods can be called on it directly. I've also replaced "$(this).val()" with the value-argument passed to fetchSuggestions which has this exact value already. * Adding more function documentation. And changing @since to 1.19 as this was merged from js2-branch into 1.19-trunk and new features aren't backported to 1.18. * Optimizing options/default construction to just "options = $.extend( {}, options )". Caching defaultOptions is cool, but doesn't really work if it's in a context/instance local variable. Moved it up to the module closure var statements, now it's static across all instances. * In makeSuggestionBox(): Fixing invalid html fragments passed to jQuery that fail in IE. Shortcuts (like '' and '') are only allowed for createElement triggers, not when creating longer fragments with content and/or attributes which are created through innerHTML, in the latter case the HTML must be completely valid and is not auto-corrected by IE. * Using more jQuery chaining where possible. * In buildRegex(): Using $.map with join( '|' ), (rather than $.each with += '|'; and substr). * Storing the init instance of mw.ajaxCategories in mw.page for reference (rather than local/anonymous). * Applied some best practices and "write testable code" ** Moved some of the functions created on the fly and assigned to 'this' into prototype (reference is cheaper) ** Making sure at least all 'do', 'set' and/or 'prototype' functions have a return value. Even if it's just a simple boolean true or context/this for chain-ability. ** Rewrote confirmEdit( .., .., .., ) as a prototyped method named "doConfirmEdit" which takes a single props-object with named valuas as argument, instead of list with 8 arguments. * Removed trailing whitespace and other minor fixes to comply with the code conventions. ** Removed space between function name and caller: "foo ()" => foo()) ** Changing "someArray.indexOf() + 1" into "someArr.indexOf() !== -1". We want a Boolean here, not a Number. ** Renamed all underscore-variables to non-underscore variants. == Bug fixes == * When adding a category that is not already on the page as-is but of which the clean() version is already on the page, the script would fail. Fixed it by moving the checks up in handleCategoryAdd() and making sure that createCatLink() actually returned something. * confirmEdit() wasn't working properly and had unused code (such as submitButton), removed hidden prepending to #catlinks, no need to, it can be dialog'ed directly from the jQuery object without being somewhere in the document. * in doConfirmEdit() in submitFunction() and multiEdit: Clearing the input field after adding a category, so that when another category is being added it doesn't start with the previous value which is not allowed to be added again... Comment: Marking deferred, moved out of core in r96250. As shown above, there are too many issues. I have no time to fix them and I didn't create the module in the first place. Aside from bugs, there's also structural problems (like the lack of separation between api-logic and ui-events binding and callbacks, it's a bit of a soup right now) See also [[User:Krinkle/Extension review/InlineCategorizer]] for a summary. ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r93351]: New comment added
User "Krinkle" posted a comment on MediaWiki.r93351. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/93351#c21674 Commit summary: AjaxCategories rewrite: Solving syntax problems, performance improvements and applying code conventions: * Replaced sprite image with separate images and letting ResourceLoader embed them with @embed (@embed means 0 http requests, less maintenance, none of the known limitations with sprites, and more readable code (named files rather than pixel offsets) * Many functions were floating in the global namespace (like window.makeCaseInsensitive). A statement ends after a semi-colon(;). All functions declared after "catUrl" were assigned to the window object. I've instead turned the semi-colons back into comma's, merged some other var statements and moved them to the top of the closure. Changed local function declarations into function expressions for clarity. * fetchSuggestions is called by $.fn.suggestions like ".call( $textbox, $textbox.val() )". So the context (this) isn't the raw element but the jQuery object, no need to re-construct with "$(this)" or "$(that)" which is slow and shouldn't even work. jQuery methods can be called on it directly. I've also replaced "$(this).val()" with the value-argument passed to fetchSuggestions which has this exact value already. * Adding more function documentation. And changing @since to 1.19 as this was merged from js2-branch into 1.19-trunk and new features aren't backported to 1.18. * Optimizing options/default construction to just "options = $.extend( {}, options )". Caching defaultOptions is cool, but doesn't really work if it's in a context/instance local variable. Moved it up to the module closure var statements, now it's static across all instances. * In makeSuggestionBox(): Fixing invalid html fragments passed to jQuery that fail in IE. Shortcuts (like '' and '') are only allowed for createElement triggers, not when creating longer fragments with content and/or attributes which are created through innerHTML, in the latter case the HTML must be completely valid and is not auto-corrected by IE. * Using more jQuery chaining where possible. * In buildRegex(): Using $.map with join( '|' ), (rather than $.each with += '|'; and substr). * Storing the init instance of mw.ajaxCategories in mw.page for reference (rather than local/anonymous). * Applied some best practices and "write testable code" ** Moved some of the functions created on the fly and assigned to 'this' into prototype (reference is cheaper) ** Making sure at least all 'do', 'set' and/or 'prototype' functions have a return value. Even if it's just a simple boolean true or context/this for chain-ability. ** Rewrote confirmEdit( .., .., .., ) as a prototyped method named "doConfirmEdit" which takes a single props-object with named valuas as argument, instead of list with 8 arguments. * Removed trailing whitespace and other minor fixes to comply with the code conventions. ** Removed space between function name and caller: "foo ()" => foo()) ** Changing "someArray.indexOf() + 1" into "someArr.indexOf() !== -1". We want a Boolean here, not a Number. ** Renamed all underscore-variables to non-underscore variants. == Bug fixes == * When adding a category that is not already on the page as-is but of which the clean() version is already on the page, the script would fail. Fixed it by moving the checks up in handleCategoryAdd() and making sure that createCatLink() actually returned something. * confirmEdit() wasn't working properly and had unused code (such as submitButton), removed hidden prepending to #catlinks, no need to, it can be dialog'ed directly from the jQuery object without being somewhere in the document. * in doConfirmEdit() in submitFunction() and multiEdit: Clearing the input field after adding a category, so that when another category is being added it doesn't start with the previous value which is not allowed to be added again... Comment: Test cases: http://www.mediawiki.org/w/index.php?title=Sandbox&oldid=430268 Steps to test this module (todo: Automate this once we have a testing environment for it) # Copy [http://www.mediawiki.org/w/index.php?title=Sandbox&oldid=430268&action=raw wikitext] to your wiki's [[Sandbox]] -page # Try the following tasks After every action check the history/diff and refresh the page to see what it looks like now ## Add a new category "Ajax add" ## Remove category "Ajax add" ## Change "Quux", does it preserve sort key ? ## Remove "Bar" ## Remove "Foo", does it remove the right one ? ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r93351]: New comment added
User "Krinkle" posted a comment on MediaWiki.r93351. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/93351#c20669 Commit summary: AjaxCategories rewrite: Solving syntax problems, performance improvements and applying code conventions: * Replaced sprite image with separate images and letting ResourceLoader embed them with @embed (@embed means 0 http requests, less maintenance, none of the known limitations with sprites, and more readable code (named files rather than pixel offsets) * Many functions were floating in the global namespace (like window.makeCaseInsensitive). A statement ends after a semi-colon(;). All functions declared after "catUrl" were assigned to the window object. I've instead turned the semi-colons back into comma's, merged some other var statements and moved them to the top of the closure. Changed local function declarations into function expressions for clarity. * fetchSuggestions is called by $.fn.suggestions like ".call( $textbox, $textbox.val() )". So the context (this) isn't the raw element but the jQuery object, no need to re-construct with "$(this)" or "$(that)" which is slow and shouldn't even work. jQuery methods can be called on it directly. I've also replaced "$(this).val()" with the value-argument passed to fetchSuggestions which has this exact value already. * Adding more function documentation. And changing @since to 1.19 as this was merged from js2-branch into 1.19-trunk and new features aren't backported to 1.18. * Optimizing options/default construction to just "options = $.extend( {}, options )". Caching defaultOptions is cool, but doesn't really work if it's in a context/instance local variable. Moved it up to the module closure var statements, now it's static across all instances. * In makeSuggestionBox(): Fixing invalid html fragments passed to jQuery that fail in IE. Shortcuts (like '' and '') are only allowed for createElement triggers, not when creating longer fragments with content and/or attributes which are created through innerHTML, in the latter case the HTML must be completely valid and is not auto-corrected by IE. * Using more jQuery chaining where possible. * In buildRegex(): Using $.map with join( '|' ), (rather than $.each with += '|'; and substr). * Storing the init instance of mw.ajaxCategories in mw.page for reference (rather than local/anonymous). * Applied some best practices and "write testable code" ** Moved some of the functions created on the fly and assigned to 'this' into prototype (reference is cheaper) ** Making sure at least all 'do', 'set' and/or 'prototype' functions have a return value. Even if it's just a simple boolean true or context/this for chain-ability. ** Rewrote confirmEdit( .., .., .., ) as a prototyped method named "doConfirmEdit" which takes a single props-object with named valuas as argument, instead of list with 8 arguments. * Removed trailing whitespace and other minor fixes to comply with the code conventions. ** Removed space between function name and caller: "foo ()" => foo()) ** Changing "someArray.indexOf() + 1" into "someArr.indexOf() !== -1". We want a Boolean here, not a Number. ** Renamed all underscore-variables to non-underscore variants. == Bug fixes == * When adding a category that is not already on the page as-is but of which the clean() version is already on the page, the script would fail. Fixed it by moving the checks up in handleCategoryAdd() and making sure that createCatLink() actually returned something. * confirmEdit() wasn't working properly and had unused code (such as submitButton), removed hidden prepending to #catlinks, no need to, it can be dialog'ed directly from the jQuery object without being somewhere in the document. * in doConfirmEdit() in submitFunction() and multiEdit: Clearing the input field after adding a category, so that when another category is being added it doesn't start with the previous value which is not allowed to be added again... Comment: The categoryRegex in the matchLineBreak condition remained unchanged since I'm terrible at regexes and don't know a lot about how the Parser handles category links, – I left the regex the way it was introduced by mdale. I understand the regex to recognize the issue, but don't know how to fix it. Same goes for newText = oldText.replace( categoryRegex, newCategoryString );. I didn't change the event handler a lot because (together with the culture of triggers events to perform actions and using DOM-inspection to get information), I'd like to get rid of that all together and instead store it in JavaScript (perhaps wgCategories or a local copy of it). That would also make containCat() a lot nicer. Events would then just call the API rather than the events ''being'' the API. This will likely require another major refactor of this module, which I don't have time for right now but would love to do later (I don't think it's a requirement for the module though,
[MediaWiki-CodeReview] [MediaWiki r93351]: New comment added, and revision status changed
User "Catrope" changed the status of MediaWiki.r93351. Old Status: new New Status: fixme User "Catrope" also posted a comment on MediaWiki.r93351. Full URL: https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/93351#c20663 Commit summary: AjaxCategories rewrite: Solving syntax problems, performance improvements and applying code conventions: * Replaced sprite image with separate images and letting ResourceLoader embed them with @embed (@embed means 0 http requests, less maintenance, none of the known limitations with sprites, and more readable code (named files rather than pixel offsets) * Many functions were floating in the global namespace (like window.makeCaseInsensitive). A statement ends after a semi-colon(;). All functions declared after "catUrl" were assigned to the window object. I've instead turned the semi-colons back into comma's, merged some other var statements and moved them to the top of the closure. Changed local function declarations into function expressions for clarity. * fetchSuggestions is called by $.fn.suggestions like ".call( $textbox, $textbox.val() )". So the context (this) isn't the raw element but the jQuery object, no need to re-construct with "$(this)" or "$(that)" which is slow and shouldn't even work. jQuery methods can be called on it directly. I've also replaced "$(this).val()" with the value-argument passed to fetchSuggestions which has this exact value already. * Adding more function documentation. And changing @since to 1.19 as this was merged from js2-branch into 1.19-trunk and new features aren't backported to 1.18. * Optimizing options/default construction to just "options = $.extend( {}, options )". Caching defaultOptions is cool, but doesn't really work if it's in a context/instance local variable. Moved it up to the module closure var statements, now it's static across all instances. * In makeSuggestionBox(): Fixing invalid html fragments passed to jQuery that fail in IE. Shortcuts (like '' and '') are only allowed for createElement triggers, not when creating longer fragments with content and/or attributes which are created through innerHTML, in the latter case the HTML must be completely valid and is not auto-corrected by IE. * Using more jQuery chaining where possible. * In buildRegex(): Using $.map with join( '|' ), (rather than $.each with += '|'; and substr). * Storing the init instance of mw.ajaxCategories in mw.page for reference (rather than local/anonymous). * Applied some best practices and "write testable code" ** Moved some of the functions created on the fly and assigned to 'this' into prototype (reference is cheaper) ** Making sure at least all 'do', 'set' and/or 'prototype' functions have a return value. Even if it's just a simple boolean true or context/this for chain-ability. ** Rewrote confirmEdit( .., .., .., ) as a prototyped method named "doConfirmEdit" which takes a single props-object with named valuas as argument, instead of list with 8 arguments. * Removed trailing whitespace and other minor fixes to comply with the code conventions. ** Removed space between function name and caller: "foo ()" => foo()) ** Changing "someArray.indexOf() + 1" into "someArr.indexOf() !== -1". We want a Boolean here, not a Number. ** Renamed all underscore-variables to non-underscore variants. == Bug fixes == * When adding a category that is not already on the page as-is but of which the clean() version is already on the page, the script would fail. Fixed it by moving the checks up in handleCategoryAdd() and making sure that createCatLink() actually returned something. * confirmEdit() wasn't working properly and had unused code (such as submitButton), removed hidden prepending to #catlinks, no need to, it can be dialog'ed directly from the jQuery object without being somewhere in the document. * in doConfirmEdit() in submitFunction() and multiEdit: Clearing the input field after adding a category, so that when another category is being added it doesn't start with the previous value which is not allowed to be added again... Comment: if ( matchLineBreak ) { categoryRegex += '[ \\t\\r]*\\n?'; So this could make the regex potentially match a bunch of spaces after the link, but not a line break? That's confusing. Document this if it's intended, or fix it if it's not. This one wasn't addressed. /** -* Execute or queue an category add +* Execute or queue an category add. +* @param $link {jQuery} +* @param category +* @param noAppend +* @param exists +* @return {mw.ajaxCategories} The behavior of this function is barely documented, specifically what it does with $link. In fact, a lot of the internal functions are undocumented, and that's only OK if they're doing trivial things. This was addressed somewhat, but the magic that happens when $link is an empty jQuery obje
[MediaWiki-CodeReview] [MediaWiki r93351]: New comment added
User "Catrope" posted a comment on MediaWiki.r93351. Full URL: https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/93351#c20662 Commit summary: AjaxCategories rewrite: Solving syntax problems, performance improvements and applying code conventions: * Replaced sprite image with separate images and letting ResourceLoader embed them with @embed (@embed means 0 http requests, less maintenance, none of the known limitations with sprites, and more readable code (named files rather than pixel offsets) * Many functions were floating in the global namespace (like window.makeCaseInsensitive). A statement ends after a semi-colon(;). All functions declared after "catUrl" were assigned to the window object. I've instead turned the semi-colons back into comma's, merged some other var statements and moved them to the top of the closure. Changed local function declarations into function expressions for clarity. * fetchSuggestions is called by $.fn.suggestions like ".call( $textbox, $textbox.val() )". So the context (this) isn't the raw element but the jQuery object, no need to re-construct with "$(this)" or "$(that)" which is slow and shouldn't even work. jQuery methods can be called on it directly. I've also replaced "$(this).val()" with the value-argument passed to fetchSuggestions which has this exact value already. * Adding more function documentation. And changing @since to 1.19 as this was merged from js2-branch into 1.19-trunk and new features aren't backported to 1.18. * Optimizing options/default construction to just "options = $.extend( {}, options )". Caching defaultOptions is cool, but doesn't really work if it's in a context/instance local variable. Moved it up to the module closure var statements, now it's static across all instances. * In makeSuggestionBox(): Fixing invalid html fragments passed to jQuery that fail in IE. Shortcuts (like '' and '') are only allowed for createElement triggers, not when creating longer fragments with content and/or attributes which are created through innerHTML, in the latter case the HTML must be completely valid and is not auto-corrected by IE. * Using more jQuery chaining where possible. * In buildRegex(): Using $.map with join( '|' ), (rather than $.each with += '|'; and substr). * Storing the init instance of mw.ajaxCategories in mw.page for reference (rather than local/anonymous). * Applied some best practices and "write testable code" ** Moved some of the functions created on the fly and assigned to 'this' into prototype (reference is cheaper) ** Making sure at least all 'do', 'set' and/or 'prototype' functions have a return value. Even if it's just a simple boolean true or context/this for chain-ability. ** Rewrote confirmEdit( .., .., .., ) as a prototyped method named "doConfirmEdit" which takes a single props-object with named valuas as argument, instead of list with 8 arguments. * Removed trailing whitespace and other minor fixes to comply with the code conventions. ** Removed space between function name and caller: "foo ()" => foo()) ** Changing "someArray.indexOf() + 1" into "someArr.indexOf() !== -1". We want a Boolean here, not a Number. ** Renamed all underscore-variables to non-underscore variants. == Bug fixes == * When adding a category that is not already on the page as-is but of which the clean() version is already on the page, the script would fail. Fixed it by moving the checks up in handleCategoryAdd() and making sure that createCatLink() actually returned something. * confirmEdit() wasn't working properly and had unused code (such as submitButton), removed hidden prepending to #catlinks, no need to, it can be dialog'ed directly from the jQuery object without being somewhere in the document. * in doConfirmEdit() in submitFunction() and multiEdit: Clearing the input field after adding a category, so that when another category is being added it doesn't start with the previous value which is not allowed to be added again... Comment: if ( matchLineBreak ) { categoryRegex += '[ \\t\\r]*\\n?'; So this could make the regex potentially match a bunch of spaces after the link, but not a line break? That's confusing. Document this if it's intended, or fix it if it's not. ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r93351]: New comment added
User "Krinkle" posted a comment on MediaWiki.r93351. Full URL: https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/93351#c20636 Commit summary: AjaxCategories rewrite: Solving syntax problems, performance improvements and applying code conventions: * Replaced sprite image with separate images and letting ResourceLoader embed them with @embed (@embed means 0 http requests, less maintenance, none of the known limitations with sprites, and more readable code (named files rather than pixel offsets) * Many functions were floating in the global namespace (like window.makeCaseInsensitive). A statement ends after a semi-colon(;). All functions declared after "catUrl" were assigned to the window object. I've instead turned the semi-colons back into comma's, merged some other var statements and moved them to the top of the closure. Changed local function declarations into function expressions for clarity. * fetchSuggestions is called by $.fn.suggestions like ".call( $textbox, $textbox.val() )". So the context (this) isn't the raw element but the jQuery object, no need to re-construct with "$(this)" or "$(that)" which is slow and shouldn't even work. jQuery methods can be called on it directly. I've also replaced "$(this).val()" with the value-argument passed to fetchSuggestions which has this exact value already. * Adding more function documentation. And changing @since to 1.19 as this was merged from js2-branch into 1.19-trunk and new features aren't backported to 1.18. * Optimizing options/default construction to just "options = $.extend( {}, options )". Caching defaultOptions is cool, but doesn't really work if it's in a context/instance local variable. Moved it up to the module closure var statements, now it's static across all instances. * In makeSuggestionBox(): Fixing invalid html fragments passed to jQuery that fail in IE. Shortcuts (like '' and '') are only allowed for createElement triggers, not when creating longer fragments with content and/or attributes which are created through innerHTML, in the latter case the HTML must be completely valid and is not auto-corrected by IE. * Using more jQuery chaining where possible. * In buildRegex(): Using $.map with join( '|' ), (rather than $.each with += '|'; and substr). * Storing the init instance of mw.ajaxCategories in mw.page for reference (rather than local/anonymous). * Applied some best practices and "write testable code" ** Moved some of the functions created on the fly and assigned to 'this' into prototype (reference is cheaper) ** Making sure at least all 'do', 'set' and/or 'prototype' functions have a return value. Even if it's just a simple boolean true or context/this for chain-ability. ** Rewrote confirmEdit( .., .., .., ) as a prototyped method named "doConfirmEdit" which takes a single props-object with named valuas as argument, instead of list with 8 arguments. * Removed trailing whitespace and other minor fixes to comply with the code conventions. ** Removed space between function name and caller: "foo ()" => foo()) ** Changing "someArray.indexOf() + 1" into "someArr.indexOf() !== -1". We want a Boolean here, not a Number. ** Renamed all underscore-variables to non-underscore variants. == Bug fixes == * When adding a category that is not already on the page as-is but of which the clean() version is already on the page, the script would fail. Fixed it by moving the checks up in handleCategoryAdd() and making sure that createCatLink() actually returned something. * confirmEdit() wasn't working properly and had unused code (such as submitButton), removed hidden prepending to #catlinks, no need to, it can be dialog'ed directly from the jQuery object without being somewhere in the document. * in doConfirmEdit() in submitFunction() and multiEdit: Clearing the input field after adding a category, so that when another category is being added it doesn't start with the previous value which is not allowed to be added again... Comment: ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r93351]: New comment added, and revision status changed
User "Krinkle" changed the status of MediaWiki.r93351. Old Status: fixme New Status: new User "Krinkle" also posted a comment on MediaWiki.r93351. Full URL: https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/93351#c20635 Commit summary: AjaxCategories rewrite: Solving syntax problems, performance improvements and applying code conventions: * Replaced sprite image with separate images and letting ResourceLoader embed them with @embed (@embed means 0 http requests, less maintenance, none of the known limitations with sprites, and more readable code (named files rather than pixel offsets) * Many functions were floating in the global namespace (like window.makeCaseInsensitive). A statement ends after a semi-colon(;). All functions declared after "catUrl" were assigned to the window object. I've instead turned the semi-colons back into comma's, merged some other var statements and moved them to the top of the closure. Changed local function declarations into function expressions for clarity. * fetchSuggestions is called by $.fn.suggestions like ".call( $textbox, $textbox.val() )". So the context (this) isn't the raw element but the jQuery object, no need to re-construct with "$(this)" or "$(that)" which is slow and shouldn't even work. jQuery methods can be called on it directly. I've also replaced "$(this).val()" with the value-argument passed to fetchSuggestions which has this exact value already. * Adding more function documentation. And changing @since to 1.19 as this was merged from js2-branch into 1.19-trunk and new features aren't backported to 1.18. * Optimizing options/default construction to just "options = $.extend( {}, options )". Caching defaultOptions is cool, but doesn't really work if it's in a context/instance local variable. Moved it up to the module closure var statements, now it's static across all instances. * In makeSuggestionBox(): Fixing invalid html fragments passed to jQuery that fail in IE. Shortcuts (like '' and '') are only allowed for createElement triggers, not when creating longer fragments with content and/or attributes which are created through innerHTML, in the latter case the HTML must be completely valid and is not auto-corrected by IE. * Using more jQuery chaining where possible. * In buildRegex(): Using $.map with join( '|' ), (rather than $.each with += '|'; and substr). * Storing the init instance of mw.ajaxCategories in mw.page for reference (rather than local/anonymous). * Applied some best practices and "write testable code" ** Moved some of the functions created on the fly and assigned to 'this' into prototype (reference is cheaper) ** Making sure at least all 'do', 'set' and/or 'prototype' functions have a return value. Even if it's just a simple boolean true or context/this for chain-ability. ** Rewrote confirmEdit( .., .., .., ) as a prototyped method named "doConfirmEdit" which takes a single props-object with named valuas as argument, instead of list with 8 arguments. * Removed trailing whitespace and other minor fixes to comply with the code conventions. ** Removed space between function name and caller: "foo ()" => foo()) ** Changing "someArray.indexOf() + 1" into "someArr.indexOf() !== -1". We want a Boolean here, not a Number. ** Renamed all underscore-variables to non-underscore variants. == Bug fixes == * When adding a category that is not already on the page as-is but of which the clean() version is already on the page, the script would fail. Fixed it by moving the checks up in handleCategoryAdd() and making sure that createCatLink() actually returned something. * confirmEdit() wasn't working properly and had unused code (such as submitButton), removed hidden prepending to #catlinks, no need to, it can be dialog'ed directly from the jQuery object without being somewhere in the document. * in doConfirmEdit() in submitFunction() and multiEdit: Clearing the input field after adding a category, so that when another category is being added it doesn't start with the previous value which is not allowed to be added again... Comment: I've believe all issues are fixed with r94268. Here's the points that I've skipped for now that I'd like to give some more thought / discussion: * 'Unexpected error occurred. $link undefined' error message should not be hardcoded in English. * Calling methods directly instead of triggering .click() on buttons. Isn't this a big faux pas in your own JSPERF guide? Yes, it sure is. I didn't introduce it though (It was originally .catlinks span a#catlinks li a in r92112), but I should've catched it in the rewrite! Fixed now in r94268. ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r93351]: New comment added
User "Catrope" posted a comment on MediaWiki.r93351. Full URL: https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/93351#c20628 Commit summary: AjaxCategories rewrite: Solving syntax problems, performance improvements and applying code conventions: * Replaced sprite image with separate images and letting ResourceLoader embed them with @embed (@embed means 0 http requests, less maintenance, none of the known limitations with sprites, and more readable code (named files rather than pixel offsets) * Many functions were floating in the global namespace (like window.makeCaseInsensitive). A statement ends after a semi-colon(;). All functions declared after "catUrl" were assigned to the window object. I've instead turned the semi-colons back into comma's, merged some other var statements and moved them to the top of the closure. Changed local function declarations into function expressions for clarity. * fetchSuggestions is called by $.fn.suggestions like ".call( $textbox, $textbox.val() )". So the context (this) isn't the raw element but the jQuery object, no need to re-construct with "$(this)" or "$(that)" which is slow and shouldn't even work. jQuery methods can be called on it directly. I've also replaced "$(this).val()" with the value-argument passed to fetchSuggestions which has this exact value already. * Adding more function documentation. And changing @since to 1.19 as this was merged from js2-branch into 1.19-trunk and new features aren't backported to 1.18. * Optimizing options/default construction to just "options = $.extend( {}, options )". Caching defaultOptions is cool, but doesn't really work if it's in a context/instance local variable. Moved it up to the module closure var statements, now it's static across all instances. * In makeSuggestionBox(): Fixing invalid html fragments passed to jQuery that fail in IE. Shortcuts (like '' and '') are only allowed for createElement triggers, not when creating longer fragments with content and/or attributes which are created through innerHTML, in the latter case the HTML must be completely valid and is not auto-corrected by IE. * Using more jQuery chaining where possible. * In buildRegex(): Using $.map with join( '|' ), (rather than $.each with += '|'; and substr). * Storing the init instance of mw.ajaxCategories in mw.page for reference (rather than local/anonymous). * Applied some best practices and "write testable code" ** Moved some of the functions created on the fly and assigned to 'this' into prototype (reference is cheaper) ** Making sure at least all 'do', 'set' and/or 'prototype' functions have a return value. Even if it's just a simple boolean true or context/this for chain-ability. ** Rewrote confirmEdit( .., .., .., ) as a prototyped method named "doConfirmEdit" which takes a single props-object with named valuas as argument, instead of list with 8 arguments. * Removed trailing whitespace and other minor fixes to comply with the code conventions. ** Removed space between function name and caller: "foo ()" => foo()) ** Changing "someArray.indexOf() + 1" into "someArr.indexOf() !== -1". We want a Boolean here, not a Number. ** Renamed all underscore-variables to non-underscore variants. == Bug fixes == * When adding a category that is not already on the page as-is but of which the clean() version is already on the page, the script would fail. Fixed it by moving the checks up in handleCategoryAdd() and making sure that createCatLink() actually returned something. * confirmEdit() wasn't working properly and had unused code (such as submitButton), removed hidden prepending to #catlinks, no need to, it can be dialog'ed directly from the jQuery object without being somewhere in the document. * in doConfirmEdit() in submitFunction() and multiEdit: Clearing the input field after adding a category, so that when another category is being added it doesn't start with the previous value which is not allowed to be added again... Comment: + category = new mw.Title( redirect[0].to )._name; Are you supposed to be accessing private members of mw.Title like that? /** -* Execute or queue an category add +* Execute or queue an category add. +* @param $link {jQuery} +* @param category +* @param noAppend +* @param exists +* @return {mw.ajaxCategories} The behavior of this function is barely documented, specifically what it does with $link. In fact, a lot of the internal functions are undocumented, and that's only OK if they're doing trivial things. + this.showError( 'Unexpected error occurred. $link undefined.' ); Hardcoded English. + $( this ) + .unbind( 'click' ) + .click( that.handleDeleteLink ) Could you document what's going on in the clic
[MediaWiki-CodeReview] [MediaWiki r93351]: New comment added
User "Catrope" posted a comment on MediaWiki.r93351. Full URL: https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/93351#c20617 Commit summary: AjaxCategories rewrite: Solving syntax problems, performance improvements and applying code conventions: * Replaced sprite image with separate images and letting ResourceLoader embed them with @embed (@embed means 0 http requests, less maintenance, none of the known limitations with sprites, and more readable code (named files rather than pixel offsets) * Many functions were floating in the global namespace (like window.makeCaseInsensitive). A statement ends after a semi-colon(;). All functions declared after "catUrl" were assigned to the window object. I've instead turned the semi-colons back into comma's, merged some other var statements and moved them to the top of the closure. Changed local function declarations into function expressions for clarity. * fetchSuggestions is called by $.fn.suggestions like ".call( $textbox, $textbox.val() )". So the context (this) isn't the raw element but the jQuery object, no need to re-construct with "$(this)" or "$(that)" which is slow and shouldn't even work. jQuery methods can be called on it directly. I've also replaced "$(this).val()" with the value-argument passed to fetchSuggestions which has this exact value already. * Adding more function documentation. And changing @since to 1.19 as this was merged from js2-branch into 1.19-trunk and new features aren't backported to 1.18. * Optimizing options/default construction to just "options = $.extend( {}, options )". Caching defaultOptions is cool, but doesn't really work if it's in a context/instance local variable. Moved it up to the module closure var statements, now it's static across all instances. * In makeSuggestionBox(): Fixing invalid html fragments passed to jQuery that fail in IE. Shortcuts (like '' and '') are only allowed for createElement triggers, not when creating longer fragments with content and/or attributes which are created through innerHTML, in the latter case the HTML must be completely valid and is not auto-corrected by IE. * Using more jQuery chaining where possible. * In buildRegex(): Using $.map with join( '|' ), (rather than $.each with += '|'; and substr). * Storing the init instance of mw.ajaxCategories in mw.page for reference (rather than local/anonymous). * Applied some best practices and "write testable code" ** Moved some of the functions created on the fly and assigned to 'this' into prototype (reference is cheaper) ** Making sure at least all 'do', 'set' and/or 'prototype' functions have a return value. Even if it's just a simple boolean true or context/this for chain-ability. ** Rewrote confirmEdit( .., .., .., ) as a prototyped method named "doConfirmEdit" which takes a single props-object with named valuas as argument, instead of list with 8 arguments. * Removed trailing whitespace and other minor fixes to comply with the code conventions. ** Removed space between function name and caller: "foo ()" => foo()) ** Changing "someArray.indexOf() + 1" into "someArr.indexOf() !== -1". We want a Boolean here, not a Number. ** Renamed all underscore-variables to non-underscore variants. == Bug fixes == * When adding a category that is not already on the page as-is but of which the clean() version is already on the page, the script would fail. Fixed it by moving the checks up in handleCategoryAdd() and making sure that createCatLink() actually returned something. * confirmEdit() wasn't working properly and had unused code (such as submitButton), removed hidden prepending to #catlinks, no need to, it can be dialog'ed directly from the jQuery object without being somewhere in the document. * in doConfirmEdit() in submitFunction() and multiEdit: Clearing the input field after adding a category, so that when another category is being added it doesn't start with the previous value which is not allowed to be added again... Comment: Reviewing the entire file, adding comments here in lieu. if ( s !== undefined ) { Since s.replace() can fail even on things that aren't undefined, shouldn't you really be checking for typeof s == 'string' ? + title = page.title.split( ':', 2 )[1]; Eww. This is kind of hackish and works only because all the returned titles are in the category namespace. Would the overhead of using mw.Title be bad? +* @param id Undocumented, as is pretty much everything else in that function BTW. text = text.replace( matches[i], id + i + '-' ); This will do fun unexpected things if id happens to be a Number, you may want to force it to a string. Also, why is there a dash at the end? This whole function doesn't make much sense to me. * Makes regex string caseinsensitive. Sounds nice and generic. But then it does this: + if ( $.inArray(
[MediaWiki-CodeReview] [MediaWiki r93351]: New comment added
User "Krinkle" posted a comment on MediaWiki.r93351. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/93351#c20292 Commit summary: AjaxCategories rewrite: Solving syntax problems, performance improvements and applying code conventions: * Replaced sprite image with separate images and letting ResourceLoader embed them with @embed (@embed means 0 http requests, less maintenance, none of the known limitations with sprites, and more readable code (named files rather than pixel offsets) * Many functions were floating in the global namespace (like window.makeCaseInsensitive). A statement ends after a semi-colon(;). All functions declared after "catUrl" were assigned to the window object. I've instead turned the semi-colons back into comma's, merged some other var statements and moved them to the top of the closure. Changed local function declarations into function expressions for clarity. * fetchSuggestions is called by $.fn.suggestions like ".call( $textbox, $textbox.val() )". So the context (this) isn't the raw element but the jQuery object, no need to re-construct with "$(this)" or "$(that)" which is slow and shouldn't even work. jQuery methods can be called on it directly. I've also replaced "$(this).val()" with the value-argument passed to fetchSuggestions which has this exact value already. * Adding more function documentation. And changing @since to 1.19 as this was merged from js2-branch into 1.19-trunk and new features aren't backported to 1.18. * Optimizing options/default construction to just "options = $.extend( {}, options )". Caching defaultOptions is cool, but doesn't really work if it's in a context/instance local variable. Moved it up to the module closure var statements, now it's static across all instances. * In makeSuggestionBox(): Fixing invalid html fragments passed to jQuery that fail in IE. Shortcuts (like '' and '') are only allowed for createElement triggers, not when creating longer fragments with content and/or attributes which are created through innerHTML, in the latter case the HTML must be completely valid and is not auto-corrected by IE. * Using more jQuery chaining where possible. * In buildRegex(): Using $.map with join( '|' ), (rather than $.each with += '|'; and substr). * Storing the init instance of mw.ajaxCategories in mw.page for reference (rather than local/anonymous). * Applied some best practices and "write testable code" ** Moved some of the functions created on the fly and assigned to 'this' into prototype (reference is cheaper) ** Making sure at least all 'do', 'set' and/or 'prototype' functions have a return value. Even if it's just a simple boolean true or context/this for chain-ability. ** Rewrote confirmEdit( .., .., .., ) as a prototyped method named "doConfirmEdit" which takes a single props-object with named valuas as argument, instead of list with 8 arguments. * Removed trailing whitespace and other minor fixes to comply with the code conventions. ** Removed space between function name and caller: "foo ()" => foo()) ** Changing "someArray.indexOf() + 1" into "someArr.indexOf() !== -1". We want a Boolean here, not a Number. ** Renamed all underscore-variables to non-underscore variants. == Bug fixes == * When adding a category that is not already on the page as-is but of which the clean() version is already on the page, the script would fail. Fixed it by moving the checks up in handleCategoryAdd() and making sure that createCatLink() actually returned something. * confirmEdit() wasn't working properly and had unused code (such as submitButton), removed hidden prepending to #catlinks, no need to, it can be dialog'ed directly from the jQuery object without being somewhere in the document. * in doConfirmEdit() in submitFunction() and multiEdit: Clearing the input field after adding a category, so that when another category is being added it doesn't start with the previous value which is not allowed to be added again... Comment: Since many functions have been moved out of the constructor (for performance and readability) into either the prototype reference on the bottom or into a local function experession, the diff is a bit hard to review. I suggest reviewing this as a file rather than a diff. ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview