[05:14:32] Hello. [10:58:52] Hello. Members of the Usability Initiative might want to comment on a proposal to improve readability of discussions [10:59:09] just like what was done at fr.wiki : http://fr.wikipedia.org/wiki/Wikip%C3%A9dia:Le_Bistro/3_octobre_2010 [10:59:24] the proposal is at http://en.wikipedia.org/wiki/Wikipedia:Village_pump_%28proposals%29#Add_projectwide_css_to_highlight_different_posts_in_a_discussion [11:02:00] the question is: is there a mailing list for Usability? Who are the persons involved in interaction design ? [11:03:09] There's no mailing list [11:03:27] The best person to contact about interaction design would probably be Parul Vora [11:03:56] Also note that the Usability Initiative was based on a grant that ended in May [11:07:17] Dodoiste: for starters, the css isn't really very pretty [11:07:27] we'd prefer a holistic approach to discussions. as it happens, that's LiquidThreads [11:08:00] RoanKattouw, thanks :-) How shouly I call you guys then? [11:08:27] No idea :) [11:08:39] Good question for Trevor or Alolita [11:09:47] werdna, I completely agree, Liquidthread is the best. This proposal is only like a patch in the meantime. But Luiquidthreads won't go live before looong, so... [11:09:59] This patch might be useful in the meantime [11:10:13] nod :-) [11:11:08] Speaking of Liquidthreads, I'd love to have it enabled at our Wikiprojects Usability and Accessibility, for testing purposes [11:11:25] on fr.wp? [11:11:31] especially for testing accessibility and reporting corresponding bugs [11:11:36] no, en.wikipedia [11:11:58] ahh [11:12:19] http://en.wikipedia.org/wiki/Wikipedia:WikiProject_Accessibility [11:12:25] Enabling on en.wp itself is a way off (we're targetting February for that, but expecting to slip a little) [11:12:44] but we'd sure appreciate being told how we're going from an accessibility perspective [11:13:49] I reported a major bug a the beginning of th tests. It was solved quickly and nicely. I belive there might be a few minor bugs to report here and there [11:14:53] werdna, OK, then we could enable it at the accessibility project at fr.wikipedia http://fr.wikipedia.org/wiki/Discussion_Wikip%C3%A9dia:Atelier_accessibilit%C3%A9 [11:15:41] Dodoiste: well, first, as always, we need a consensus, and preferably also a bug filed [11:15:47] but I will talk to some people and see what we can do [11:19:34] werdna, OK, I'll begin a discussion there, and fill a bug. But if you can get other people to agree on a quicker way that would be cool too. [11:21:34] nod [11:21:40] I'll talk to some people [11:22:17] :-) [17:02:24] Morning flipzagging, I was just getting started on reviewing your code [17:02:37] RoanKattouw: hey [17:02:44] thanks for that [17:02:46] Gonna commit some minor fixes and FIXME comments to SessionStash.php now, followed by a stylize run [17:02:53] please do [17:03:31] After that I also wanna talk to you a bit to get some clarity on something, but let me commit first [17:23:22] flipzagging: Committed http://www.mediawiki.org/wiki/Special:Code/MediaWiki/74263 , please read that. I did change a few things without checking to see if they broke callers (bad Roan!) so I'll grep for callers now [17:23:33] RoanKattouw: heh [17:23:36] let's see [17:23:38] Mostly exception -> null and false -> null [17:23:48] I kept changing those conventions, sorry [17:23:48] In the former case the docs actually said it'd return null ^^ [17:24:05] Feel free to revert me, as long as the conventions are consistent [17:24:07] also, I'm throwing a lot of exceptions and such, not sure if this is kosher for MediaWiki style, but I find it useful [17:24:25] The code I read had one function returning false, and another claiming to return null but throwing exceptions instead [17:24:51] So you should settle on /either/ null /or/ false /or/ exceptions, and use that somewhat consistently [17:24:56] oh, I see [17:25:04] no, the code was right, the comments were wrong ;) [17:25:18] Go ahead and change it back then :) [17:25:36] array_diff_key() ? That's kind of cool [17:25:40] The other thing I wanted to discuss is stash keys (the $key variable indexing $_SESSION) [17:25:46] ok [17:26:05] The code and docs seem to be somewhat contradictory on whether they're integers or strings [17:26:18] :) [17:26:22] the answer is yes ;) [17:26:30] well, this is how it works now [17:26:40] UploadBase always creates random integers [17:26:42] Then there's this one function that'll set $key as a random int between 0 and 2^32 - 1 if not set [17:26:50] yes [17:26:55] ...but doesn't document this fact at all [17:27:08] The $key param doesn't even have a default value of null/false/0 [17:27:26] that's a mistake then [17:27:41] but if I set that to have default the others have to have defaults too [17:27:44] other args [17:27:46] I also documented the fact that $key has to "be unique", which by my own admission is misleading as well (it only need be unique per session) [17:28:01] fair enough [17:28:13] Or change the param order [17:28:30] It's in the base class, there's no parent function or anything else to keep in mind, right? [17:28:52] SessionStash is its own thing. [17:28:57] no inherited classes [17:29:11] Right [17:29:38] So then SessionStashFile::transform() seems to use strings for $key [17:29:49] Specifically, extensionless file names [17:30:29] yes. [17:30:46] it's trying to keep the same practices as in UploadBase, which just used integers. [17:30:55] But, in reality, these are now pseudo-filenames for URLs. [17:31:06] so, the files have names like 12345 [17:31:13] and the thumbnails have names like 120px-12345 [17:31:31] OK [17:31:40] if I replace UploadBase::stashSession then I can make this a little more sensible, but I'm taking this one step at a time [17:31:48] Sure [17:31:54] if I get a nice API test suite together I could more plausibly replace it all [17:32:03] but I decided to defer that this weekend ;) [17:33:02] why ->getHandler() to ->handler ? [17:33:15] I know in most cases they are the same, but getHandler is slightly safer. [17:33:25] I changed it the other way around [17:33:30] ->handler to ->getHandler() [17:33:38] Or at least that's what I intended to do [17:33:41] oh right, I'm reading it wrong. [17:33:42] nm [17:42:25] that array_diff_key trick works but I find it requires explanation [17:42:34] is that a really common idiom? [17:42:48] good morning peoples [17:42:58] hey trevor, be my guinea pig [17:43:11] what does this do? [17:43:13] $data = array_diff_key( $stashData, array( 'mTempPath' => true ) ); [17:43:52] maybe I should try this in mediawiki [17:44:11] it returns the diff off the keys of those two vars? [17:44:43] data is now a copy of stashData minus mTempPath [17:44:47] Sam as: $data = array_diff( array_keys( $stashData ), array( 'mTempPath' ) ); [17:45:03] yes, I think so [17:45:06] were you looking for an interesection? [17:45:08] No [17:45:40] Roan changed some of my code to use this idiom, I wasn't familiar with it. It's neat, just concerned if it needs a comment or if I'm the only one that doesn't know about this. [17:46:01] he's big on functional programming [17:46:15] well, this isn't functional programming... it's PHP shortcuttery [17:46:36] yeah, feel free to comment it, but basically diff in PHP array terms is a ( "this" without "that" ) sort of thing [17:48:07] well, using array_diff_key might not be an amazing feat in functional programming trickery, but it's a more functional approach than writing a loop that collects the non-similar cross section of the keys... [17:48:11] what was there before? [17:48:25] I just iterated over keys [17:49:47] like: foreach ( array_keys( $data ) as $keys ) { ... } [17:50:06] http://www.mediawiki.org/wiki/Special:Code/MediaWiki/74263 [17:50:08] how did you skip mTempPath then? with a conditional? [17:50:12] search for "flattened" [17:50:44] n.b. Roan's fix is slightly wrong even as it was (diffed data with itself) [17:51:22] his approach seems less clear... [17:51:39] it's fairly elegant though, we shouldn't be afraid to use the language. [17:51:50] also, it seems possible that in your approach that $path will never get defined [17:52:02] maybe not, if I knew more about the rest of the code [17:52:09] yyyeah [17:52:10] well [17:52:24] is mTempPath always there? [17:52:28] same thing with his code [17:53:05] there should be a check if mTempPath is there or not since it's essential, however SessionStashFile will throw an exception anyway if the path is wrong [17:54:10] in your implementation, $path may not be defined, in his $stashData['mTempPath'] may not be defined [17:54:18] they have the same issue, just in different places [17:54:52] How is "$file = new SessionStashFile( $this, $this->repo, $path, $key, $data );" going to cope with $path being null btw? [17:55:05] if you were to pre-initialize it [17:55:10] validateFilename should catch it [17:56:16] so, it isn't optional? [17:56:56] sorry, i really should learn more about this code [17:57:00] it's okay [17:57:07] I'm doubt I'm being helpful making you explain things in great detail [17:57:10] Roan is reviewing it, I didn't mean for you to get this deep now [17:57:30] well, the $path parameter is essential, but the constructor will detect a bad value anyway. [17:57:44] that said, it's not obvious to a casual reader that it will. [17:57:55] basically, I'm just getting at the idea that you should throw an errror as early on as possible if $stashData['mTempPath'] is required but invalid/not set [17:58:20] this seems to be a piece of code that assumes it's there, and I'm unaware of any guarantees to that effect [17:58:29] right, this is a good point [17:59:05] Furthermore, his implementation is insane [17:59:19] the $data $data thing? [17:59:26] $data = array_diff_key( $data, array( 'mTempPath' => true ) ) == unset( $data['mTempPath'] ); [17:59:37] why is his implementation so freaking complex? [17:59:38] yeah, but that's not what we wanted [17:59:50] the second $data is supposed to be $stashData. [17:59:53] well, then his implementation is even more insane [17:59:57] oh [18:00:02] I'm fixing this. ;) [18:00:04] yes, thats no good [18:00:32] that said, we could just use the unset method since we don't care about stashData anyway. [18:00:40] yeah [18:00:44] and then it's REALLY simple. [18:00:44] Hmm yeah I guess I should just use unset() [18:00:46] Sorry :) [18:00:54] no, it was rather obfuscated to start with [18:01:06] RoanKattouw: we are just having fun, sorry if I came of as overly critical [18:01:10] RoanKattouw: good morning [18:01:40] No worries [18:01:57] I was gonna mention one other remark before getting pulled into helping my sister with math [18:02:11] Which is that there's like a one in a zillion chance that mt_rand() will produce a collision [18:02:41] One in ~4 billion I should say [18:03:03] For paranoia it'd be nice if you'd check the availability of $key after the mt_rand() call :) [18:03:19] RoanKattouw: can you take https://bugzilla.wikimedia.org/show_bug.cgi?id=22680 ? [18:03:28] i mean, it's assigned to you, but... [18:03:31] can you poke? [18:03:39] Sure [18:03:44] I've fixed that issue like twice now [18:03:54] i can see that from the comments [18:04:01] If this is another one of those jQuery 1.3 bugs I'm gonna yell at someone [18:04:03] you seem more familiar with it [18:04:35] When is Wikimedia switching to jQuery 1.4? [18:04:42] Well the resource loader uses it [18:04:46] Aye. [18:04:50] And I see no reason why we couldn't switch to 1.4 sooner [18:04:58] I think it'd be smart to do them separately. [18:05:04] Yeah [18:05:28] ok [18:05:38] I'm not totally sure the deployed code doesn't have weird bugs caused by 1.4 incompat that went away magically at some point. Back in January right after the 1.4 release trunk sure did have that problem [18:05:47] I have to attend a staff meeting thingy [18:05:50] So I'd have to prototype/sandbox a deployment install with 1.4 [18:05:53] Oh right, 11 o'clock [18:05:54] Isn't the current 1.4.1? [18:06:07] be back in a bit [18:06:08] Yeah it could also be that [18:06:19] That jQ fixed it in a .1 release [18:06:34] Either way I think RL uses 1.4.1 [18:06:55] I checked, it's 1.4.2 even [18:07:23] flipzagging: See comment about mt_rand() possibly producing a collision in scrollback. Now gonna poke at a JS bug for a bit, then back to UW review [18:08:01] So the point releases might have fixed some of the bugs. [18:08:22] Yes [18:08:47] The bug I was hitting was that $( 'any selector here' ) always returned the document in addition to whatever the selector matched [18:09:08] For some reason this only happened in an MW environment, not on jQuery.com [18:09:49] This was back in January after the 1.4.0 release. I didn't really have time to debug that issue so I kinda forgot all about 1.4. Then when I tried again 6 months later with a later point release of 1.4, I couldn't reproduce [18:34:09] Well, crap [18:34:21] I've discovered what the cause of that bug is, Trevor [18:34:44] Probable cause, at least [18:35:39] Actually the bug seems to be more profound on Wikipedia.... [18:43:33] I'm back - what's up? [18:45:30] OK looks like thedj was right [18:45:33] It's a jQ 1.3 bug [18:45:38] See there's this memory leak bug in IE [18:45:50] jQuery uses a cache object (jQuery.cache) to cache all sorts of stuff in [18:46:01] Unless it manually unsets that, you get a memory leak in IE [18:46:27] So 1.3 unconditionally does $(window).bind('onload', function() { for( var i in jQuery.cache ) { do stuff } } ); [18:46:50] And Firefox, which is not affected by said memory leak, notices that there's an onunload handler and refuses to bfcache the page [18:47:00] So instead, 1.4.2 chose to do it in IE only [18:47:16] if ( window.attachEvent && !window.addEventListener ) { [18:47:18] window.attachEvent("onunload", function() { .... [18:47:48] Moral of the story: migrate to jQ 1.4.2, problem solved [18:47:59] However, I discovered /another/ bug that's nigh impossible to fix [18:48:18] ha ha... /listening [18:48:26] Which is that if the warning pops up, bfcache is still broken [18:48:47] isn't that the point of the warning? [18:48:54] Well, no [18:48:54] saying, dude, you are about to loose your work [18:48:59] Yes, but... [18:49:04] bfcache is not just about losing work [18:49:18] It's about reloading the page (i.e. from the server), executing JS again, etc. [18:49:29] Well maybe it won't contact the server [18:49:40] But it will re-execute JS at the very least [18:49:59] The reason the popup breaks bfcache is as follows [18:50:21] The onbeforeunload handler detaches itself from window.onbeforeunload while it runs, so that if there's no popup, there won't be any handler and bfcache will work [18:50:32] However, if there is a popup, it'll use setTimeout() to restore it [18:50:51] The logic here is that if the user clicks Cancel, then tries to leave the page again later, the handler should still be there [18:50:53] RoanKattouw: I refuse to write code that deals with events that are rarer than cosmic rays. [18:51:08] RoanKattouw: that said, the real issue would be if the PRNG was broken. [18:51:27] However, the timeout timer happily continues running while the user is reading the dialog [18:51:43] RoanKattouw: but in that case, I would try to write a real UUID sequence, no point fooling around with random numbers at all. [18:51:54] I discovered this by using a 5000ms instead of 0ms timeout, and observed that if I clicked OK fast, I'd get a working bfcache, but if I took my sweet time it'd break [18:52:45] flipzagging: It's just that probabilistic things scare me. Hash-based would be fine I guess [18:53:00] that would be logical, basing it on the hash of the file. [18:53:21] Do you have access to the hash of the file contents at that point? [18:53:29] yes, if we're creating a new one. [18:53:43] OK [18:53:45] I would have done it that way myself except for trying not to change too much at once. [18:53:53] That'd also introduce the 'feature' of filtering duplicate files [18:54:00] yyyyeah [18:54:08] There should probably be some client-side UW support for that too, otherwise it's too magical [18:54:23] there is the normal uniqueness check [18:54:36] sha1 versus database [18:54:45] That's not the problem [18:54:52] It's about trying to upload two copies of one files at once [18:54:54] I know what you mean [18:55:02] corner case [18:55:04] Or two copies of one file within one session, generally [18:55:19] Could be documented as "don't do this, it's stupid and will break this code" [18:55:21] two copies of one file within one session is more likely. The first could fail. [18:55:36] Right [18:55:45] Well the second upload would overwrite the first in that case [18:55:47] I think overwriting it is Just Fine. [18:55:53] Exactly [18:56:02] Because that's what MediaWiki would do anyway, except if instructed otherwise. [18:56:09] The session stash's behavior would be a bit weird, but could be documented [18:56:20] Anyway -- I know you're on another review, but do you think this Session Stash notion is even sane? [18:56:43] Bryan Tong Minh was implementing an entire new zone for files, I thought this was "simpler" but it's arguably rather strange. [18:57:02] when I was innocent and young, like in August. [18:57:10] I thought this was simpler, I mean. [18:57:17] This makes sense [18:57:28] k [18:57:29] You very specifically do not want other users to be able to access files [18:57:34] One thing that bothers me [18:57:43] right, but we could implement that in the db too. [18:57:52] Is that if the key is randomly generated by stashFile(), the caller seems to not be able to retrieve the generated key [18:58:02] you get it in the API return. [18:58:16] I am thinking I should add the feature where you can assign it if you really want to. [18:58:23] Or wait yeah [18:58:33] You get an object back with a public (why?!?) session key field [18:58:39] ? [18:58:42] public? [18:58:56] public $sessionKey; [18:59:04] In SessionStashFile [18:59:06] well, PHP is single threaded, its publicness is irrelevant to security. [18:59:29] I'm paranoid, you know that ;) [18:59:35] well, okay [18:59:53] As a general principle, a class should work in isolation and be resistant to deliberate tampering from other code that is within the bounds of what the docs/specs allow [18:59:56] For the sake of form we can make a private var with accessor, so no one can inadvertently change the key. [19:00:02] Exactl [19:00:26] RoanKattouw: if you want me to get all formal with this code I can, but that's the kind of code Tim ridicules ;) [19:00:41] but sure, your approach is better style [19:00:49] I don't think Tim would find that ridiculous at all [19:01:03] If there were a setter without validation, sure, then it's kinda ridiculous [19:01:30] But to preserve the validity of this object, the session key needs to explicitly be unsettable [19:01:44] ok, let me write a note, I'll try to be less promiscuous with members. [19:02:03] Promiscuity is fine if you really don't care who sets it and to what [19:02:09] immutability is in general awesome, though [19:02:13] it's almost functional then :) [19:02:18] i.e. if you would end up doing function setFoo($foo) { $this->foo = $foo; } anyway [19:02:24] Yes, immutable was the word I was looking for [19:02:49] However, if your prospective setter contains some validation check or should be deliberately omitted, that's a reason for the field not to be public [19:39:52] Oh well crap [19:40:18] Not only does jQuery 1.3.2 cause this, that $.stars plugin adam_miller pulled from somewhere causes it too [19:40:37] For the same reason even (deliberate cleanup to prevent memleaks in IE6) [19:42:05] RoanKattouw what does it cause? [19:42:14] bfcache breakage [19:42:40] Because the stars plugin sets an unload handler to clean up some stuff to IE6's broken garbage collector won't cause memory leaks [19:42:51] However, it does so unconditionally, also in non-IE browsers [19:43:08] Which in turn causes Firefox to think that the page is not cacheable and has to be reloaded when you click the back button [19:43:21] So the feature where clicking back in Firefox is instantaneous doesn't work [19:43:57] so should i warp that unload handler in some if(IE) type code? [19:49:16] I've just done that [19:53:04] so, I'm splitting off the iframe specific stuff [19:53:11] it's about 69k of raw JS code [19:53:19] sorry. 60k [19:55:32] :O [19:56:26] OK I'm gonna schedule the jQuery 1.3.2 -> 1.4.2 migration on Wikimedia for Oct 11 (next Monday) around 07:00 UTC [19:58:18] RoanKattouw: would it be possible to get a dump of the survey data from the ArticleFeedback survey? [20:00:06] Yes. Tomorrow. [20:00:14] Please send me an e-mail requesting it [20:05:23] arrggghhh... robla wrong laptop today.. going to go join brandon [20:13:20] RoanKattouw: wow - splitting off the iframe thing [20:13:26] dropped the final payload by 30k [20:14:02] Nice [20:14:03] After gzip? [20:14:08] yeah [20:14:19] and the old iframe hackery at the bottom of wikiEditor.js [20:14:37] it's now an elegant requirements system, that activates context extensions on the fly [20:14:51] it uses the isRequired thing [20:15:18] Cool [20:16:05] ooh, even better [20:16:16] contentCollector is not needed for the textarea stuff either [20:16:46] i'm down to 118k for the entire edit page JS (including vector extension) down from about 155k. [20:24:31] RoanKattouw: I am going to clean up the preferences I think [20:24:50] highlight is now able to be depended on naturally, rather than having to manually turn it on because other things need it [20:24:59] Ah yes [20:25:02] so, that pref/feature can go bye bye [20:25:04] Oh the niceties of RL [20:25:09] yes [20:25:57] I also think things like templateEditor, templates, previewDialog should be user/global false by default [20:26:19] The experimental, dangerous and known-broken ones? [20:26:50] yeah [20:33:36] RoanKattouw, thanks for fixing this bug 22680 (Vector EditWarning breaks page caching): I'll finally renable the enhanced toolbar :-) [20:33:46] It's not fixed [20:34:03] It will hopefully be next Monday [20:34:05] yet, but soon, isn't it? [20:34:17] :-) [20:35:00] 'cause I love this toolbar, but when my browser crashes, or Wikipedia fails, or else I lose the text I'm typing. And I hate that :-) [20:35:28] :) [20:35:43] OK guys, got school in the morning, see you all tomorrow [21:58:18] looks like 135 total to me [23:00:42] Today, in SuperDuper Shocking News: Twitter founder Evan Williams is "stepping down" and the COO gets his job! [23:01:02] This just in: Venture Capitalists like to see Money Being Made, not Thrown Away!