search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] [Live HTML] New diff format #4319

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by dangoor Tuesday Aug 06, 2013 at 20:34 GMT Originally opened as https://github.com/adobe/brackets/pull/4675

Putting this up for comments and to share the implementation with@jasonsanjose who is doing the browser-side work.

dangoor included the following code: https://github.com/adobe/brackets/pull/4675/commits

core-ai-bot commented 3 years ago

Comment by njx Tuesday Aug 06, 2013 at 20:55 GMT

Drive-by comment...the calculation and meaning of these positions is just complex enough that I think it would be valuable to have some comments explaining how it works (what the positions mean for text and non-text edits, what the assumptions/guarantees are in terms of the order of edits and how later edits can/can't refer to IDs from previous edits, and an explanation of some of the details of how the neighbors are found).

core-ai-bot commented 3 years ago

Comment by jasonsanjose Tuesday Aug 06, 2013 at 21:35 GMT

Oops. I merged my branch here that include commits from another open pull request #4659.

core-ai-bot commented 3 years ago

Comment by dangoor Wednesday Aug 07, 2013 at 00:46 GMT

@njx yes, totally agree about the commenting. I usually do that in a pass at the end once I've got things working (sometimes catching other little things during that pass that don't leap out while working on the details of getting it functioning properly).

@jasonsanjose No problem about the extra commits. I'll figure it out.

core-ai-bot commented 3 years ago

Comment by dangoor Wednesday Aug 07, 2013 at 02:46 GMT

I just committed a change that adds the new firstChild/lastChild/afterID/beforeID support. For some reason that is not immediately clear to me right now, the incremental edit is failing to parse in the lastChild case.

core-ai-bot commented 3 years ago

Comment by dangoor Wednesday Aug 07, 2013 at 15:37 GMT

I just pushed some code cleanup. All of the tests are passing. I have a list of more tests to write, but I think this does it for the basic approach to position info.

core-ai-bot commented 3 years ago

Comment by dangoor Wednesday Aug 07, 2013 at 15:42 GMT

Oh yeah, one other note: addPositionToEdit may not fully ensure that afterID is only present as needed. I'm hoping to shake that out as test cases are added (it may be fine, but I can't be sure).

The other thing I meant to mention is that basic text replacing isn't working any more when live development is turned on. There's probably still something more to update in there.

core-ai-bot commented 3 years ago

Comment by jasonsanjose Wednesday Aug 07, 2013 at 15:56 GMT

I'll have Internet at home by 4pm. I can look into this then if you can't get to it.

core-ai-bot commented 3 years ago

Comment by dangoor Wednesday Aug 07, 2013 at 16:19 GMT

Oh yeah, I forgot that you're off today. We'll take care of it.

core-ai-bot commented 3 years ago

Comment by njx Wednesday Aug 07, 2013 at 18:07 GMT

I'm starting to investigate the issue on the browser side now.

core-ai-bot commented 3 years ago

Comment by dangoor Wednesday Aug 07, 2013 at 20:35 GMT

I just pushed some commits that reduce the size of wellformed.html and adds a new test case. That case is failing on the incremental test. It may have something to do with how I'm setting up the scenario (I do a replaceRange outside of the call to doFullAndIncrementalTest).

In a separate branch I had started making changes to handle the merging of text nodes. Also on my list to test:

Feel free to work on whichever cases make sense. I'll be online later...

core-ai-bot commented 3 years ago

Comment by njx Wednesday Aug 07, 2013 at 21:22 GMT

I'm looking at the failure in the incremental test. It looks like we're getting the wrong mark range for the incremental edit--it thinks the changed range is the <em> tag, and doesn't see the <strong> tag. There may be some (literal) edge cases for edits that are at the edge of a marked range, although that seems to work in other cases. I'll investigate.

core-ai-bot commented 3 years ago

Comment by njx Wednesday Aug 07, 2013 at 21:32 GMT

Ah. Looks like that bug has always been there, but it's not as obvious if you're typing as opposed to pasting a whole range at once. Probably we should be conservative, and if the edit looks like it's at the beginning or end of a tag range, treat the parent as the incremental update range instead of the tag itself.

core-ai-bot commented 3 years ago

Comment by njx Wednesday Aug 07, 2013 at 22:04 GMT

Pushed a commit that fixes the incremental edit bug.@dangoor, your new test is passing now.

core-ai-bot commented 3 years ago

Comment by njx Thursday Aug 08, 2013 at 00:02 GMT

I fixed up a couple of my earlier non-working tests and pushed them. I also pushed a test for inserting an element in the middle of text. It worked and did the right thing in the browser, but the set of edits wasn't actually what I expected. The case was adding <img> in the middle of <h1>GETTING STARTED| WITH BRACKETS</h1>. I would have expected the edits to be:

delete text "GETTING STARTED WITH BRACKETS"
insert text "GETTING STARTED" (first child)
insert element <img> (last child) 
insert text " WITH BRACKETS" (last child)

But what it generated was:

insert element <img>
insert text " WITH BRACKETS" (after <img>)
replace text "GETTING STARTED" (before <img>)

In particular, the first insert has no position specifier other than parentID (it's not marked as firstChild or lastChild and has no beforeID or afterID). It turns out this works okay with the browser code, because in that case the browser code just does appendChild(). However, I'm not sure if that's actually the intended way for that edit to come out.

core-ai-bot commented 3 years ago

Comment by njx Thursday Aug 08, 2013 at 01:20 GMT

I've added yet more tests in a new branch, nj/incr-typing-tests, intended to simulate actual typing (character at a time), which is more realistic than most of our existing unit tests that test inserting an entire tag at a time. In the course of working on these tests, I discovered a very bad bug in the "implied close tag" processing, which is fixed in that branch. However, that fix breaks the first of the incr-typing-tests in a way that doesn't make any sense (it shouldn't affect that test since that test never has a state where the <p> is inserted without the </p>--but it ends up making it so the edit list doesn't actually include the tag insertion).

I need to go offline for the day, but will pick this up again tomorrow.@dangoor, you might want to take a quick look at that branch to see if anything obvious pops out at you. (The parser bug and fix should be pretty obvious; why it breaks the new tests isn't clear.)

core-ai-bot commented 3 years ago

Comment by dangoor Thursday Aug 08, 2013 at 01:37 GMT

I can understand how the "insert element in the middle of text" case would end up with that set of changes, but I agree that it seems a bit off. I'm kind of okay with that as a special case that works that way. The only thing that concerns me is that the production of that diff is tied to the way text is managed (via the synthetic ID that is created).

Maybe I'll put a note about this near the function that creates the text IDs, because we would almost certainly change that function if we're doing something about the way text is managed. There should also be a comment for the function that gets the edit position.

I'll take a look at your changes on the robust-diffs branch and merge them if all seems well. Then, I'll take a look at your incr-typing-tests branch.