Recently Edited: Merge duplicate fix

[shresthabijay]

refers to #323

[shresthabijay]

Hi. I have made these changes to the data structure and merge rules. For now I have used the object structure instead of tree. With this implementation we only have to iterate the list once for merging and deleting O(n). It removes all the duplicates.

Yet to implement replacing descendants if the descendant has not been edited in a long time. We need a proper way to removing direct sibling relations between new context when we replace descendants.

Please review this approach and let me know if all the merging conditions are correctly working. I still am working on tree structure on sidelines.

[raineorshine]

It seems that this will be substantially different with the tree implementation, so maybe I will wait until then to review, given the complexity of this code. Do you still need me to review before implementing the tree structure?

[shresthabijay]

It seems that this will be substantially different with the tree implementation, so maybe I will wait until then to review, given the complexity of this code. Do you still need me to review before implementing the tree structure?

No it won't be necessary to review this right now . I will complete the tree implementation first.

[shresthabijay]

Hey Raine. I have completed the tree structure implementation. I have written logic for almost all of the merge cases we discussed earlier. I have tested on my own . There are no duplicates anymore and merges are happening properly. I have written logic for replacing descendant context with the ancestor if descendant thoughts are too old too. I still have to work on limiting no of recently edited thoughts in the tree. Please test the merge cases, deletion and ancestor replacing descendant cases thoroughly and let know if everything is happening as we discussed. Also let me know about the possible changes you want me to make to the codebase.

[raineorshine]

Haven't looked at the code yet, but functionally this is working great! Everything is merging very intuitively. I've tested some of our original use cases, plus some random adding, editing, and deleting. Very impressive!

I'd like to try this "in the wild" and provide more feedback on the behavior after I have used it with real data. This is already a big improvement though.

The only thing that seemed off was moving:

e.g.

• a
  • b
  • c
    • d
      • e

The recently edited list is a.c.d.e. When I move c before b, it becomes a.c. I would have expected it to still be a.c.d.e.

Similar behavior occurs when dragging into a different context, but the expected behavior is not as obvious, so I am not sure yet if I consider that an issue.

[raineorshine]

Here are some recommendations for code improvements!

I have not had the chance to review onNodeChange yet. I'm going to need to clear my head to get through that one :). Maybe we can find a way to make it a little more readable/less complex.

[shresthabijay]

Hey Raine. In this latest commit I have changed the class implementation back to functional and used Immer for immutability. Immer produce allows mutatation to its draftState and it will produce the nextState based on the mutations to the draft. We already have functions for treeChange, treeDelete that mutates the tree . So we can just wrap them with Immers produce and provide draftState to them instead of original tree and even after all the mutations we will get pure immutable tree object in return from produce. Works like magic.

I have completed move logic too but it still needs to be tested properly.

All the requested changes are not in this latest commit though. I will work on those changes after you give me green flag on Immer implementation and move logic test.

[raineorshine]

Good progress! I'm happy with Immer going forward. Some additional comments added.

[shresthabijay]

In this latest change I have fixed _.get iteration issue, added jsdocs style comments and other requested changes. I also completed move logic. I have tested and merges are happening intuitively. I want you review the functionality properly specially deleting and moving. Let me know soon!

[raineorshine]

Minor change, but could you add a shortcut for toggling the sidebar?

toggleSidebar.js:

import { store } from '../store.js'

export default {
  id: 'toggleSidebar',
  name: 'Toggle Recently Edited',
  keyboard: { alt: true, key: 'r' },
  hideFromInstructions: true,
  exec: () => {
    store.dispatch({ type: 'toggleSidebar', value: !store.getState().showSidebar })
  }
}

[raineorshine]

This is working so well! I only encountered one edge case and one crash that sadly I cannot reproduce.

The edge case involves the command subcategorizeAll.

Steps to Reproduce

Start with these thoughts:

• a
  • b
    • c
  • d
    • e

Recently edited list is:

• a . d . e
• a . b . c

Now move the cursor to a . b, execute subcategorizeAll, and enter "x". (The effect is the same whether you edit the new thought or not, it's just clearer to show with a non-empty thought.)

Thoughts are now:

• a
  • x
    • b
      -c
    • d
      • e

Current Behavior

Recently edited list is:

• a . x . d
• a . x . b . c

Expected Behavior

Recently edited list should be:

• a . x . d . e
• a . x . b . c

i.e. e is dropped.

subcategorizeAll dispatches several independent existingThoughtMove actions, which would be better as a single composed reducer. Maybe that is causing the unusual result.

[shresthabijay]

Hi I have fixed the subcategorizeAll issue and also refactored some functions in this latest changes. Please review the changes and functionality. Thank you!

[raineorshine]

Given the complexity of this problem, you have done an impressive job architecting a solution that is not only functionally precise, but does so with a minimum of logic. Well done!

Now that the recursive functions have been refactored, I don't see any obvious performance problem. Of course it's difficult to tell the exact performance implications, but I think it's good enough for now, especially with throttled edits. It will be obvious in our performance profiling if there is any issue.

I have a few suggestions, mostly about cleaning up the code or simplifying certain structures. There is one important question regarding nodeMove.

If there is anything from your Google Docs documentation that would make sense to add as a comment in the code for other developers, please add it.

Thanks for all your work on this!!!

[raineorshine]

Minor change, but could you add a shortcut for toggling the sidebar?

toggleSidebar.js:

import { store } from '../store.js'

export default {
  id: 'toggleSidebar',
  name: 'Toggle Recently Edited',
  keyboard: { alt: true, key: 'r' },
  hideFromInstructions: true,
  exec: () => {
    store.dispatch({ type: 'toggleSidebar', value: !store.getState().showSidebar })
  }
}

@shresthabijay

[shresthabijay]

Hi! I have added nodeAdd function that just calls nodeChange but is more structured for special case of adding new node. I have added important comments and examples to clarify how it is working. I also added created a DUMMY_TOKEN instead of direct use of empty string in nodeAdd.

I also added toggleSidebar shortcut. But alt+r was not working somehow , so i used alt+e for now.

Please review the changes. Thank you!

[shresthabijay]

I totally forgot about this change. It's done in the latest commit !

[shresthabijay]

In this latest commit I have modified nodeAdd to only use single parameter as we discussed earlier . I also implemented context encoding for escaping unsafe characters including empty string for firebase sync. Please review the changes!