[00:04:33] 3VisualEditor / 3Editing Tools: VisualEditor: [Regression pre-wmf6] While changing the Image Type to Frame/Basic, it automatically selects the alignment as Left when it is positioned at the right of the page, but it doesn't change position in CE surface until save - 10https://bugzilla.wikimedia.org/65564 (10J... [00:05:03] 3VisualEditor / 3Editing Tools: VisualEditor: [Regression wmf18] Making an image Right, Basic and Border does not include the Right and Border in the wikitext - 10https://bugzilla.wikimedia.org/62852#c5 (10James Forrester) Skipping right (and just using default) is OK for thumb and frame (but not frameless o... [00:09:49] 3VisualEditor / 3Editing Tools: VisualEditor: Firefox throws NS_ERROR_NOT_AVAILABLE when clicking save button - 10https://bugzilla.wikimedia.org/65401 (10James Forrester) [00:10:18] 3VisualEditor / 3Editing Tools: VisualEditor: [Regression pre-wmf6] While changing the Image Type to Frame/Basic, it automatically selects the alignment as Left when it is positioned at the right of the page, but it doesn't change position in CE surface until save - 10https://bugzilla.wikimedia.org/65564 (10J... [00:10:19] 3VisualEditor / 3Editing Tools: VisualEditor: [Regression wmf18] Making an image Right, Basic and Border does not include the Right and Border in the wikitext - 10https://bugzilla.wikimedia.org/62852 (10James Forrester) [00:10:50] 3VisualEditor / 3Editing Tools: VisualEditor: [Regression wmf4] There is an extra space appearing in media search result dialog (scroll bar displacing last column of results) - 10https://bugzilla.wikimedia.org/64911 (10James Forrester) a:5Trevor Parscal>3None [00:29:22] 3VisualEditor / 3Editing Tools: VisualEditor: Cannot close Media Settings dialog with empty caption field and the TypeError: TypeError: node.getCaptionNode is not a function appears - 10https://bugzilla.wikimedia.org/65568 (10ryasmeen) 3NEW p:3Unprio s:3normal a:3None Created attachment 15449 --> h... [00:30:06] 3VisualEditor / 3Editing Tools: VisualEditor: Cannot close Media Settings dialog, empty caption field and the TypeError: TypeError: node.getCaptionNode is not a function appears - 10https://bugzilla.wikimedia.org/65568 (10ryasmeen) a:3Moriel Schottlender [00:30:16] * James_F sighs. :-( [00:34:19] 3VisualEditor / 3Editing Tools: VisualEditor: Cannot close Media Settings dialog, empty caption field and the TypeError: TypeError: node.getCaptionNode is not a function appears - 10https://bugzilla.wikimedia.org/65568#c1 (10ryasmeen) Example page 1: The first image on: http://en.wikipedia.beta.wmflabs.org/w... [00:44:19] 3VisualEditor / 3Editing Tools: VisualEditor: [Regression pre-wmf6] Cannot close Media Settings dialog, empty caption field and the TypeError: TypeError: node.getCaptionNode is not a function appears - 10https://bugzilla.wikimedia.org/65568 (10James Forrester) 5NEW>3ASS p:5Unprio>3High [02:07:48] 3VisualEditor / 3MediaWiki integration: VisualEditor: [Regression] Switching to edit source mode throws TypeError: Cannot read property 'css' of null and TypeError: Cannot read property 'each' of null for a specific case - 10https://bugzilla.wikimedia.org/65557#c2 (10Roan Kattouw) (In reply to Roan Kattouw f... [02:13:47] (03PS1) 10Catrope: Unbind confirm dialog handlers after either of the events fires once [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/134546 (https://bugzilla.wikimedia.org/65557) [02:15:22] 3VisualEditor / 3Technical Debt: VisualEditor: Clean up unbinding of ConfirmDialog events - 10https://bugzilla.wikimedia.org/65571 (10Roan Kattouw) 3NEW p:3Unprio s:3normal a:3None Clean up the mess I made in https://gerrit.wikimedia.org/r/134546 [02:15:27] (03CR) 10Catrope: "Tech debt bug to remind ourselves that this needs to be done more nicely: https://bugzilla.wikimedia.org/show_bug.cgi?id=65571" [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/134546 (https://bugzilla.wikimedia.org/65557) (owner: 10Catrope) [02:33:04] 3VisualEditor / 3Editing Tools: VisualEditor: [Regression pre-wmf6] Cannot close Media Settings dialog, empty caption field and the TypeError: TypeError: node.getCaptionNode is not a function appears - 10https://bugzilla.wikimedia.org/65568#c2 (10Roan Kattouw) This happens because imgModel.getImageNodeType()... [03:31:49] 3VisualEditor / 3Editing Tools: VisualEditor: [Regression pre-wmf6] Cannot close Media Settings dialog, empty caption field and the TypeError: TypeError: node.getCaptionNode is not a function appears - 10https://bugzilla.wikimedia.org/65568#c3 (10Moriel Schottlender) Most times, we can't trust the node.getTy... [03:38:34] 3VisualEditor / 3Editing Tools: VisualEditor: [Regression pre-wmf6] Cannot close Media Settings dialog, empty caption field and the TypeError: TypeError: node.getCaptionNode is not a function appears - 10https://bugzilla.wikimedia.org/65568#c4 (10Roan Kattouw) (In reply to Moriel Schottlender from comment #3... [03:54:13] * mooeypoo shoots default alignment [04:02:29] (03PS1) 10Mooeypoo: Fix MWImageModel's getImageNodeType() [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/134554 (https://bugzilla.wikimedia.org/65568) [04:07:00] (03CR) 10Catrope: [C: 032] Fix MWImageModel's getImageNodeType() [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/134554 (https://bugzilla.wikimedia.org/65568) (owner: 10Mooeypoo) [04:07:27] aaand I just found a secondary bug. [04:07:47] which makes no sense. [04:08:09] What bug? [04:08:31] (03Merged) 10jenkins-bot: Fix MWImageModel's getImageNodeType() [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/134554 (https://bugzilla.wikimedia.org/65568) (owner: 10Mooeypoo) [04:08:44] newFragment.getSelectedNode() <-- this returns null [04:08:47] even though it shouldn't [04:09:02] newFragment = surfaceModel.getFragment( newNodeRange ); <-- this returns a proper fragment. [04:09:21] What is newNodeRange? [04:09:24] but then newNode = newFragment.getSelectedNode(); returns null [04:09:40] it's the recursive call [04:09:59] after inserting the new node, I go through the insertion to find *just* the image node [04:10:11] the 'newNodeRange' is the range of that node without whatever's around it [04:10:23] That was crucial for the caption insertion [04:10:25] I see the code now [04:10:30] You're searching for the node twice [04:10:34] wth, this worked before. [04:10:38] hm? [04:10:38] You could just do newNode = coveredNodes[i].node; [04:10:53] newNodeRange is set to nodeOuterRange so this should work fine [04:11:12] I want to also set it as the fragment *and* select it, which is why I set it up that way [04:11:15] Unless coveredNodes does not contain any image nodes and newNodeRange is never set [04:11:21] for also adding in the surfaceModel [04:11:29] I understand that [04:11:31] if that's the case it's worse.. it should [04:11:40] hm. [04:11:46] I'm saying you don't need .getSelectedNode() because you already have an opportunity to grab the node earlier [04:11:48] You even check if its type is image-y [04:11:50] However [04:11:58] hmm [04:12:06] If you're getting null from getSelectedNode() , there's either something wrong with that function or something wrong with your range [04:12:46] oh I see what you're saying, ha, get that inside the loop. hah. [04:12:53] Can you dump the range and the data in that range (ve.instances[0].model.documentModel.data.data.slice( start, end ) ) to se what's going on? [04:13:06] yeah let me see [04:13:34] The internals of getSelectedNode() are almost identical to your coveredNodes loop [04:13:46] this hsould work.. it DID work [04:13:50] I'm pretty sure it's mathematically provable that the return value of getSelectedNode() will equal coveredNodes[i] [04:14:05] I added it into the loop now, as you suggested [04:14:11] Which is why I'd really like to know why you got null [04:14:53] wtf [04:15:28] coveredNodes.length = 1 which means it should get into the for loop once [04:15:30] but it doesn't [04:16:06] Does it run more than once or not at all? [04:16:35] once, and skips the condition, because its coveredNodes[0] is a paragraph [04:16:40] mrrr [04:16:44] A paragraph? :O [04:16:53] I know! [04:16:57] Oooooh [04:16:58] what in the name... [04:17:02] I get it [04:17:02] I just ADDED an image. [04:17:05] oh..? [04:17:08] You're inserting an inline image [04:17:12] In a block slug [04:17:14] Most likely [04:17:21] actually, no [04:17:27] I'm transforming an inline image into a block [04:17:32] basic -> thumb [04:17:35] Hah really? [04:17:48] I wanted to test all default-aligned stuff [04:18:02] inline to block, not block to inline? [04:18:09] inline to block [04:18:21] Can you dump contentToInsert to check? [04:18:25] The point of the last part is to add the caption [04:18:40] yep [04:18:57] Also dump fragment.getRange() in between the .insertContent() and .getCoveredNodes() calls [04:19:07] (This requires splitting that statement up because those calls are chained) [04:19:51] yup it's adding mwBlockImage [04:20:17] how do I copy the content of the watch expression in chrome? [04:20:21] when it's an array... [04:20:45] Don't worry [04:20:50] Just tell me what it looks like [04:21:07] ok so contentToInsert has 4 items [04:21:19] Open image, open caption, close caption, close image? [04:21:29] as expected, mwBlockImage (with attributes), mwImageCaption, /mwImageCaption, /mwBlockImage [04:21:31] aye [04:21:41] This *WORKED* like... this morning. [04:21:59] This sounds good so far [04:22:21] So, after the insertContent() call but before the getCoveredNodes() call: [04:22:26] * What is fragment.getRange() ? [04:22:30] coveredNodes again only has 1 item [04:22:31] and it's a paragraph [04:22:34] OK [04:22:48] oh, hm, hang on [04:22:58] This can happen after the getCoveredNodes() call too, BTW [04:23:02] So you can dump it right now [04:23:24] Also, look at fragment.getSurface().getDocument().completeHistory and find the transaction that inserted this image [04:23:29] and see if it inserted anything else [04:26:04] my range is 20,20 [04:26:10] which should be perfect [04:26:40] Hold on [04:26:44] It's a zero-length range? :O [04:27:05] Ooh [04:27:16] Was your inline image at the very start/end of a paragraph [04:27:17] ? [04:28:10] yes it was in its own paragraph [04:28:15] Aha [04:28:18] but i'm also calling collapseRangeToEnd() [04:28:20] or start [04:28:23] Doesn't matter [04:28:28] OK so here's what's going on [04:28:33] In the normal case, you start with: [04:28:44]

Foo||Bar

[04:28:52] Then you remove the inline image: [04:28:57]

Foo||Bar

[04:29:29] * mooeypoo sees slugs coming in your analysis [04:29:35] Nope, no slugs :) [04:29:40] And then you insert the block image: [04:29:41] Right. Then the insertion breaks apart the paragraph [04:29:59]

Foo|

|Bar

[04:30:03] However [04:30:12] Now if you start with this: [04:30:17]

Foo||

[04:30:26] not even, no text at all [04:30:31] Right, OK [04:30:33] Let's do that one [04:30:39]

[04:30:46] You remove the inline image [04:30:49]

[04:30:54] And you insert the block image [04:31:20] But then fixupInsertion goes "hold on, you're trying to do

{block image}, which is stupid, you should do {block image}

" [04:31:30] (this is the same code that makes the

appear in the other case) [04:31:46] So normally it would add

to your insertion [04:32:00] But in this case, it decides that's stupid because the resulting paragraph would be empty, and so it *moves your insertion point* [04:32:16] bah. [04:32:23] And so you get

[04:32:33] oy. That.. would explain it. [04:32:40] We were counting on the insertion happening inside the fragment's range, so the range would grow and cover the inserted content [04:33:00] But Ed's smartass code moved the insertion point out of there, and so the range gets pushed to the right rather than being grown [04:33:05] and I just tested, btw, it works perfectly on Foo|img|Bar [04:33:23] Shoot. [04:33:31] So.. should I insert a placeholder? [04:33:51] instead of removing the image and re-adding it, maybe I should put a placeholder in, remove the image, add the new image, and remove the placeholder [04:34:05] That sounds dirty [04:34:14] I'd rather find a way to find the inserted data [04:34:19] aye, but that would prevent this lovely truck [04:34:21] err trick [04:34:28] So maybe we should make it less trick-like [04:34:41] I have an idea [04:34:53] what i'm worried about, honestly, is that we already had a difficult time getting the proper range for inserting the caption [04:35:04] Yeah that's this same thing [04:35:17] The whole reason for that with the covered nodes stuff was because I was trying to avoid doing it propertly [04:35:20] *properly [04:35:25] Because I thought that would be difficult [04:35:30] But I guess we'll have to [04:35:35] Oh, that's not the proper way? [04:35:38] Is there another? [04:35:39] Well [04:35:46] There is if I build it :) [04:35:51] I thought that's the best way to get one node out of many covered ones [04:35:55] Yes [04:36:01] But, look at it this way [04:36:10] Oh, hold on [04:36:12] Sorry [04:36:15] But again we're getting into the problem of this not being covered.. [04:36:18] We'll still need coveredNodes, you're right [04:36:27] as in, my worry is that in some cases the node's covered and in some it isn't. [04:36:28] But we need a way of reliably getting the range of the inserted content [04:36:38] RoanKattouw, no no but that won't help us [04:36:45] coveredNodes works for nodes inside the range [04:36:50] the problem here is that the range MOVED [04:36:52] That's the thing [04:36:55] We need to get the right range [04:36:56] it doesn't include the nodes anymore :\ [04:37:05] And I think I know of a way to do that [04:37:08] oh [04:37:11] We'd change the definition of insertContent() [04:37:23] ooh? [04:37:27] Currently, insertContent() doesn't do anything special to the range, it just lets it be translated according to the transaction [04:37:41] Which results in the expand-or-move-depending-on-what-fixupInsertion-felt-like behavior [04:37:57] Instead, we could change it, in one of two ways [04:38:02] hmm [04:38:10] Which one is better I'm not sure yet [04:38:33] One possibility is to have it always set the range to be the stuff that was inserted [04:38:49] So that would fix the case where the insertion point was moved [04:38:52] I thought that was the most logical one, I thought that's actually what happens [04:39:06] But it would still include things like

that were added to make the data balanced [04:39:17] Yes, this is actually what happens right now, except in the moving-insertion-point case [04:39:24] yeah but then you can fix them by searching through actual coveredNodes [04:39:31] Exactly [04:39:41] when the insertion-point-moved happens, you can't. [04:39:45] So we'd fix the insertion point problem but still need covered nodes [04:39:48] Call this approach #1 [04:39:51] * mooeypoo nods [04:40:03] Approach #2 could be to have it only select the content /you told it/ to insert [04:40:13] So it would be like #1 but excluding things like

[04:40:19] so, basically, do the 'coverednode' check for me [04:40:32] Pretty much, yeah [04:40:34] I personally think that #2 makes more sense [04:40:38] You could do an offset lookup instead [04:40:55] intuitively, you would expect the range to change according to what you inserted [04:41:04] BUT... I am not sure if there might be cases where you want the full surrounding range. [04:42:05] Generally I don't think there are [04:42:19] But what I'm worried about is if there are cases where things get stripped from the insertion data [04:42:56] Like, if you're trying to do

Foo{

Bar

}Baz

, what happens? [04:43:04] Reading the test cases suggests that'll split the paragraph [04:43:27] But I'd have to analyze the code to prove there is no case where things are removed from the insertion [04:43:40] I think I'm gonna go with #1 at least for now because it's easier to see how it would be implemented [04:44:19] We'd add a method in ve.dm.Transaction so SurfaceFragment#insertContent could do tx = newFromInsertion( ... ); newRange = tx.getInsertionRange(); [04:44:35] hmm [04:44:59] The implementation of getInsertionRange() (or getModifiedRange if we want to be generic) would be easy [04:45:30] so we can choose [04:45:37] either use that one or the other. That sounds good. [04:45:49] For the image model, I could just use the insertionRange always [04:45:58] oi, my goulash is boiling, brb real quick [04:46:05] Yeah I guess in theory we could choose [04:46:18] But I would want insertContent() to just always use the insertion range [04:46:28] Rather than using an empty range in seemingly random cases [04:46:57] #2, on the other hand, would be more difficult [04:48:38] We'd have to have fixupInsertion() return the range corresponding to the original data along with the fixed-up data (it already returns an object with new data, insertion offset, and number of things to remove, so that would be doable) [04:48:52] We'd then have to propagate that data from there into the Transaction object and then to SurfaceFragment [04:49:42] It's a bit icky because there's no good reason why the Transaction object would know this, the presence of the information is specific to what code built the tx, whereas with #1 it's information that's inherent in the tx [04:49:50] back, sorry, all good. [04:50:04] OTOH, with cscott_away 's evil plans for transaction intents, it might not be a crazy idea [04:50:38] intent? like in android? [04:50:52] I forget if he calls it intents or intentions [04:51:12] well, in Android it's kinda like transaction [04:51:37] Intentions, apparently [04:51:51] Which AIUI are descriptions of what you'd like the transaction to do in principle [04:51:54] It carries data around, more or less. But yeah. [04:51:58] Kind of like arguments passed to a tx builder [04:52:09] I'll have to talk to him about this while he's still in the office [04:52:20] It could be a nice way to facilitate #2 [04:52:23] But for now let's do #1 [04:52:27] * mooeypoo nods [04:52:46] not entirely sure how, though, when the transaction splits the paragraph apart [04:53:23] ? [04:53:33] I mean with #1 we'd still need the coveredNodes stuff [04:53:40] But we'd solve the insertion point moving issue [04:54:19] I mean I'm not sure how to solve the insertion point moving [04:54:38] if the insertion point DOESN'T move, my code should work as is in the MWImageModel [04:54:40] We would ask the transaction object "from where to where did you actually change things" [04:54:57] Then insertContent() would set that as the new range, rather than letting offset translation just happen [04:59:13] hmm [04:59:25] that makes sense. [05:00:19] btw, did you see my response regarding using the model's version of 'get node type' ? [05:00:28] it'll be tricky, but the only real way to do it is make sure the mode's not wrong [05:00:48] (and I didn't make stupid typos, but that's a natural assumption from the previous assertion) [05:03:09] I saw a commit :) [05:03:26] And a response on BZ before that, which seemed to describe the commit more or less [05:04:04] If you're talking about https://bugzilla.wikimedia.org/show_bug.cgi?id=65568#c3 , I replied [05:04:55] Whoa wtf [05:04:57] * This will move the fragment's range to the end of the insertion and make it zero-length. [05:04:59] In the docs for insertContent() [05:05:02] Well that's a big fat lie [05:08:26] RoanKattouw, well, it's not a complete lie, not always [05:08:34] haha true [05:12:53] 3VisualEditor / 3Editing Tools: VisualEditor: [Regression pre-wmf6] Cannot close Media Settings dialog, empty caption field and the TypeError: TypeError: node.getCaptionNode is not a function appears - 10https://bugzilla.wikimedia.org/65568#c7 (10Moriel Schottlender) (In reply to Roan Kattouw from comment #4... [05:17:11] (03PS1) 10Catrope: [WIP] Always cover inserted content in SurfaceFragment#insertContent [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/134559 [05:17:45] mooeypoo: --^^ [05:17:51] Play around with that [05:18:07] It should make insertContent set the range correctly [05:18:21] I didn't test it thogh [05:18:29] (03CR) 10jenkins-bot: [V: 04-1] [WIP] Always cover inserted content in SurfaceFragment#insertContent [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/134559 (owner: 10Catrope) [05:19:40] clearly :P [05:20:13] 05:18:28 >> TypeError: 'null' is not an object (evaluating 'node.canHaveChildren') [05:20:14] wtf [05:21:46] lolol [05:21:48] offset=NaN [05:22:18] getModifiedRange returned new ve.Range( 0, NaN ) [05:22:20] That'll go well [05:24:22] Oh, hahaha [05:24:29] It helps if you initialize accumulator vars to zero [05:25:03] (03PS2) 10Catrope: [WIP] Always cover inserted content in SurfaceFragment#insertContent [VisualEditor/VisualEditor] - 10https://gerrit.wikimedia.org/r/134559 [05:25:07] OK there [05:25:11] hah [05:25:17] That actually passes the existing tests on my machine [05:26:25] i'm testing now on the node that failed [05:26:35] the mwImageModel should work without any tweaks, right? If I understood what you did correctly [05:26:48] it gets the full range, and then looks for the right node anyways inside the Image Model [05:27:09] Yeah exactly [05:27:15] It should be fully compatible with your existing code [05:27:21] Applying this should make your code just start working [05:27:24] (the best kind of patch) [05:27:44] indeed [05:27:45] let's see [05:29:09] ok so inspecting everything, the fragment range is still 20/20 (flat) [05:29:11] BUT it works [05:29:49] Ahm, wat [05:29:56] The fragment range shouldn't be 20,20 [05:30:07] Not right after the insertContent() call anyway [05:30:07] coveredNodes again have only 1 item too, and it's still paragraph [05:30:11] and yet... it.. works [05:30:12] this is weird [05:30:15] let me clear cache [05:33:55] RoanKattouw, ok this is really weird [05:33:58] i'm still getting the same thing [05:34:04] and newNodeRange is null [05:34:44] Ahm, wat? [05:34:45] this: fragment.collapseRangeToEnd().insertContent( contentToInsert ).getRange(); [05:34:50] is 'flat'.. 61/61 [05:34:56] wtf [05:34:58] (using another node further down the page) [05:35:13] let me try refreshing the cache again, I am having occasional flukes with it. [05:37:19] WM [05:37:21] *M [05:37:23] *WFM [05:37:25] Come on keyboard [05:37:38] Page with just [[File:VisualEditor-logo.svg]] [05:37:41] change type to thumbnail [05:37:48] Without my fix, there's a JS error [05:37:51] With my fix, it works [05:38:27] [[File:Logo mo.jpg|border|frameless]] [05:38:30] to thumb [05:38:49] fail for me. I'm adding a double-check in the model in any case as fallback, but this makes no sense [05:40:00] yeah I cleared cache twice [05:40:09] :\ TypeError: newNode is undefined [05:40:34] hmm yeah even in my test case I get a zero-length range, that's weird [05:41:06] Ooooh [05:41:09] It's because my code is stupid [05:41:17] Is your image also the very first thing on the page? [05:41:33] No, but there's only a heading before it [05:41:42] Hmm [05:41:46] == frameless == [image] [05:42:04] Hmm and getModifiedRange() returns the right result [05:42:08] And still the range is wrong, wtf [05:42:14] ok hang on [05:42:16] now it works [05:42:25] so, the range is wrong, but it works [05:42:31] I removed all my "corrections" [05:43:00] Yeah mysteriously it works for me too [05:43:00] makes no sense, really.. my 'correction' was to move the newNode = ... to inside the loop with coveredNodes[i].node [05:43:11] Probably because the range accidentally ends up in a fortuitous place or something [05:43:13] I removed that (so I'm using the clean code from master) and it works [05:43:40] I guess it makes sense if the range is still quirky, but then if it's still quirky why does it work NOW and not before [05:44:03] This: [05:44:03] newFragment = surfaceModel.getFragment( newNodeRange ); [05:44:04] newFragment.select(); [05:44:04] newNode = newFragment.getSelectedNode(); [05:44:08] seems to select the node properly [05:44:09] That's the thing, it *shouldn't* work [05:44:14] even though the newFragment range is wrong [05:44:16] how weird is that [05:44:45] Unless getCoveredNode messes with the range or something [05:44:47] wtf [05:44:51] Isn't that the best type of bug? "It works, but i have no idea why!" [05:45:19] oooooooooooh [05:45:29] Guess what collapseRangeToEnd() does [05:45:31] It *clones* the range [05:45:42] oh, I forgot that's there... hah! [05:45:58] That's why our debugging is showing wrong data [05:46:08] The actual fragment that's being operated on does have the correct range [05:46:15] Also, moving the newNode assignment into the for loop should totally work [05:46:19] wait, it clones the range, but we're checking the range for the fragment [05:46:31] why is it showing us not the one that it's actually acting on? [05:46:41] RoanKattouw, it would if the range is right [05:46:44] it didn't when the range is flat [05:46:49] Sorry [05:46:54] It clones the fragment [05:46:58] And the range with it [05:47:10] So when you do fragment.getRange() [05:47:35] You're getting the range of the old fragment before collapseRangeToEnd() was called, not the new one that .insertContent() actually operated on [05:47:54] ahha [05:47:57] And because insertContent()'s new behavior differs from standard range translation in this case, the two fragments are now different [05:47:59] wait, no, the new fragment [05:48:06] the new range is collapsed, which is what we got [05:48:09] the wrong one. [05:48:32] k, success! [05:48:36] I also added a fallback [05:51:19] (03PS1) 10Mooeypoo: Adjust newNode range and fallback in MWImageModel [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/134563 [05:51:23] RoanKattouw, followup to yours ^^ [05:52:31] mooeypoo: Not actually a dependency [05:52:44] well the code breaks without yours [05:52:46] Your change works in isolation [05:52:55] It only breaks in the exact same cases as it broke before though [05:52:57] but it breaks without yours before my fix too [05:53:01] right [05:53:08] I guess it's not a dependency, it's a followup [05:53:36] I thought it would be wise to relate them, though. Should I change to 'followup' ? related-change? [05:53:43] "Also see #" ? :P [05:54:49] RoanKattouw, actually, what *is* the proper way to define it in commit messages? [05:55:01] similar to jsduck? [05:55:17] We could relate them but they're not really related [05:55:28] They're only related in that we made the changes while investigating the same issue [05:55:54] Ah. I guess so. [05:56:22] They're conceptually related as in they conceptually fix a related bug (with fallbacks and reading the proper range) but that's just quibbling over detail :p i'll remove it [05:56:40] (03PS2) 10Mooeypoo: Adjust newNode range and fallback in MWImageModel [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/134563 [05:57:31] (03CR) 10Catrope: [C: 032] Adjust newNode range and fallback in MWImageModel [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/134563 (owner: 10Mooeypoo) [05:59:01] (03Merged) 10jenkins-bot: Adjust newNode range and fallback in MWImageModel [extensions/VisualEditor] - 10https://gerrit.wikimedia.org/r/134563 (owner: 10Mooeypoo) [06:00:29] woot woot [06:00:56] oh, uh, RoanKattouw I don't mind +2ing yours now that we tested it works, but it's WIP [06:01:48] oh you're creating unit tests. Forgot about those. [06:01:58] * mooeypoo pretends she didn't just say that [06:02:24] I'm going to create them tomorrow [06:02:28] After I get some sleep [06:02:33] yeah you should get to sleep [06:02:36] and me too [06:02:53] going to check on what other bugs were reported, then head off. [06:02:58] thanks for the help!! [06:07:33] 3VisualEditor / 3Editing Tools: VisualEditor: [Regression pre-wmf6] While changing the Image Type to Frame/Basic, it automatically selects the alignment as Left when it is positioned at the right of the page, but it doesn't change position in CE surface until save - 10https://bugzilla.wikimedia.org/65564#c2 (... [06:07:58] yay, RoanKattouw the earlier bug fix fixed also this bug (at least it seems to) [06:08:14]