[03:21:13] @RoanKattouw: Is there any update on https://gerrit.wikimedia.org/r/#/c/105647/ [03:21:26] Does it work for you? [16:24:51] Heya. [16:47:32] * Elitre waves at her PM [16:48:17] Aha. Hey Elitre, *this* IRC server has people in it! :-) [17:06:20] it "had" people in it :) [17:46:20] (03PS2) 10Mooeypoo: Add placeholders to MediaSizeWidget [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/114773 [18:01:42] (03PS3) 10Divec: WIP:Fix handleEnter in nodes that don't split [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/109693 [18:02:17] (03CR) 10jenkins-bot: [V: 04-1] WIP:Fix handleEnter in nodes that don't split [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/109693 (owner: 10Divec) [18:10:15] (03PS1) 10Cmcmahon: Update to latest mediawiki-selenium gem for better login [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/115207 [18:12:48] (03CR) 10Jhall: [C: 032] Update to latest mediawiki-selenium gem for better login [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/115207 (owner: 10Cmcmahon) [18:13:58] (03Merged) 10jenkins-bot: Update to latest mediawiki-selenium gem for better login [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/115207 (owner: 10Cmcmahon) [18:35:35] (03PS5) 10Mooeypoo: [wip] Set up wiki-default image size [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/114420 [18:40:40] mooeypoo: Just checking: https://gerrit.wikimedia.org/r/#/c/114420/ is the ve-mw commit that https://gerrit.wikimedia.org/r/#/c/114773/ refers to? [18:41:10] RoanKattouw, yes, oops, I linked them one-sided [18:41:32] I forgot to add the link to the commit message of this commit [18:42:50] No worries [18:43:11] Arguably ve-core commits should generally avoid referring to ve-mw commits [18:43:21] But I just wanted to get things straight [18:43:24] in my head [18:49:57] RoanKattouw, it doesn't depend on the mw commit, (the other way around is true) but to test it, you'll probably have an easier time working with both [18:50:19] Sounds good [18:50:26] Yeah I was going to test both at the same time [18:51:21] edsanders:I reopened this last week for a small issue: https://bugzilla.wikimedia.org/show_bug.cgi?id=60180 [18:52:39] Right now, both the scroll bars in keyboard shortcut page are moving simultaneously [18:55:26] (03PS4) 10Trevor Parscal: [WIP] Automatic menu positioning (above or below) [oojs/ui] - 10https://gerrit.wikimedia.org/r/114899 [19:03:19] (03CR) 10Trevor Parscal: [C: 032] doc: Use lowercase types where primitives (not objects) are expected [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/114628 (owner: 10Krinkle) [19:04:21] (03Merged) 10jenkins-bot: doc: Use lowercase types where primitives (not objects) are expected [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/114628 (owner: 10Krinkle) [19:07:04] (03CR) 10Trevor Parscal: [C: 032] Resize handle images [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/114475 (owner: 10Esanders) [19:08:01] (03Merged) 10jenkins-bot: Resize handle images [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/114475 (owner: 10Esanders) [19:11:37] James_F, btw, I was in Hunter on Friday, accompanying Sumana as she told students about GSoC. We talked a little afterwards, and she made the point that GSoC projects have to be about code.. so.. no documentation. [19:11:46] *but* OPW's can be documentation. [19:15:25] James_F: hello [19:18:45] mooeypoo: Ah, right. [19:18:46] divec: Hey [19:23:32] I wonder if you could help me with https://gerrit.wikimedia.org/r/#/c/109693/ [19:24:13] You reported an issue which I can't entirely reproduce [19:25:22] divec: Ah. :-( [19:25:37] divec: My local test install of MW is completely hosed, unfortunately. [19:25:53] Ok [19:26:21] divec: I /think/ it was when the cursor was between two lists, but I could be wrong. [19:26:29] Between two lists: great [19:26:32] I'll try that then [19:26:56] divec: And it might have been caused, or indeed fixed, by some other changes – I didn't entirely convince myself that this wasn't already an issue. [19:27:30] Ok, will investigate [19:28:46] (03PS4) 10Divec: WIP:Fix handleEnter in nodes that don't split [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/109693 [19:29:28] (03PS3) 10Trevor Parscal: Endash rather than hyphen for copyright date range [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/107177 (owner: 10Jforrester) [19:29:45] (03CR) 10jenkins-bot: [V: 04-1] WIP:Fix handleEnter in nodes that don't split [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/109693 (owner: 10Divec) [19:31:03] https://gerrit.wikimedia.org/r/#/c/107177/2 did a trivial rebase, but it would appear there's some desire to abandon this? [19:31:14] (03CR) 10Catrope: [C: 04-1] [wip] Set up wiki-default image size (033 comments) [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/114420 (owner: 10Mooeypoo) [19:31:56] TrevorParscal: edsanders made some arguments about how he doesn't think it's worthwhile because his OS'es IME sucks, and some other people also can't be bothered. [19:32:10] TrevorParscal: I don't really care one way or the other, except that we should be consistent. [19:32:20] Well, I'm thinking that the reason I wrote it with a hyphen is because that's how normal people write date ranges, and maintaining the endash approach will be an invisible inconsistency we will have to maintain forever [19:32:31] but, I really don't care which is correct [19:32:34] Sure. Feel free to abandon. [19:33:42] (03Abandoned) 10Trevor Parscal: Endash rather than hyphen for copyright date range [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/107177 (owner: 10Jforrester) [19:35:02] RoanKattouw, I am a little confused about your last point, the 'scale a scaled value' ? [19:35:20] do you mean I should use the image dimensions rather than its thumbnail dimensions? [19:35:32] Yes [19:35:34] RoanKattouw's point isn't scaling [19:35:41] The thumbnail dimensions have already been scaled by the server [19:35:48] RoanKattouw, if that's the case, I was worried that thumb might have different dimensions than image, like how some places crop thumbnail. *BUT* I am not sure that's happening in MW at all [19:35:53] Using the exact same logic you're using to scale again, just in PHP instead [19:36:22] ok, so they're not auto produced thumbnails. [19:36:24] So, .thumbwidth and .thumbheight are already computed as width/ratio and height*ratio in PHP [19:36:30] Well yes they are auto produced too [19:36:49] They're just scaled images. It's not like (escuse the horrible comparison :p ) wordpress that crops/scales thumbs per user configuration [19:36:55] As in, if you follow the URL of the thumbnail, there's a machine (confusingly also called a scaler) that will make one for you on demand [19:37:07] * mooeypoo nods  [19:37:19] Well, we don't *crop* thumbs per user config [19:37:32] ok i wasn't sure I can trust that thumbnail is exact same ratio as original, so now it makes more sense. [19:37:42] But as you are aware we do scale them to arbitrary sizes that in some cases depend on user config [19:37:49] Well, so the problem is you /were/ trusting that [19:38:05] right, if it's just scale, i can trust the original image dimensions and scale from that [19:38:09] And while it's true that they are the same ratio, that's true modulo rounding errors [19:38:18] And I'd like us not to have compounding rounding errors :) [19:38:23] RoanKattouw, no, I went with the thumb scale, so if the thumb was cropped, the new presentation will go with *that* dimension [19:38:35] Oh, right [19:38:39] Well we never crop the thumb [19:38:49] but you're right, if it doesn't crop ever, then originals make more sense [19:38:50] Only scale it using the same math that we use in VE already [19:38:54] * mooeypoo nods [19:39:33] I'm just worried that if there's a compound rounding error, the value you compute may not be equal to the value you would have computed if you scaled to that size directly [19:39:49] And such an inconsistency will find a way to cause a hard-to-debug bug [19:40:13] ... aaa! my local wiki fluked. [19:42:21] ah. update fixed it. [19:43:25] (03Abandoned) 10Trevor Parscal: Use endash rather than hyphen for copyright date range [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/107178 (owner: 10Jforrester) [19:47:04] James_F: Thanks, the list sandwich works nicely (that is, it fails consistently) [19:47:54] divec: Yay for my half-remembered bug report. :-) [19:48:24] (03PS6) 10Mooeypoo: [wip] Set up wiki-default image size [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/114420 [19:49:36] (03CR) 10Divec: [C: 04-1] "As James notes, it fails in "...

|

    ..."" [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/109693 (owner: 10Divec) [19:49:42] (03CR) 10jenkins-bot: [V: 04-1] [wip] Set up wiki-default image size [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/114420 (owner: 10Mooeypoo) [19:49:57] (03CR) 10Catrope: [C: 04-1] "This seems to work great, except that the media size widget is not disabled when the default button is selected. So while the button is in" (031 comment) [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/114420 (owner: 10Mooeypoo) [19:50:15] (03CR) 10Catrope: "Also, stray console.log, see inline comment. That's probably why Jenkins failed as well." [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/114420 (owner: 10Mooeypoo) [19:50:18] RoanKattouw: Is there a better way to to select dm nodes by type ? Right now i'm using selectNodes, range is from the beginning of the doc to the end, using 'covered'. which gives me text nodes, then I check the parent for type. [19:50:50] rmoen: What are you trying to do? Find all MWHeadingNodes I guess? [19:50:53] yep [19:51:44] Hmm [19:52:38] Yeah so that strange issue we saw where the numbers jumped is from looping through all text nodes and then adding the parent.. i could filter out duplicate mwHeading nodes [19:52:50] ^ I suppose [19:53:22] RoanKattouw: It just felt a bit hacky so i wanted to be sure there wasn't another way before i go down that road [19:54:10] Oooh I understand the counting issue now [19:54:12] Yeah, OK, so [19:54:15] yea [19:54:16] hah [19:54:40] The "best" way to do this now is to use selectNodes, range beginning to end, mode 'leaves' (I think 'covered' is different but I'm not sure how offhand) [19:55:09] yeah, covered gives me all the text nodes and inline transclusions [19:55:12] Then ignore non-text nodes, for text nodes, get parent, filter for mwHeading, filter duplicates [19:55:23] Ahh [19:55:36] Right that sounds like it's the same as 'leaves', probably because the distinction between the two disappears when selecting the entire document [19:55:38] ok so basically filter duplicates [19:55:45] yeah [19:55:48] Filtering duplicates BTW is easy because you know that duplicates will always be successive [19:56:12] So you can have Foo - Bar - Bar - Bar - Baz , but not Foo - Bar - Baz - Bar - Bar or whatever [19:56:14] RoanKattouw: Yeah just cache the leading mwHeading [19:56:21] Exactly, just compare to previous [19:56:31] So, I put "best" in scare quotes [19:56:45] Because it would be nice if there was a utility for "give me all nodes of this type" [19:56:49] And I should write one at some point [19:57:01] RoanKattouw: Yeah, I was just hoping there was such a thing [19:57:02] heh [19:57:04] But since the nodes you are interested in happen to be content branch nodes, you're fine [19:57:08] Oh, one last thing: [19:57:18] When I said "ignore non-text nodes", I lied [19:57:34] You have to check .isContent() instead [19:57:50] Otherwise you might miss a heading that contains only an inline transclusion or only an image or whatever [19:57:59] Right [19:58:07] And while you may not be able to do an intelligent text rendering especially for the image case, you don't want your counts to skip it [19:58:12] If it only contains a non text node we would miss it [19:58:16] Yeah [19:58:39] Cool ty [19:58:50] Also, I suppose you could just check if the parent is an mwHeadnig [19:58:57] Yeah [19:59:00] just check dupes [19:59:05] You have to do that anyway, and if .isContent() is false then the parent definitely won't be an mwHeading anyway [19:59:17] So forget what I said about checking if the node is a text node or a content node or whatever, just check the parent type [20:00:12] Should we have a general policy on permissible source code characters? [20:00:40] RoanKattouw, James_F, TrevorParscal: ^^ [20:00:50] divec: How would we do tests for IMEs then? [20:01:10] :-) [20:03:24] RoanKattouw_away: Sweet! "RoansMainPage" TOC is now building correctly ;) [20:03:59] In all seriousness, I prefer to use \uXXXX escapes in strings, as otherwise crappy tool support can add an extra dimension of mindbogglingness [20:04:53] And I'd probably consider banning non-ASCII characters outside of strings in .js files [20:04:58] @RoanKattouw: Hi, is there any update on https://gerrit.wikimedia.org/r/#/c/105647/ [20:05:33] HTML / wiki test data, on the other hand, needs the full glory of any weird unicode we can devise :-) [20:05:50] Hey James_F, So RoanKattouw_away and I thought it would be cool if you could come up with an extreme TOC test page? As your wikitext skills > than ours :) [20:06:07] rmoen: Extreme in what ways? But sure. :-) [20:06:33] James_F: Something complex that you think might break my code [20:08:15] James_F: You know, like when you made the test page for me with categories, or mooeypoo's media settings test page [20:10:00] (03PS1) 10Jforrester: Disable redirect field unless the checkbox is set in setup [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/115237 [20:12:18] (03PS1) 10Jforrester: Don't let the user set a static redirect flag on a non-redirect [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/115239 [20:14:51] rmoen: Sure, making one now. [20:16:52] rmoen: https://www.mediawiki.org/wiki/VisualEditor:TestTOC1 is a start. [20:17:11] James_F: Sweet ty! [20:17:25] rmoen: And https://www.mediawiki.org/wiki/VisualEditor:TestTOC2 for __TOC__ placement in the same. [20:18:02] James_F: handy... no transluctions in the headings? [20:18:19] rmoen: Ha, that's a nice issue – will add. [20:22:05] rmoen: What about now? [20:22:28] rmoen: Note BTW MW's totally different handling of the titles to their HTML display value. [20:25:56] James_F: Nice that looks like a good test ;) [20:26:15] rmoen: Not totally sure it's pathological, but it's a start… [20:26:45] (03PS7) 10Mooeypoo: [wip] Set up wiki-default image size [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/114420 [20:27:14] ha, oops, RoanKattouw_away that's what happens when I fix code while eating... whoops [20:28:08] mooeypoo: Clearly you should work less! Oh, wait… :-) [20:28:23] hah [20:29:23] speaking of work, I think I might need a new task. I'm going over the remaining bugs in media edit dialog, and some seem to be fixed, or will-be-fixed-with-pending-commits or depending on other fixes first. [20:31:14] * James_F nods. [20:32:03] mooeypoo: Most of the things in https://www.mediawiki.org/wiki/VisualEditor/Roadmap#Next_up have been slightly cookie-licked, sadly. [20:32:34] mooeypoo: #3 might be an option, though Trevor's ideas about that possibly need fleshing out if you're going to want to do it. [20:33:02] mooeypoo: What's the status of inline images, BTW? I've lost track. [20:33:22] inline images await Trevor's refactoring to image data model [20:33:33] Ah, of course. [20:33:34] :-( [20:33:51] there are still a couple of small bugs in media edit dialog i'm working on fixing [20:33:56] but the list's getting smaller and smaller [20:34:22] Is 'return undefined' Halal? [20:34:44] mooeypoo: Yeah. [20:35:07] (we do it a couple of times) [20:37:42] ... Halal? [20:37:51] as in the food trucks? [20:38:12] mooeypoo: As in "is it allowed". [20:38:33] ("Kosher") [20:39:13] oh, so the word is the same meaning I thought of, but I've never heard it used in that context. [20:39:14] Interesting [20:40:15] mooeypoo: It's just me talking in a way that sounds a bit weird ("kosher" wouldn't sound weird at all). [20:40:41] divec: Doesn't sound odd to my ears, but then, London's Muslim community is a bit higher than North Wales. ;-) [20:40:51] It's okay, I just have a mental image of the NY food trucks whenever I hear that word. [20:41:04] "Gotos are haram" (as in "not halal") sounds slightly less weird to me [20:41:08] The kind of food you greedily consume when drunk. [20:41:28] Heh [20:41:50] Which is a sort of irony, I guess, but that's the way things are here. [20:46:42] James_F: is it just your MW that's broken? Can you test pure Javascript VE stuff? [20:48:47] divec: Yes. [20:48:54] divec: Sorry, "Yes and yes." [20:51:01] hm, James_F scalable's "max dimensions" seem to not apply at all to images. Is this on purpose? I remember we were only supposed to limit image size in certain cases. [20:51:38] I thought it was limiting to max dimensions of the image (so, no stretching) for all images except vector [20:51:57] (03PS5) 10Divec: Fix handleEnter in nodes that don't split [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/109693 [20:52:15] James_F: Any chance you could take a look at 109693 for me? [20:52:35] mooeypoo: There are different max dimensions for different image types, I think. [20:52:38] mooeypoo: Or something. [20:53:56] James_F, re this: https://bugzilla.wikimedia.org/show_bug.cgi?id=61282 [20:54:06] I dn't get a tiny dot, I get a huuuuuuuuuuge image. [20:54:39] I assume that's not quite what we want. Docs are very bad in explicitly saying what can and can't be scaled, though. It seems you can scale a thumbnail up, too, in wikitext. [20:55:09] divec: Looking now. [20:55:37] mooeypoo: Hmm. Yeah. Ask edsanders|away what he was doing? [20:55:44] * mooeypoo nods [20:56:02] I'm testing stuff in wikitext too. Docs are annoyingly vague :\ [20:56:42] divec: I no longer get the exception thrown; in general it seems good. [20:56:45] James_F: thanks. I myself tested a local demos/ve/pages/list.html consisting of
    • foo

    • bar
    [20:56:45] "thumb" seems to limit size to original size (so [[File:image.jpg|thumb|800000px]] renders as 400x300 because that's the original size. But without thumb, the image is as huge as I seem to want it. [20:57:01] James_F: Cool; that's good news. Thanks! [20:57:03] Oooh. [20:57:04] Uncaught Error: Unbalanced set of replace operations found [20:57:05] I have a 800000px-wide image on m page now, and it's rather fascinating. [20:57:07] That's not good. [20:57:14] I forgot to touch wood [20:57:39] Go to /w/extensions/VisualEditor/lib/ve/demos/ve/#!/src/pages/empty.html [20:57:54] Place cursor in first position and press Enter a few times. [20:58:17] Then re-place the cursor at the top and forwards-delete a few times. [20:58:27] Every few times (every other time?) it throws that. [20:58:44] * James_F checks. [20:58:56] divec: Not a new bug. Don't worry. [20:59:05] divec: Well. Do worry. But that doesn't block this. [20:59:21] Ah, ok. Thanks, that helps! [20:59:30] Indeed, select all and forward-delete and you get "Uncaught Error: Unbalanced input passed to document". [20:59:33] * James_F sighs at CE. [20:59:59] mooeypoo: Yes, Thumb is "make smaller or at most original size". [21:00:07] * mooeypoo nods [21:00:25] mooeypoo: So a 100x100px image in a thumb should be 100x100px, even though the thumb size is 220x160px (or whatever). [21:00:33] Also, given an empty, *non*-final bullet, should Enter break the list in two and insert an empty paragraph? [21:01:01] divec: Maybe. Current behaviour is not. [21:01:25] (It doesn't, but Libreoffice does and deletes the empty bullet in the process) [21:03:00] divec: Catrope to Earth, what's up? [21:04:08] divec: characters in source code, were you talking about characters in strings (as opposed to using escape sequences) or actual javascript symbols? [21:04:18] or, characters in comments? [21:04:24] All three. [21:04:30] TrevorParscal: I've added you on https://gerrit.wikimedia.org/r/115229 - MatmaRex and I are conspiring to kill a few different things [21:04:38] ok [21:05:06] Draft proposal: string literals SHOULD not contain unescaped (\uXXXX) non-ASCII characters [21:05:07] are you opposed to removing this stuff? [21:05:26] Comments SHOULD not contain non-ASCII characters [21:05:45] Outside string literals, Javascript code MUST not contain non-ASCII characters. [21:05:50] divec: I support that [21:05:50] End of draft proposal. [21:05:57] RoanKattouw: ? ^^ [21:06:47] rdwrer: Does that WikiEditor change fix the jQuery deprecation warnings in my console? [21:06:48] (Rationale: otherwise, someone somewhere will lose an afternoon dealing with a "paradox" caused by a badly-formatted diff) [21:06:57] rdwrer: If so, I'll just drive-by +2; they're hugely annoying. :-) [21:07:22] (I have been that afternoonless critter too many times) [21:07:30] divec, TrevorParscal: +1 re ASCII chars [21:08:06] James_F: No, that's a separate thing [21:08:08] s/badly-formatted/wrongly-displayed/ [21:08:35] I would have +2'd it myself if we'd fixed that [21:09:01] rdwrer: Is there anyone fixing that? [21:09:07] divec: now, write that into an RFC, spend weeks on end promoting it, wait for it to be scheduled to be discussed, defend it to the death over IRC and when it's finally approved, write a commit that makes the change, bully others into +2, and then carefully monitor the codebase for the foreseeable future, applying -1 to all offending commits [21:09:19] or... perhaps we shouldn't do things the "platform engineering" way [21:09:20] (03PS1) 10Jforrester: Always try to put redirects at index 0, offset 0 [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/115287 [21:09:25] James_F: Not to my knowledge [21:09:31] rdwrer: Boo. [21:09:39] TrevorParscal: Can jshint help? [21:09:40] TrevorParscal: Be nice. :-P [21:09:50] James_F: what warnings? [21:09:52] divec: maybe [21:10:03] divec: ASCII 8 or ASCII 7? [21:10:09] If not, I agree it's kinda uphill [21:11:27] James_F: Characters between 32 and 126 inclusive, together with 10 (newline) [21:11:34] divec: Eww. [21:11:39] divec: You monster. [21:11:57] Ok and 9 (tab) [21:12:15] Just cos you all like it so much :-p [21:12:15] divec: Sorry, yôü mønstèr! [21:12:23] * James_F grins. [21:12:31] James_F: what were all the square boxes for just then? [21:13:28] divec: Srsly? [21:13:46] Nô [21:13:57] Mucho bueno. [21:14:01] divec: Anyways, earth was calling? [21:14:05] RoanKattouw: +E. [21:14:22] RoanKattouw: Yeah ... re: https://gerrit.wikimedia.org/r/#/c/105231/ [21:15:38] OK [21:16:18] So the thing I'm failing at, like a frustrated Rubik's Cuber who wants to tear the stickers off, is: // TODO: Return only in non-slug, non-branch node circumstances [21:16:37] OK [21:16:58] in ve.ce.Surface.prototype.handleInsertion [21:17:31] So, you're saying the handleInsertion code should only be executed if the current selection is collapsed && ( inSlug || inBranchNode ) ? [21:19:14] Hmm, I think it may need to run in certain circumstances if the selection is not collapsed [21:19:43] Sorry, I was wrong [21:19:49] But otherwise, if ( inSlug || inBranchNode ) then we do need to do some processing [21:19:57] We should NOT run if the selection is collapsed && !inSlug && !inBranchNode [21:21:17] Which I think means that we have collapsed selection in a real text node, right? [21:21:27] Also, what is the reason handleInsertion shouldn't run in that case? [21:21:29] True (but also if uncollapsed, if the selection only contains content) [21:22:43] Avoiding running handleInsertion: Because we want to handle the text change via the SurfaceObserver [21:22:48] OK [21:22:54] There are additional circumstances though [21:22:57] Pawning for instnce [21:23:18] Isn't that handled by the Slug and Branch? (Sounds like a pub) [21:23:51] Pawning can happen in a real text node [21:23:54] If you have insertion annotations [21:24:12] in response to keyDown? [21:24:35] Yeah [21:24:39] Suppose the following case [21:24:56] I place the cursor in the middle of a text node with unannotated text [21:25:08] I press Ctrl+B or click the Bold button. The Bold button is now depressed [21:25:12] I type a character [21:25:20] That character should be inserted as bold [21:25:23] Ah, the current handleInsertion has all that handling inside an if ( slug || this.needsPawn(...) ) { ... } [21:25:35] Right [21:25:52] needsPawn() basically checks if surroundingAnnotations !== insertionAnnotations [21:26:02] Sorry, I don't know why I didn't read that correctly. Explains a lot [21:26:42] Anyways, I know you explained this to me last month, but please refresh my memory: why do we need to bypass handleInsertion in some cases? Is it because the newFromReplacement transaction has already done everything that was supposed to be done? [21:26:54] Yes [21:27:23] So a simple triggering of onContentChange is enough [21:29:08] Right! [21:29:09] (Zoom out back to the big story: we're trying to reduce the amount of changes echoed back to the surface, so the IME doesn't close/choke) [21:29:12] Yeah [21:29:40] So, handleInsertion happens first, we predictively ignore, native action happens, observer notices, onContentChange happens, does the right thing [21:29:56] The hard part is handleInsertion being able to predict that onContentChange is going to get it right [21:30:32] Exactly [21:31:36] I am jealous of complexity theorists right now, they can just come up with a nondeterministic algorithm that solves it and wave their hands and say there's a mapping to some deterministic algorithm :) [21:31:52] Heh :-) [21:32:26] OK, so let's figure out in what circumstances handleInsertion's actions are obsoleted by onContentChange [21:32:55] Pawning (including slug resolution) seems pretty clear that it needs to stay in handleInsertion [21:33:29] slug || pawn || branch? [21:33:39] No, we're not done yet [21:33:47] Yes [21:33:49] So, if the selection is collapsed, it's easy [21:33:51] Ok [21:34:12] We can compute the insertionAnnotations, check if we're in a slug, that gives us enough information to decide to pawn or not [21:34:21] In fact, for non-collapsed selections the existing behavior is fine, AFAICT [21:34:24] Ahm [21:34:30] for *collapsed* selections sorry [21:34:47] Apart maybe from the poll calls at the end? [21:34:56] Or I guess those are needed to cause onContentChange to be callde? [21:35:11] I think so. The case we're really trying to fix is selected text being overwritten [21:35:15] Yes [21:35:19] So, if the selection is not collapsed [21:35:33] Which chokes some IMEs because they select then overwrite [21:35:38] Then we go into lines 1575-1585 [21:36:06] If the selection is not just content, then we need removal cleverness [21:36:24] i.e. what's now unconditionally done on lines 1579-1581 [21:37:00] If the selection is all content, then we need to compute what the new insertion annotations will be [21:37:05] And then decide whether to pawn based on that [21:37:07] Yes. (Not forgetting the .clear() which has a subsequent effect) [21:37:35] Which seems fine with the existing logic [21:37:46] So, as far as I can tell, we could do something like this: [21:37:51] Hold on [21:38:00] (Also, where is the .clear() call? I don't see it) [21:38:14] OH! [21:38:29] Line 1583, I see [21:39:01] Is that .clear() call there just because we just removed something? [21:39:09] "We need to compute the new insertion annotations": in case someone's just clicked Bold or something? [21:39:36] So, I don't fully understand the logic [21:39:41] But look at line 1576 [21:39:51] "Pull annotations from the first character in the selection" [21:39:57] The .clear() call is there because of the immediately preceding .change(...) call. Otherwise the surfaceObserver will see a spurious discrepancy. [21:40:14] Perhaps .update() would have been a better name than .clear() [21:40:15] Apparently what we're inserting needs to be annotated with the same annotations as the first character of the set that we deleted [21:40:27] Yeah no this makes sense to me, after change you need clear [21:40:32] Also, hold on [21:40:38] I thought .change() called .clear()? [21:40:50] Did I not fix this in my clear-clear-clear-clear-clear commit? [21:41:01] this.model.change(...) calls this.surfaceObserver.clear() ? [21:41:29] Wait, no, of course now [21:41:31] *not [21:41:56] Never mind [21:42:08] OK, so [21:42:10] If we're in a non-collapsed text selection ... [21:42:17] Then if the user has clicked Bold ... [21:42:24] Then it will have applied it to the text [21:42:30] It looks like the intent of line 1576 is to prevent cases where replacing mixed-annotation text would behave stangely [21:42:46] divec: You can click Bold and then select text [21:42:54] So we shouldn't have non-realised annotations [21:43:06] Ah, ok. Is that a good thing? [21:43:11] So, I think it's a bit more complicated than just that [21:43:20] * divec compares LibreOffice [21:43:22] Cases where we have to pawn include: [21:43:59] * The annotations that should apply to the to-be-inserted character are not applied to the neighboring character, so the browser won't carry over properly [21:44:00] LibreOffice forgets you clicked Bold if you change the selection. [21:44:20] * The annotations applied to the selected text that's about to be removed are mixed (not all identical) [21:44:35] * The annotations of the selected text are all the same but don't match the preceding character [21:44:38] Also, it seems to be normal behaviour (e.g. LibreOffice) to copy the first character's formatting on overwrite-insert. [21:44:47] The second and third case are simplified right now by just doing the removal first [21:44:56] Yeah, so we do do first character right now [21:45:13] That's how we compute what should happen [21:45:37] And in the current implementation it's fairly clear what will happen in the browser because we always do simple insertions (if it was a replacement, we pre-applied the removal) [21:45:52] So, we need to figure out what browsers do natively when replacing annotated text [21:46:19] As far as I've seen (which is not exhaustive), they keep the formatting of the first character [21:46:26] Clarification [21:46:45] My suspicion is that it might be as bad as "if it is not the case that all selected characters have the same annotation and this doesn't match the annotation of the character preceding the selection, the browser behavior may be wrong" [21:47:00] But we could investigate that [21:47:19] They keep the formatting of the character at the end of the selection which is earliest in document order [21:47:20] Anyway, let's hear your clarification and then I'll pitch a proposed approach [21:47:46] (which is important for reverse selections) [21:47:59] Right, so .start as opposed to .from in ve.Range parlance [21:48:05] Yes [21:48:10] OK [21:48:16] So let me propose this for handleInsertion: [21:48:19] Just to confirm ... [21:48:23] Yes... [21:48:43] Are you saying we need the behaviour where you click Bold, then change the selection, and it will still be bold if you start typing? [21:48:55] I think that's something that currently works [21:49:07] Ok, but should it? Nothing else seems to do that [21:49:09] We could decide to make that no longer work [21:49:28] However, I argue that IMEs that select characters temporarily behind the scenes shouldn't trigger that to be cleared [21:49:52] Hmmm, that's an interesting point [21:50:13] I think it only matters if the IME selects something before there is any candidate text [21:50:18] Yeah [21:50:31] I was about to say, I suppose we can only have a pre-annotation if the IME is just opening [21:50:38] It's a weird edge case if you type 'CHA' and then press B and then expect to be able to type 'N' and get my name in bold [21:50:38] We can't have one appear while it's open [21:50:49] Yes [21:51:03] In most cases you probably won't be able to switch on bold without closing the IME [21:51:10] Yes, jinx [21:51:24] Presumably Ctrl+B would either close the IME or not be caught by the surface's keydown handler or both [21:51:34] And clicking the bold button may or may not close it I suppose [21:52:13] So I guess if Trevor and James are OK with it we could 1) never apply pre-annotations to replacements and 2) clear pre-annotations when the user selects something [21:52:23] But #2 would really just be reducing user confusion for the most part [21:52:24] I will happily click Wontfix if that ever appears in bugzilla [21:52:51] Yes; it ticks the "go with wordprocessors" box [21:53:13] Right, OK [21:54:06] So hopefully, if browsers are sane enough, that plus #1 means that we'd never have to pawn content replacements [21:55:36] (03PS1) 10Mooeypoo: Allow dynamic limit to maxDimensions [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/115306 [21:56:05] I very much hope so! [21:56:56] And if that is true, I think we can replace if ( !selection.isCollapsed() ) on line 1574 with if ( !selection.isCollapsed() && !selectionIsExclusivelyContent ) [21:58:37] And that should be all for the changes to handleInsertion I think [21:58:38] Because 1591 will not fire if !selection.isCollapsed(), right? [21:58:53] Be careful reasoning about line 1591 [21:59:08] (03PS1) 10Mooeypoo: [wip] Limit thumbnail dimensions in media edit dialog [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/115307 [21:59:19] Because the conditional removal on 1579-1581 may or may not cause the selection to become collapsed [21:59:36] James_F, I keep ruining ed's Scalable. [21:59:39] ;) [21:59:40] Clarification: if ( !selection.isCollapsed() && selectionIsExclusivelyContent ) ... [21:59:57] ... then 1591 won't fire [22:00:12] Yes [22:00:17] mooeypoo: Well, edsanders|away should be not-ill so he can shout at you. :-) [22:00:29] Because line 1579 will not run in that case and there's nothing else that modifies the selection [22:00:30] hehe [22:00:48] James_F, in any case, another bug fix ready for review, though once again, it's split to two in ve-core and ve-mw [22:00:49] and 1616-1617 should happen only if one of the this.model.change calls has happened, of course [22:01:36] divec: Really? [22:01:42] mooeypoo: Yay. [22:02:09] divec: Surely we should at least cause a poll to happen somehow? [22:02:15] Or is that handled elsewhere? [22:03:43] I also don't understand why that stopTimerLoop is there [22:04:17] Yiirrrrsss ... this is somewhat unclear to say the least [22:04:23] haaha [22:04:29] Sounds familiar [22:04:33] onDocumentKeyDown already polls [22:05:17] Except that right now it doesn't if this.inIme === true (a condition which approximately tells you nothing reliable about the IME) [22:06:07] this.inIme should probably be killed then [22:06:26] (03CR) 10Catrope: [C: 032] Disable redirect field unless the checkbox is set in setup [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/115237 (owner: 10Jforrester) [22:06:31] Some of the stopTimerLoop calls are there because we were trying to do a minimally disruptive replacement of old code which triggered a bunch of stuff when you called surfaceObserver.doAWholeBunchOfThingsForSomeUnclearReason() [22:07:07] ... rather than because they made any sense [22:07:31] Yeah I remember that :) [22:07:31] this.inIme should definitely die, and I think stopTimerLoop probably should too [22:07:40] (03Merged) 10jenkins-bot: Disable redirect field unless the checkbox is set in setup [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/115237 (owner: 10Jforrester) [22:07:43] stopTimerLoop itself seems useful for the focus/blur handler [22:07:51] But probably not for much outside that [22:08:00] (03CR) 10Catrope: [C: 032] Don't let the user set a static redirect flag on a non-redirect [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/115239 (owner: 10Jforrester) [22:08:17] (I'm not completely certain whether stopTimerLoop was written on the assumption that Javascript timers can act multithreadedly. There may have been a subtler reason, though) [22:08:31] (03CR) 10Catrope: [C: 032] Always try to put redirects at index 0, offset 0 [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/115287 (owner: 10Jforrester) [22:08:36] That's true [22:08:44] (03PS1) 10Catrope: Always try to put redirects at index 0, offset 0 [extensions/VisualEditor] (wmf/1.23wmf15) - 10https://gerrit.wikimedia.org/r/115309 [22:09:12] (03Merged) 10jenkins-bot: Don't let the user set a static redirect flag on a non-redirect [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/115239 (owner: 10Jforrester) [22:10:09] (03PS2) 10Catrope: Always try to put redirects at index 0, offset 0 [extensions/VisualEditor] (wmf/1.23wmf15) - 10https://gerrit.wikimedia.org/r/115309 [22:10:18] (03Merged) 10jenkins-bot: Always try to put redirects at index 0, offset 0 [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/115287 (owner: 10Jforrester) [22:10:26] divec: Right, it seemed to be used as a "disable interrupts as a poor man's synchronization tool to deal with concurrency" thing [22:10:58] It did rather [22:11:22] So, Trevor has a meeting with Alolita right now (as you may be aware) but once he's back we should discuss the clear-pre-annotations-on-selection thing [22:11:44] Cause if they're on board with that, that would make things simpler [22:11:49] Yeah. Want to ask him, or shall grab him later? [22:12:28] Let me peek into the meeting room to see if he and Alolita are actually in there [22:12:32] (I get up at seven for LE stand-ups now, so I'm fairly keen not to be up in several hours time) [22:12:44] If so I'll wait until they emerge and ambush him then [22:12:54] We can also discuss with James now and with Trevor later [22:12:59] He should emerge in no more than 45 mins [22:13:15] Sorry, make that no more than 17 mins according to his calendar [22:14:42] James_F: We want to kill the current functionality, whereby you can click Bold, then move the cursor to somewhere not bold, then start typing and it comes out bold. [22:15:41] (03CR) 10Esanders: [C: 04-1] Allow dynamic limit to maxDimensions (035 comments) [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/115306 (owner: 10Mooeypoo) [22:15:53] oh, hi ed! [22:15:56] edsanders, feeling better? [22:16:03] marginally [22:16:15] divec: So James and I looked at this [22:16:18] James_F: Other wordprocessors etc don't seem to allow it, and it looks confusing to me. (Furthermore it makes my life easier in handling overwriting, though that's secondary) [22:16:22] Ok, cool [22:16:26] And it looks like VE already mostly if not entirely behaves this way [22:16:30] well enough for some grammar pedantry [22:16:37] I tried getting a selection + a pre-annotation and I couldn't [22:16:55] In fact, if I even so much as change the cursor position, my pre-annotation goes awy [22:17:04] Which seems reasonable enough behavior [22:17:16] And it has all the properties that we want [22:17:23] (03CR) 10Esanders: Allow dynamic limit to maxDimensions (031 comment) [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/115306 (owner: 10Mooeypoo) [22:17:27] Provided it's actually implemented correctly, which we'd have to check [22:17:59] Which ... hold on [22:18:05] *headdesk* [22:18:14] This follows directly from the code I was reading all along [22:18:50] If the selection isn't collapsed, get the annotations of the first character (call them A), delete the selection, set the insertion annotations to be A [22:19:14] So, obviously, by the code I was *staring at the whole time* , pre-annotations on selections already don't work [22:19:16] edsanders, just curious "isForced" is past tense? I thought it's passive..? As in "it's forced to be X" ? (this is really unimportant in the large scheme of things, and I will rename it if you want, but your comment made me wonder about the English) [22:19:23] I'm sorry for getting us into that rabbit hole ::S [22:20:54] mooeypoo, I'm not sure on the term for the tense if that's what you mean (divec will) but really what you want is an imperative [22:21:01] So let me check if insertionAnnotations gets cleared reliably when the selection changes [22:21:13] "you must force it to..." [22:21:22] I'm just thinking if you need the force mode at all though [22:21:34] edsanders, ok, I thought the convention was "is"+Passive Verb for boolean stuff, but again, it's minor. [22:22:00] edsanders, the only reason I had it was to not get rid of the condition in the "isCurrentDimensionsValid" method [22:22:06] that would be if you are describing the existing state [22:22:23] divec: Yup, I see now that insertionAnnotations is recomputed every time the selection changes, so we're safe [22:22:39] we might not need the force at all, if we remove the first if condition in isCurrentDimensionsValid .. otherwise, we have to force the system to recheck even if this.valid is already set [22:22:44] mooeypoo, wouldn't it be better to just nullify this.valid every time you change one of the enforced properties [22:22:50] like we do with dimensions [22:23:04] edsanders, well, I want to recheck the status even without thouching the values [22:23:28] divec: So, with that side track done (sorry), I'm fairly convinced that just adding && selectionIsExclusivelyContent to line 1574 will be sufficient [22:23:35] how would the status change if you don't touch the values? [22:23:49] the point is this.valid is a cache [22:23:52] divec: Plus cleanup of the surfaceObserver interaction at the bottom of the function but you know much more about that than I do [22:24:00] edsanders, the entire point was this: I have a "thumb" image whose max dimensions are, say, 400x300. I put into the size widget '500' <-- it's now invalid. I don't touch the size now, I just switch to "Frameless". Now the size is valid even though I didn't touch it, so I force a revalidation. [22:24:03] edsanders, does this make sense? [22:24:08] as long as you invalidate it properly you shouldn't need a force mode [22:24:15] The conditions change. [22:24:22] we limit max size on some images but not on all [22:24:23] so just invalidate the cache when you call setEnforcedMax [22:24:27] and we allow fo rthose image types to change [22:24:29] RoanKattouw: just to clarify, what does this bit of 1635-6 do? '|| new ve.dm.AnnotationSet( documentModel.getStore() ); [22:24:42] edsanders, ok.. ican do that.. though to be honest, I don't quite see the difference [22:25:02] just organization? [22:25:17] divec: 1635 of what file? [22:25:23] it's consistent with the current approach [22:25:34] Oh I see it [22:25:45] divec: That new ... () statement creates an empty set [22:26:01] divec: And... it seems obsolete [22:26:15] Because .getInsertionAnnotations() return this.insertionAnnotations.clone() , so that'll definitely be a set [22:26:20] mooeypoo, some other use in the future my decide to call setEnforcedMax, and not know that that could invalidate the this.valid cache [22:26:32] *user [22:26:34] edsanders, good point. [22:26:45] k, I'm changing it up. [22:27:00] divec: It looks like there was a time where surfaceModel.insertionAnnotations could be null and getInsertionAnnotations() could return null, but this has now been sanitized such that setInsertionAnnotations( null ) just sets to an empty set [22:27:25] divec: So that || new .. () is completely unnecessary [22:27:35] (03CR) 10Mooeypoo: Allow dynamic limit to maxDimensions (031 comment) [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/115306 (owner: 10Mooeypoo) [22:28:45] RoanKattouw: just to be completely clear about an earlier point you made ... [22:29:38] Currently, 1635-getInsertionAnnotations will return the thing that we passed into 1632-setInsertionAnnotations a moment earlier [22:30:23] Yes [22:30:38] If the latter was run [22:30:39] mooeypoo, while we're there we might as well add setEnforcedMin for symmetry [22:30:41] Because it's in a conditional [22:30:46] * mooeypoo nods [22:31:05] edsanders, https://gerrit.wikimedia.org/r/#/c/115306/1/modules/ve/ve.Scalable.js <-- am I misunderstanding something with the last comment? [22:31:12] But if at 1621 we make selectionIsExclusivelyContent as part of the condition, then we'll have to be careful that we do actually setInsertionAnnotations. [22:31:45] So we probably want an inner if ( selectionIsExclusivelyContent ) { ... } around the change+clear only. [22:31:51] mooeypoo, true && foo === foo [22:31:53] Ahm [22:32:00] divec: What case are you afraid of? [22:32:13] Oh, yeah I see [22:32:15] You're right [22:32:25] insertionAnnotations needs to be computed correctly no matter what the selection is [22:32:27] edsanders, not according to chrome debugger :\ But I will retest.. it may have been something I checked wrong. [22:32:42] > true && 123 [22:32:55] where's that ecmabot? [22:33:06] >> no? [22:33:11] >> true && 123 [22:33:13] mooeypoo: You could indeed say that "is forced" is passive voice in English [22:33:24] ecmabot is down [22:33:27] aw [22:33:30] Probably due to labs problems [22:33:35] And/or lack of a Timo to restart it [22:33:35] >>> true && 123 [22:33:46] anyway - try that, you'll get 123 [22:33:47] edsanders: Should be 123 I believe [22:33:53] mooeypoo: passive-voiced stative verbs are pretty adjective-y really [22:34:03] edsanders, ok, I may have mixed something up in there, it showed boolean when I tested, but it was before I made a couple of other changes, so it might have been my tests. [22:34:08] so min = isMin && getMinDimensions [22:34:12] will be false or an object [22:34:33] edsanders, gotcha. Didn't know that. I kept seeing 'false' as value there. I'll take that off. [22:34:41] np [22:35:19] RoanKattouw: That's great: thanks! So selectionIsExclusivelyContent ... [22:35:54] I want to test that in the CE if possible, I think [22:36:12] edsanders, just out of curiosity, is it the same as saying min = getMinDimensions || false; ? [22:36:31] wait, no, you're checking isMin in there [22:36:35] forget that. [22:36:35] right [22:40:19] RoanKattouw: is there a CE-only way of doing isStructuralOffset? [22:40:44] divec: So, I think we should test that in the model, and I think there's a utility for it hiding somewhere [22:41:02] Why do you want to do it CE-only? [22:42:22] ve.dm.ElementLinearData.prototype.isContentData [22:43:55] RoanKattouw: if I can do it CE-only, then it moves towards being able to do several key events without syncing the model [22:44:13] s/key events/keyevents/ [22:44:26] Right [22:45:06] I mean, I can leave that for another day/patch if expedient [22:45:17] Ahm [22:45:17] Maybe [22:45:21] Well, so, let's see [22:45:28] What you could do [22:45:41] Is do an offset lookup in the CE tree for the start and end offsets [22:46:06] Then see if you end up with the same (reference-identical) CBN [22:46:38] (There is one function that does a leaf node lookup and another that does the if ( node.isContent() ) { node = node.getParent(); } thing for you, and I always forget which is which, read the code to be sure) [22:47:14] ve.ce.Document.prototype.getNodeFromOffset is one of those two and ve.ce.DocumentNode.prototype.getNodeFromOffset is the other [22:47:55] Great. I think that's enough for me to work with, then. Thanks! [22:48:10] OK, one last question [22:48:16] How ready is https://gerrit.wikimedia.org/r/#/c/109693 ? [22:48:20] I'm halfway through reviewing it [22:50:10] Errm, well I'm slightly hesitant to say it's ready, because a lot of time passed in the middle of getting it working in different cases [22:50:24] Is there a big obvious problem? [22:50:31] divec: :-) [22:51:27] (03PS2) 10Mooeypoo: Allow dynamic limit to maxDimensions [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/115306 [22:51:36] :-) [22:53:04] (03PS2) 10Mooeypoo: [wip] Limit thumbnail dimensions in media edit dialog [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/115307 [22:53:05] divec: No, it looks sane enough. Commenting on some of the TODOs now [22:53:52] edsanders, I corrected per your CR [22:53:57] * mooeypoo runs to class now [22:54:00] edsanders, feel better! [22:54:02] cya guys later [22:56:03] Heh, "unimoo" :-) [22:58:53] (03CR) 10Catrope: [C: 04-1] Fix handleEnter in nodes that don't split (033 comments) [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/109693 (owner: 10Divec) [23:21:16] (03CR) 10Esanders: [C: 04-1] Allow dynamic limit to maxDimensions (032 comments) [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/115306 (owner: 10Mooeypoo) [23:22:17] mooeypoo, cool, looks good, couple of minor things ^ [23:31:04] (03PS6) 10Divec: WIP: Fix handleEnter in nodes that don't split [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/109693 [23:58:04] RoanKattouw_away: Note the 4 entries on https://wikitech.wikimedia.org/wiki/Deployments#Week_of_February_24th