From 2fc06a08842cc7d5f7ef105ed96f1faea2da0551 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Fri, 19 Nov 2021 01:10:04 -0500
Subject: [PATCH] Changeset: Add TODO comments for issues noticed

---
 src/node/utils/padDiff.js         | 2 ++
 src/static/js/Changeset.js        | 2 ++
 src/static/js/contentcollector.js | 6 ++++++
 3 files changed, 10 insertions(+)

diff --git a/src/node/utils/padDiff.js b/src/node/utils/padDiff.js
index a8841cf22..05cba308f 100644
--- a/src/node/utils/padDiff.js
+++ b/src/node/utils/padDiff.js
@@ -383,6 +383,8 @@ PadDiff.prototype._createDeletionChangeset = function (cs, startAText, apool) {
             const oldValue = oldAttribs.get(key);
             if (oldValue !== value) backAttribs.set(key, oldValue);
           }
+          // TODO: backAttribs does not restore removed attributes (it is missing attributes that
+          // are in oldAttribs but not in attribs). I don't know if that is intentional.
           return backAttribs.toString();
         });
 
diff --git a/src/static/js/Changeset.js b/src/static/js/Changeset.js
index 11494a5f7..24cc855a2 100644
--- a/src/static/js/Changeset.js
+++ b/src/static/js/Changeset.js
@@ -2098,6 +2098,8 @@ exports.inverse = (cs, lines, alines, pool) => {
             const oldValue = oldAttribs.get(key) || '';
             if (oldValue !== value) backAttribs.set(key, oldValue);
           }
+          // TODO: backAttribs does not restore removed attributes (it is missing attributes that
+          // are in oldAttribs but not in attribs). I don't know if that is intentional.
           return backAttribs.toString();
         });
         consumeAttribRuns(csOp.chars, (len, attribs, endsLine) => {
diff --git a/src/static/js/contentcollector.js b/src/static/js/contentcollector.js
index 59e726232..2e6005bd8 100644
--- a/src/static/js/contentcollector.js
+++ b/src/static/js/contentcollector.js
@@ -235,6 +235,9 @@ const makeContentCollector = (collectStyles, abrowser, apool, className2Author)
       // to enable the content collector to store key-value attributes
       // see https://github.com/ether/etherpad-lite/issues/2567 for more information
       // in long term the contentcollector should be refactored to get rid of this workaround
+      //
+      // TODO: This approach doesn't support changing existing values: if both 'foo::bar' and
+      // 'foo::baz' are in state.attribs then the last one encountered while iterating will win.
       const ATTRIBUTE_SPLIT_STRING = '::';
 
       // see if attributeString is splittable
@@ -265,6 +268,9 @@ const makeContentCollector = (collectStyles, abrowser, apool, className2Author)
     const attribs = new AttributeMap(apool)
         .set('lmkr', '1')
         .set('insertorder', 'first')
+        // TODO: Converting all falsy values in state.lineAttributes into removals is awkward.
+        // Better would be to never add 0, false, null, or undefined to state.lineAttributes in the
+        // first place (I'm looking at you, state.lineAttributes.start).
         .update(Object.entries(state.lineAttributes).map(([k, v]) => [k, v || '']), true);
     lines.appendText('*', attribs.toString());
   };