nodes mapping

[atulbhatt-system32]

No description provided.

[edenvidal]

@atulbhatt-system32 how do we resolve it?

[atulbhatt-system32]

@edenvidal
We need to give a good amount of time on this issue to find ways to find or build that missing information from whatever information we have.

[edenvidal]

@atulbhatt-system32 can you explain it further please? 🙏 what is it, and why is it a burden? to what?

[VictoriaShyika]

@edenvidal, @atulbhatt-system32
During HTML parsing, a tree is created and each element is given an ID. The main problem is that the identifiers of each element can change after each rendering, because the parser creates a new tree each time when something change and the identifiers of previously expanded nodes may not match the identifiers of nodes from the new tree, so unexpected collapsing/expanding of nodes is possible.
Because previously the identifier corresponded to the node that we expanded, but after several re-renders, the identifier may change and correspond to a completely different element, so unexpected behavior is possible.

[edenvidal]

so, the issue is - inconsistent element ids during html parsing and rendering, right? @atulbhatt-system32 @VictoriaShyika

lmk if i get it - when html is parsed, each element gets an id. these ids can change with each new render when changes happen, leading to issues like unexpected collapsing or expanding of elements. if an id changes after several re-renders, it might refer to a different element.

we need to look into:

1. can we keep ids the same for elements that haven’t changed but moved?
2. ideally, ids should stay the same to avoid problems. keeping ids stable is important for a predictable document structure. can we do that?
3. how can we limit id updates to only necessary elements?
4. what ways can we track elements with changed ids to ensure they are referenced correctly after re-renders?

last question - is this current behavior already enforces us to compromise in the past? or to write redundant code that covers it? i'd be happy to know even more about it.
i am certain we can make rendering more stable and predictable by keeping element ids consistent and minimizing unnecessary id changes. it is something that should just work in the background and resolve on a deeper level. it might be related to this or this.

[edenvidal]

@maxdnc, check this out.

[edenvidal]

@atulbhatt-system32 is this relevant?

[atulbhatt-system32]

Yes @edenvidal. As there can be expanded nodes which are not selected so we need to update those too after uids changes.

[atulbhatt-system32]

@vicki29 I think it will tough to solve this. Why not for now we make sure at least the nodes in which move operation has happened after drag and drop those remain expanded.

[edenvidal]

@atulbhatt-system32, not exactly. the hovered container expands while dragging. check figma for reference. other than that, i don't see a case where a node should expand or collapse. (maybe im wrong)

[atulbhatt-system32]

@edenvidal
The node in which drop happens also expands only to one level in which drop happens.
The dropped node remains in same state in which they are dropped.

[edenvidal]

@atulbhatt-system32 i think yes, this is how it happens

[edenvidal]

@atulbhatt-system32 how far did @vicki29 took us with this? thanks

[atulbhatt-system32]

There wasn't anything that she was able to do.

[Mrdigs]

Nudge to note I have submitted an upwork proposal but not heard back.

Update: the situation with undo is firmer than described in the proposal as I have a better understanding of how that's handled, and have a solution for it.

[edenvidal]

from @Mrdigs

• I have a generally working solution that makes use of Monaco Decorations to map between editor positions and node uids. The advantage of this approach is that there does not need to be any complex code that listens for edits and adjusts the positions accordingly as the editor takes care of it for us.

• Most of the changes to do this are in the parseHtml() function in useElementsHelper.tsx. Since the new code scans for, adds and removes decorations on the editor model, this makes that function less generic as it can now only be used to parse the contents of the editor rather than any HTML (theoretically - in practice it is only currently used to parse the editor contents).

• There is a 2nd parseHtml() function in _node/file/handlers/handlers.ts. I can't see what the purpose of this other parse function is, but as it does not have a reference to the editor model, it cannot perform the same uid tracking.

• Because parsed nodes re-use their previous uids, newly generated uids need to be truly unique rather than simple increments to prevent collisions. For my test code, I have added the "uuid" package to the project and using that. I'm assuming you'll either accept that or replace it with your preferred generator. But, there are places where they are assumed to be integers and sortable, e.g in copyAndCutNode() in useElementsHelper.tsx - I don't know what the impact of changing the uids are on any of this code, I suspect that my solution negates the need for this code in the 1st place but I'll need that to be confirmed by somebody more familiar with the code.

• Decorations are not preserved on undo, therefore all uids are reset and thus none match, leading to the whole node tree collapsing (because the uids are now random rather than sequenced). This is possibly the biggest issue. It may be possible to maintain some kind of stack of decoration states that can be synced with the undo/redo stack which I'm happy to look at, but if that is not possible then it might be neccessary for undo/redo operations to be implemented in the code rather than relying on the Monaco implementation - which I think would fall outside the scope of this job.

• Decorations are also deleted when editor.ITextModel.setValue() is called understandably as there's no way of knowing where they should be placed when the entire document is overwritten, and this problem would exist even if Decorations were not used to solve the root issue, so calls to that method need to be replaced with calls to editor.ITextModel.pushEditOperations() instead so as to allow the Decorator positions to track the edits. This means all the operations in useElements.tsx that rely on the helperModel returned by getEditorModelWithCurrentCode() and this would have to fall outside the scope of this job because it would really need somebody very familiar with that code, preferably the original author.

• There is one alternative to ditching getEditorModelWithCurrentCode() that I have thought of: it could copy the Decorations as well as the document from the actual codeViewInstanceModel, which can then be written back when all of the edits have been applied. I'm happy to look into that potential solution to the problem.

---

from @atulbhatt-system32
2nd parseHtml() function in _node/file/handlers/handlers.ts, that is indeed old was kept to have older functionality as a backup but can be removed.
"But, there are places where they are assumed to be integers and sortable, e.g in copyAndCutNode() in useElementsHelper.tsx". Yes UUIDs need to be integers as we sort them in some operations. The sorting is required so that we can generate the updated html in case when multiple nodes are there in a single operation. This is required to make sure a single history event is there even when multiple nodes are there.

Overall about how relying on decorations can help us with our problem - A video explaining it will be much better.

---

and here we are... (let's continue here)

[Mrdigs]

Before I can continue with this ticket, I need to confirm that the stakeholders understand the impact of persisting uids across edits has on code reliant on sorting the uids in numerical order:-

Imagine we have a document which contains the following node. I am using "id" attributes here to show the uid of each node. My change also associates the uid with each node within the source code, except they are invisible. The attribute is for demonstration purposes only.

<body id="1">
<p id="2">Para X</p>
</body>

Now, the user inserts another <p> before the original like so:

<body id="1">
<p>Para Y</p>
<p id="2">Para X</p>
</body>

At this stage, the new <p> has no uid as we are now just re-parsing the document. During parsing, the <body> and the 2nd <p> both retain their uids, so a new uid has to be generated only for the new <p>.

As it has to be unique, it cannot be 1 or 2. Let's assume it becomes 3:*

<body id="1">
<p id="3">Para Y</p>
<p id="2">Para X</p>
</body>

Now, if the nodes were to be sorted by uid, and these sorted uids were used to generate the updated html as Atul states, then the two <p> elements would become reversed.

This is the potential problem I am worried about, because before my change "Para Y" would have acquired uid 2 and "Para X" would have acquired uid 3 - and so sorting the uids would retain the insertion order.

_* When uids are generated, the code is traversing the DOM in depth-first order, so there is no way of knowing which uids are already used elsewhere in the document and this is why I wanted to use the "uuid" library.

• However, I see now that the callback passed to the parseHtml() function calls a function setMaxNodeUidRef(). So if the value of the max uuid from the previous call to parseHtml() can be also passed as another argument to parseHtml(), then I can count up from that value. This would mean the uuids can be both numerical and unique. But would not resolve the above problem with the order of the uids in the node tree.

[atulbhatt-system32]

@Mrdigs
Currently the entire document is parsed again, so the uids are always in sequence.Para Y will have uid as 2 and Para X will have uid as 3

But still I'm not sure how the decorations will help with keeping track of which uids should be expanded.

[Mrdigs]

Hi,

I'm afraid my screen recorder isn't recording audio for some reason, so I will have to transcript the video.

1. For this video, I have made the decorations visible, as it's easier to follow. I have also added some hover text to show the UID associated with each decoration.

2. As you can see, there is a decoration at the beginning of each DOM node. If I hover over the decoration for the doctype node, you'll see it has UID 1.

3. The <body> node has a UID of 4. If I edit the title, you will see that the editor moves the decoration for the <body> node as I type.

4. This means that when the updated document is parsed and the DOM traversal in parseHtml() reaches the <body> node, I can query the decoration at the source code location for that node, and it will give me the same decoration with a UID of 4, so that same UID is assigned to that node in the tree.

5. So next I insert a <div> containing a <p> - it has been given a UID of 11. So now if I expand that node in the tree, the expanded nodes data records that node 11 is expanded.

6. Now, if I paste in another <div> ahead of the original, you will see that the tree inserts a new collapsed node above the initial node. This node did not have a decoration at it's source code location when the document was parsed, and so it has been assigned a new UID of 19.

7. The second <div> in the tree remains expanded because the decoration lookup has ensured that this node retains the same UID of 11 as it did before, and the tree knows that node 11 is expanded.

8. Therefore, node 19 now appears in the document ahead of node 11.

Hopefully this explains it for you?

Screencast.2024-07-15.08.33.18.mp4

[atulbhatt-system32]

@Mrdigs For some reason the video doesn't play for me. @edenvidal is it the same for you?

[atulbhatt-system32]

@Mrdigs can you share it from a google drive link. I am unable to play it.

[Mrdigs]

@atulbhatt-system32 Sure, here it is

(weird, I can hear myself typing in it now - so the microphone was working)

[atulbhatt-system32]

Hi,

I'm afraid my screen recorder isn't recording audio for some reason, so I will have to transcript the video.

1. For this video, I have made the decorations visible, as it's easier to follow. I have also added some hover text to show the UID associated with each decoration.
2. As you can see, there is a decoration at the beginning of each DOM node. If I hover over the decoration for the doctype node, you'll see it has UID 1.
3. The <body> node has a UID of 4. If I edit the title, you will see that the editor moves the decoration for the <body> node as I type.
4. This means that when the updated document is parsed and the DOM traversal in parseHtml() reaches the <body> node, I can query the decoration at the source code location for that node, and it will give me the same decoration with a UID of 4, so that same UID is assigned to that node in the tree.
5. So next I insert a <div> containing a <p> - it has been given a UID of 11. So now if I expand that node in the tree, the expanded nodes data records that node 11 is expanded.
6. Now, if I paste in another <div> ahead of the original, you will see that the tree inserts a new collapsed node above the initial node. This node did not have a decoration at it's source code location when the document was parsed, and so it has been assigned a new UID of 19.
7. The second <div> in the tree remains expanded because the decoration lookup has ensured that this node retains the same UID of 11 as it did before, and the tree knows that node 11 is expanded.
8. Therefore, node 19 now appears in the document ahead of node 11.

Hopefully this explains it for you?

Screencast.2024-07-15.08.33.18.mp4

In summary,
Your solution is to not change the uids assigned already to the existing nodes by tracking them using the decorations. This will remove the need to find the updatedUids of the expanded nodes. Am I getting it right?

[Mrdigs]

@atulbhatt-system32 Yes, correct. The uid of a node never changes, therefore it's expanded state is never lost. Only new nodes are given new uids. That means the sequence of the uids is in insertion order, not source code order.

I need you to tell me whether this is a problem for the other parts of the code that sort the uids, as I cannot see what is the purpose sorting them.

[Mrdigs]

@atulbhatt-system32 I think I have resolved the video issue:

Screencast.Converted.mp4

[atulbhatt-system32]

@atulbhatt-system32 Yes, correct. The uid of a node never changes, therefore it's expanded state is never lost. Only new nodes are given new uids. That means the sequence of the uids is in insertion order, not source code order.

I need you to tell me whether this is a problem for the other parts of the code that sort the uids, as I cannot see what is the purpose sorting them.

We can decide if sorting will actually be needed if we use this mechanism based on how it will handle the below use case.

Ex: Cut and Paste

Let's say we have multiple nodes selected. Now I Cut them one by one, that is remove the source code sequentially for all nodes and copy them to clipboard.

But the thing is I can't parse it again and again for every time I perform the cut for each node. I need the final updated code and then I parse the html again.

So we sort the uids for the purpose of doing cut operation in such a way that the source code location of the nodes are not affected.

I hope it makes sense.

Now, if the editor can give you the updated source code locations based on the uids selected to cut then I think we don't need to sort at all. It will simplify the process entirely.

[Mrdigs]

@atulbhatt-system32 Ahhhh, now it makes sense! Thank you 🙏

Well I suppose if all else fails then it could sort on the source code location instead of the uids.

But yes, you can take a uid and find it's source code location before parsing by querying the uid decorations - and, the helperModel will contain the decorations. So this particular case seems simple to resolve and as you say, simplify it.

If we can say that one or the other of the above solutions can replace all cases of sorting by uid, then I think we're good to go.

Do you agree?

[atulbhatt-system32]

@Mrdigs Yes I agree.

[Mrdigs]

Hi,

I am close to creating a pull request for this now, so I thought I'd document the changes here beforehand:-

In useElementHelper.tsx::parseHtml(), the editor is queried to check if there is a decoration providing the uid of the node at the source code location of the current node. If there is then that uid is re-used for the tree node. If no decoration is found then a new uid is generated by incrementing the maxNodeUidRef current value.

Whether an existing decoration is found or a new uid is created, the function adds the uid and source code location to a Map<TNodeUid, TNodePositionInfo> which parseHtml() returns to the caller, and then added to the state using the setNodeUidPositions action, and so becomes part of the undo state stack.

The purpose of this Map is to record what decorations should exist at any point in the undo stack. In the CodeView component, the Map and the editor decorations are kept in sync by deleting or adding decorations as required. Note that the value property of the Editor component has been removed - this is deliberate: the value is now set in an effect accessing the editor model directly.

This is because calling setValue() on an ITextModel clears its decorations, and so it's important to note that it must not be called without also creating the decorations needed to store the current uids in the model.

Most of the remaining changes are to deal with the last point above. There is now a companion function to getEditorModelWithCurrentCode() called setEditorModelValue(), which copies both the value and the decorations from a source model to a target model:-

  function getEditorModelWithCurrentCode() {
    /*The role of helperModel is to perform all the edit operations in it 
  without affecting the actual codeViewInstanceModel and then apply the changes 
  to the codeViewInstanceModel all at once.*/
    const codeViewInstanceModel = monacoEditorRef.current?.getModel();
    const helperModel = editor.createModel("", "html");
    codeViewInstanceModel &&
      setEditorModelValue(codeViewInstanceModel, helperModel);
    return helperModel;
  }
  function setEditorModelValue(
    sourceModel: editor.ITextModel,
    targetModel: editor.ITextModel,
  ) {
    if (sourceModel && targetModel) {
      const uidDecorations = getUidDecorations(sourceModel);
      targetModel.setValue(sourceModel.getValue());
      targetModel.deltaDecorations([], uidDecorations);
    }
  }

Here's an example of how it is intended to be used. Note, it never calls helperModel.setValue() or codeViewInstanceModel.setValue() directly:-

  const someEdit = () => {
    const codeViewInstanceModel = monacoEditorRef.current?.getModel();
    const helperModel = getEditorModelWithCurrentCode();
    helperModel.applyEdits([editsToApply...]);
    setEditorModelValue(helperModel, codeViewInstanceModel);
  }

I have changed all of the edit operations in useElements.tsx to use this approach. They all appear to still work as before but somebody more familiar with the application needs to test them. The useTurnInto and useFormatCode actions have also been amended to do the same thing.

I have rewritten useFormatCode fairly extensively as it was overwriting whole node subtree's and that will remove decorations, losing the uids. This action now uses the "diff-match-patch" library to make formatting edits on the model using a diff between the original code and the formatted code, thereby preserving decorations - and uids.

The uid sorting question from earlier in the thread, I have resolved by changing the sortUidsAsc and sortUidsDsc functions to sort by source code location.

I think that's probably about it.

[Mrdigs]

One more thing, I am going to commit with the decorations showing to aid with testing. Once testing is complete, they can be hidden by commenting or removing two lines in the CodeView component:-

        newDecoration.options.hoverMessage = { value: uid };
        newDecoration.options.inlineClassName = "uid-decoration";

[edenvidal]

thanks a lot @Mrdigs! before i am testing it, i'd be happy to know.

@atulbhatt-system32 can you do the testings? also (@Mrdigs , @atulbhatt-system32 ) is there anything to test from a user experience standpoint? is there anything missing to fully resolve this issue to the core?

[Mrdigs]

I think it is fully resolved, although I missed testing it on a project with multiple files.

Basically, any operation on the tree that modifies the source. I think I have caught and tested all of them: duplicate, copy/cut, paste, move, group, ungroup, and format.

I should note that many of these operations create new nodes, including move (drag and drop), which is a cut and paste behind the scenes. In these cases, the new nodes get new uids, and therefore their expanded state is not preserved, so:

If an expanded node is moved to a new position, I wouldn't expect the expanded state of that node to be preserved, but all other node's state to be preserved.

However the solution does provide a mechanism to handle this in many cases: it would involve the cut operation storing the uid of the cut node in the clipboard (but not the copy operation), and the paste operation creating a decoration containing that uid at the paste location.

There's also a good amount of scope for simplifying the code for many of these operations via the decorations, but I have restricted myself to implementing the uid tracking and preventing any regression, due to time constraints and minimising changes.

[edenvidal]

from @Mrdigs

Let's take a concrete example of two different ways a user might make an edit to a document in the code editor, to demonstrate.

We start with a body element containing two `<p>` elements:

```html
<body>
<p>A paragraph</p>
<p>Another paragraph</p>
</body>

Now, imagine the user deletes the two <p> elements:

<body>
</body>

At this point, the document contains no <p> elements, only a <body> element. If they then add a <div> element containing two <p> elements, like so:

<body>
<div>
<p>A paragraph</p>
<p>Another paragraph</p>
</div>
</body>

Then, the overall effect of the two edits appears to be that the <p> elements have been wrapped in a <div>. But from the editor's perspective, they are not the <p> elements as before. The old <p> elements were deleted, along with all the metadata about them that the editor contains (i.e., their uids).

Let's start over with the same original document:

<body>
<p>A paragraph</p>
<p>Another paragraph</p>
</body>

This time, if the user moves the cursor to after the opening body tag and inserts a line break, a tab, and "<div>":

<body>
<div>
<p>A paragraph</p>
<p>Another paragraph</p>
</body>

Then moves the cursor down and inserts another tab on the 2 following lines:

<body>
<div>
    <p>A paragraph</p>
    <p>Another paragraph</p>
</body>

Then moves to the end of the line containing the second <p> and inserts a line break, a tab, and "</div>":

<body>
<div>
    <p>A paragraph</p>
    <p>Another paragraph</p>
</div>
</body>

Now, the two <p> elements have also been wrapped, but crucially they were never removed from the document and therefore they are the same <p> elements as existed before the edit, with the same metadata and therefore the same uids.

The first example describes how the group operation currently works (except it is performed with code instead of the user making direct edits), which is why it does not preserve the correct uids.

The second example is how it needs to work. The diff technique I referred to above emulates the second example by determining the minimum number of edits to apply to the document to reach the end state from the beginning state (that is, two inserted lines plus two inserted tabs), leaving the code in between unchanged.

[atulbhatt-system32]

from @Mrdigs

Let's take a concrete example of two different ways a user might make an edit to a document in the code editor, to demonstrate.

We start with a body element containing two `<p>` elements:

```html
<body>
<p>A paragraph</p>
<p>Another paragraph</p>
</body>

Now, imagine the user deletes the two <p> elements:

<body>
</body>

At this point, the document contains no <p> elements, only a <body> element. If they then add a <div> element containing two <p> elements, like so:

<body>
<div>
<p>A paragraph</p>
<p>Another paragraph</p>
</div>
</body>

Then, the overall effect of the two edits appears to be that the <p> elements have been wrapped in a <div>. But from the editor's perspective, they are not the <p> elements as before. The old <p> elements were deleted, along with all the metadata about them that the editor contains (i.e., their uids).

Let's start over with the same original document:

<body>
<p>A paragraph</p>
<p>Another paragraph</p>
</body>

This time, if the user moves the cursor to after the opening body tag and inserts a line break, a tab, and "<div>":

<body>
<div>
<p>A paragraph</p>
<p>Another paragraph</p>
</body>

Then moves the cursor down and inserts another tab on the 2 following lines:

<body>
<div>
    <p>A paragraph</p>
    <p>Another paragraph</p>
</body>

Then moves to the end of the line containing the second <p> and inserts a line break, a tab, and "</div>":

<body>
<div>
    <p>A paragraph</p>
    <p>Another paragraph</p>
</div>
</body>

Now, the two <p> elements have also been wrapped, but crucially they were never removed from the document and therefore they are the same <p> elements as existed before the edit, with the same metadata and therefore the same uids.

The first example describes how the group operation currently works (except it is performed with code instead of the user making direct edits), which is why it does not preserve the correct uids.

The second example is how it needs to work. The diff technique I referred to above emulates the second example by determining the minimum number of edits to apply to the document to reach the end state from the beginning state (that is, two inserted lines plus two inserted tabs), leaving the code in between unchanged.

I see the issue with how currently grouping is directly happening by replacing the existing

with wrapped

. This is causing the inconsistency.

For the context I haven't tested the app at this point for what all might not be working as expected in @Mrdigs implementation. But this gives us an idea on what we need to look on.

Also How about background sync. What happens when the changes are done in the file when the rnbw is not focused and then the changes syncs back.

[edenvidal]

@atulbhatt-system32 , we're handling the issue internally with @stanislavasal - we'll keep using this thread for your consultant 🙏

[Mrdigs]

@atulbhatt-system32 , @stanislavasal - I have resolved the issue related to selected nodes not being re-selected when undoing a group operation.

The issue was that parseHtml() was being called before the CodeView had a chance to update its content (and therefore it's decorations), leading to a temporary loss of the correct uids in the node tree.

I didn't want to change this order because it would likely have major unintended side effects, and so I have changed parseHtml() to allow the value of my new history state nodeUidPositions, which is used to correctly identify the old uids specifically only when didUndo or didRedo are true, as this is a special case where the uids have to come from the history rather than the editor decorations because of that order of the CodeView updating after parseHtml() is called.

My description above of the "div technique" for grouping and ungrouping operations isn't really related or needed, unless a requirement to retain expanded states during those operations is identified.

Also, the technique won't work well for cases where the nodes selected for grouping are not next to each other, because in that case, there is no choice but to "cut" the nodes out of the document, and then re-insert them as it currently does. But there is scope for potentially updating nodeUidPositions in the group operation to maintain uids even if they are cut and pasted.

Cheers

[edenvidal]

thanks @Mrdigs for the detailed answer and fix. i think it works and feels great (with the caveats you've mentioned). i think it need some ux and heavy testing in order to understand how it should go from here.

for now, we'll only need a few things to be able to accept this pr:

• remove the green marker indication in the code.
  
• earlier in our messages, you've mentioned cleaning redundant code. now that there's a somewhat stable solution, feel free to reduce and we'll review 🙏
• @atulbhatt-system32 your final confirmation.

[atulbhatt-system32]

@edenvidal I think the changes work great I did some basic testing I believe you might have done some proper real user testing by yourself.

As for code that also looks good.

[Mrdigs]

@edenvidal That's great news, I'll review the code and get back to you

[Mrdigs]

@edenvidal I have removed the green markers - just commented the code that adds them in case they needed in future (and left the CSS in rnbw.css).

I did some experimentation with disabling some code that I think may now be redundant (such as needToSelectNodePaths) but found there were the flow of the application demanded them in certain operations.

My recommendation is to let this PR settle and allow the code to evolve incrementally with the new approach rather than attempting to make too many changes at once.

[edenvidal]

thanks @Mrdigs , this is great!
@stanislavasal let's confirm the pr 🙏