Last modified: 2008-09-01 18:48:43 UTC
Created attachment 5233 [details] Diff with the current version I've proposed several enhancements to "wikibits.js" on Wikipedia's Village Pump (techincal). The changes are described in detail, here: http://en.wikipedia.org/wiki/User:Alex_Smotrov/mw/sortable They're also discussed on a Talk page, here: http://en.wikipedia.org/wiki/User_talk:SharkD/Sandbox/wikibits_4 The source code of the edited file itself can be found here: http://en.wikipedia.org/wiki/User:SharkD/Sandbox/wikibits_4 A diff with the current version is attached to this message. The changes include: 1. Add a table header class to allow columns to be sorted in reverse by default. 2. Expand the ALT text for the clickable icons so that it is more verbose. 3. Change the date sorting routine so that it detects more date formats. 4. Change the clickable icon so that it always remains in a fixed position and doesn't break onto new lines. 5. Search for the yen currency when sorting tables. 6. Various minor file size, readability and performance optimizations. 7. Disable code that isn't ever used. 7a. Disable special handling of tables with THEAD elements since the THEAD element is not supported by Wikimedia. 7b. Disable alternate row coloring. A few additional proposals were made, but do not exist in the version of the file I have provided. -Mike
Please provide a unified diff against SVN, see http://www.mediawiki.org/wiki/Subversion#Making_a_diff Thanks.
Created attachment 5234 [details] Diff with svn diff Please use a unified diff, diff -u. The output of the "svn diff" command is even better, because it gives the revision you're diffing against. I've attached such a patch. Will review in a moment.
The patch is not reviewable. You've changed practically every line of the file by removing the trailing semicolons. Even if this were desirable, which I don't think it is, it makes it impossible for me to figure out what substantive changes you've made. As I said to SharkD about this, please file one bug per change. Do not try to attach a single patch that changes every single line of the file, and *especially* do not mix extensive stylistic changes with substantive ones. You can CC me on any further bugs you open, and I'll review patches (preferably formatted with "diff -u" or "svn diff") that address one single issue each. This bug addresses too many things, so it can't be given any meaningful resolution other than INVALID.
(In reply to comment #1) > Please provide a unified diff against SVN, see > http://www.mediawiki.org/wiki/Subversion#Making_a_diff > Thanks. Yes, I did that. It's attached to the message.
Your diff is not unified (notice > and < instead of + and -) and it's not made using SVN. That's not a huge deal, though -- although please do at least use the -u option when making your diffs -- I just applied it to my own MW copy and made the unified diff myself. It can't be reviewed because it changes too much at once, that's the problem.
I have succesfully opened both files in TortoiseMerge, but can't find an option to save a unified diff. How is this accomplished? Also, how do I check out SVN as an anon without write priveledges? I keep getting an '403 Forbidden' error when trying to import from 'http://svn.wikimedia.org/svnroot/mediawiki/trunk/phase3/skins/common'. Thanks!
(In reply to comment #5) > Your diff is not unified (notice > and < instead of + and -) and it's not made > using SVN. That's not a huge deal, though -- although please do at least use > the -u option when making your diffs -- I just applied it to my own MW copy and > made the unified diff myself. It can't be reviewed because it changes too much > at once, that's the problem. The individual changes are described in detail, here: http://en.wikipedia.org/wiki/User:Alex_Smotrov/mw/sortable Is that not enough?
(In reply to comment #6) > I have succesfully opened both files in TortoiseMerge, but can't find an option > to save a unified diff. How is this accomplished? You don't use TortoiseMerge to create diffs to svn trunk. You check out the file you want to change, change it, and choose "Create Patch..." from the directory context menu: http://tortoisesvn.net/docs/release/TortoiseSVN_en/tsvn-dug-patch.html > Also, how do I check out SVN > as an anon without write priveledges? I keep getting an '403 Forbidden' error > when trying to import from > 'http://svn.wikimedia.org/svnroot/mediawiki/trunk/phase3/skins/common'. It works for me. What series of commands (or GUI options) are you using to check it out? (In reply to comment #7) > The individual changes are described in detail, here: > > http://en.wikipedia.org/wiki/User:Alex_Smotrov/mw/sortable > > Is that not enough? No, for a couple of reasons: 1) If some of the changes are accepted but not others (which is likely), we need to be able to commit only the ones that are accepted. If it's only one patch, we can't do that without trying to figure out which lines do what. This patch is over 1300 lines long, it would be faster for us to reimplement the changes ourselves than to try to figure out what parts to use. 2) We have to be able to review the implementation as well as the intended effect. You might have made an error that we'd catch, or there might be some other issue with how you do it that we want to see fixed before we take the patch. So we have to figure out what each changed line in the patch is supposed to do, and to do that we need to be told what *you* think they do. If you give us a long list of things that are changed and then a long patch that changes all of them, it won't be clear what each change to the file is supposed to be doing from your summary. So it makes it harder to figure out what you're doing, and harder to say that what you're doing is good.
(In reply to comment #8) > (In reply to comment #6) > > Also, how do I check out SVN > > as an anon without write priveledges? I keep getting an '403 Forbidden' error > > when trying to import from > > 'http://svn.wikimedia.org/svnroot/mediawiki/trunk/phase3/skins/common'. > It works for me. What series of commands (or GUI options) are you using to > check it out? I chose the 'import' option instead of 'checkout'. All the files seem to have downloaded properly now. -Mike
In order to create (and store) a separate file for each proposal, do I need to check out the SVN version multiple times in separate folders?
You can create one folder, make the first change, create patch, revert to trunk version (this should be an option in the context menus somewhere), make the second change, create patch, revert, etc.
(In reply to comment #11) > You can create one folder, make the first change, create patch, revert to trunk > version (this should be an option in the context menus somewhere), make the > second change, create patch, revert, etc. I'd rather store the changes in case I make a mistake or something. I've gone through 135 revisions to reach the file's current state, so I'm rather error-prone. Also, this site keeps logging me out. Is there a way to stay logged in?
(In reply to comment #12) > I'd rather store the changes in case I make a mistake or something. I've gone > through 135 revisions to reach the file's current state, so I'm rather > error-prone. You can save all the patches. You have to, if you want to submit them. > Also, this site keeps logging me out. Is there a way to stay logged in? Try not restricting your session to your current IP address.
(In reply to comment #13) > (In reply to comment #12) > > I'd rather store the changes in case I make a mistake or something. I've gone > > through 135 revisions to reach the file's current state, so I'm rather > > error-prone. > You can save all the patches. You have to, if you want to submit them. Good point. > > Also, this site keeps logging me out. Is there a way to stay logged in? > Try not restricting your session to your current IP address. Thanks, I'll do that.
(In reply to comment #8) > (In reply to comment #6) > > I have succesfully opened both files in TortoiseMerge, but can't find an option > > to save a unified diff. How is this accomplished? > You don't use TortoiseMerge to create diffs to svn trunk. You check out the > file you want to change, change it, and choose "Create Patch..." from the > directory context menu: > http://tortoisesvn.net/docs/release/TortoiseSVN_en/tsvn-dug-patch.html I've created a patch using this method. How do I determine whether it is in "unified" format?
Make sure the lines start with + and -, not > and <. Create Patch should automatically use the unified format.
(In reply to comment #0) > 7. Disable code that isn't ever used. > 7a. Disable special handling of tables with THEAD elements since the THEAD element is not supported by Wikimedia. I don't like this idea. Just because we don't support it now, doesn't mean that we won't support it in the future, and that extensions won't make use of it. Show/Hide even supports it. Personally, I just started using Drupal for my own portfolio, and I noticed that drupal has a nice feature with table headers. I don't know if this is JS, or browser built in. But when you scroll past the top of the table, the header follows at the top of the page so you always know what a column is about. That kind of thing makes me feel like finding a way to make it possible to use table headers inside of MediaWiki and make that feature possible. > 7b. Disable alternate row coloring. I don't like this idea either. It's plenty possible for people to be making use of this feature. Removing it would be a regression. Just because one wiki does not use a feature, does not mean that people don't make use of it. That shouldn't be the base reason for removing features. We're trying to create features, not kill them.
Row coloring is a noticeable performance hit for very large tables, so it should be at least disabled by default. If that's desired it should be done at the server level, preferably.
