[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 "...

(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

[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]