Add support for multiple comments on the same Range (#459)

frontend/src/ofrak/remote_resource.js

@@ -468,13 +468,13 @@ export class RemoteResource extends Resource {
     return find_replace_results;
   }
 
-  async add_comment(optional_range, comment) {
+  async add_comment(optional_range, comment_text) {
     await fetch(`${this.uri}/add_comment`, {
       method: "POST",
       headers: {
         "Content-Type": "application/json",
       },
-      body: JSON.stringify([optional_range, comment]),
+      body: JSON.stringify([optional_range, comment_text]),
     }).then(async (r) => {
       if (!r.ok) {
         throw Error(JSON.stringify(await r.json(), undefined, 2));
@@ -508,13 +508,13 @@ export class RemoteResource extends Resource {
     await this.update_script();
   }
 
-  async delete_comment(optional_range) {
+  async delete_comment(optional_range, comment_text) {
     await fetch(`${this.uri}/delete_comment`, {
       method: "POST",
       headers: {
         "Content-Type": "application/json",
       },
-      body: JSON.stringify(optional_range),
+      body: JSON.stringify([optional_range, comment_text]),
     }).then(async (r) => {
       if (!r.ok) {
         throw Error(JSON.stringify(await r.json(), undefined, 2));

frontend/src/ofrak/resource.js

@@ -365,11 +365,11 @@ export class Resource {
     throw new NotImplementedError("summarize_tree");
   }
 
-  async add_comment(optional_range, comment) {
+  async add_comment(optional_range, comment_text) {
     throw new NotImplementedError("add_comment");
   }
 
-  async delete_comment(optional_range) {
+  async delete_comment(optional_range, comment_text) {
     throw new NotImplementedError("delete_comment");
   }
 

frontend/src/resource/ResourceTreeNode.svelte

@@ -244,15 +244,17 @@
   >
     {rootResource.get_caption()}
   </button>
-  {#await commentsPromise then comments}
-    {#each comments as comment}
-      <div class="comment">
-        <Comment
-          comment="{comment}"
-          rootResource="{rootResource}"
-          selfId="{selfId}"
-        />
-      </div>
+  {#await commentsPromise then comment_group}
+    {#each comment_group as [comment_range, comment_strs]}
+      {#each comment_strs as comment_text}
+        <div class="comment">
+          <Comment
+            comment="{{ comment_range, comment_text }}"
+            rootResource="{rootResource}"
+            selfId="{selfId}"
+          />
+        </div>
+      {/each}
     {/each}
   {/await}
 

frontend/src/views/Comment.svelte

@@ -20,17 +20,18 @@
   import Hoverable from "../utils/Hoverable.svelte";
   import Icon from "../utils/Icon.svelte";
   export let comment, rootResource, selfId;
-  let text = comment[1];
-  let addresses = text.matchAll("#[a-fA-F0-9]+[@0x[0-9a-fA-F]+]*", text);
+  let addresses = comment.comment_text.matchAll(
+    "#[a-fA-F0-9]+(@0x[0-9a-fA-F]+)?"
+  );
   let text_elements = [];
   Array.from(addresses).forEach((location) => {
-    let text_split = text.split(location[0]);
+    let text_split = comment.comment_text.split(location[0]);
     text_elements.push(text_split[0]);
     text_elements.push(createAddressButton(location[0]));
-    text = text_split.slice(1).join(location[0]);
+    comment.comment_text = text_split.slice(1).join(location[0]);
   });
 
-  text_elements.push(text);
+  text_elements.push(comment.comment_text);
 
   function createAddressButton(location) {
     let resource_id;
@@ -53,19 +54,22 @@
     return button;
   }
 
-  async function onDeleteClick(optional_range) {
+  async function onDeleteClick(comment) {
     // Delete the selected comment.
     // As a side effect, the corresponding resource gets selected.
     $selected = selfId;
-    await rootResource.delete_comment(optional_range);
+    await rootResource.delete_comment(
+      comment.comment_range,
+      comment.comment_text
+    );
     $resourceNodeDataMap[$selected].commentsPromise =
       rootResource.get_comments();
   }
 </script>
 
 <div class="comment">
   <Hoverable let:hovering>
-    <button title="Delete this comment" on:click="{onDeleteClick(comment[0])}">
+    <button title="Delete this comment" on:click="{onDeleteClick(comment)}">
       <Icon
         class="comment_icon"
         url="{hovering ? '/icons/trash_can.svg' : '/icons/comment.svg'}"

ofrak_core/ofrak/core/comments.py

@@ -1,6 +1,6 @@
 from copy import deepcopy
 from dataclasses import dataclass
-from typing import Dict, Tuple, Optional
+from typing import Dict, List, Tuple, Optional
 from ofrak.component.modifier import Modifier
 from ofrak.model.component_model import ComponentConfig
 from ofrak.model.resource_model import ResourceAttributes
@@ -12,10 +12,10 @@
 @dataclass(**ResourceAttributes.DATACLASS_PARAMS)
 class CommentsAttributes(ResourceAttributes):
     """
-    User-defined comments, each comment associated with an optional range.
+    User-defined comments, list of the comments associated with an optional range.
     """
 
-    comments: Dict[Optional[Range], str]
+    comments: Dict[Optional[Range], List[str]]
 
 
 @dataclass
@@ -39,7 +39,6 @@ async def modify(self, resource: Resource, config: AddCommentModifierConfig) ->
                     f"Range {config_range} is outside the bounds of "
                     f"resource {resource.get_id().hex()}"
                 )
-
         try:
             # deepcopy the existing comments, otherwise they would be modified in place
             # and OFRAK would then compare the new attributes with the existing ones and find
@@ -48,32 +47,34 @@ async def modify(self, resource: Resource, config: AddCommentModifierConfig) ->
         except NotFoundError:
             comments = {}
 
-        # Here I'm appending appending overlapping comments with a new line.
-        # Overwriting comments that share a range is counter intuitive and not easily understood without digging into the code.
-        # TODO: Refactor comments strucutre to not key off of the range to allow individual overlapping comment attributes.
-        if config.comment[0] in comments:
-            comment = config.comment[1] + "\n" + comments[config.comment[0]]
-        else:
-            comment = config.comment[1]
-        comments[config.comment[0]] = comment
+        if config.comment[0] not in comments:
+            comments[config.comment[0]] = []
+
+        comments[config.comment[0]].append(config.comment[1])
         resource.add_attributes(CommentsAttributes(comments=comments))
 
 
 @dataclass
 class DeleteCommentModifierConfig(ComponentConfig):
+    """
+    If comment_text is provided, deletes the matching comment in that comment_range
+    If comment_text is None, deletes ALL comments in that comment_range
+    """
+
     comment_range: Optional[Range]
+    comment_text: Optional[str] = None
 
 
 class DeleteCommentModifier(Modifier[DeleteCommentModifierConfig]):
     """
-    Modifier to delete a comment from a resource.
+    Modifier to delete comment(s) from a resource.
     """
 
     targets = ()
 
     async def modify(self, resource: Resource, config: DeleteCommentModifierConfig) -> None:
         """
-        Delete the comment associated with the given range.
+        Delete the comment(s) associated with the given range.
 
         :raises NotFoundError: if the comment range is not associated with a comment.
         """
@@ -82,10 +83,22 @@ async def modify(self, resource: Resource, config: DeleteCommentModifierConfig)
         except NotFoundError:
             comments = {}
         try:
-            del comments[config.comment_range]
+            if config.comment_text is None:
+                # Delete ALL comments in this range
+                del comments[config.comment_range]
+            else:
+                comments[config.comment_range].remove(config.comment_text)
+                # Clean up if this was the last comment in this range
+                if len(comments[config.comment_range]) == 0:
+                    del comments[config.comment_range]
         except KeyError:
             raise NotFoundError(
                 f"Comment range {config.comment_range} not found in "
                 f"resource {resource.get_id().hex()}"
             )
+        except ValueError:
+            raise NotFoundError(
+                f"Comment {config.comment_text} with range {config.comment_range}"
+                f" not found in resource {resource.get_id().hex()}"
+            )
         resource.add_attributes(CommentsAttributes(comments=comments))

ofrak_core/ofrak/gui/server.py

@@ -735,18 +735,30 @@ async def add_comment(self, request: Request) -> Response:
     @exceptions_to_http(SerializedError)
     async def delete_comment(self, request: Request) -> Response:
         resource = await self._get_resource_for_request(request)
-        comment_range = self._serializer.from_pjson(await request.json(), Optional[Range])
+        comment_data = self._serializer.from_pjson(
+            await request.json(), Union[Tuple[Optional[Range], Optional[str]], Optional[Range]]
+        )
+        comment_range: Optional[Range] = None
+        comment_text: Optional[str] = None
+
+        if type(comment_data) == tuple:
+            comment_range = comment_data[0]
+            comment_text = comment_data[1]
+        else:
+            comment_range = comment_data
+
         script_str = (
             """
         await {resource}.run"""
             f"""(
-            DeleteCommentModifier, DeleteCommentModifierConfig({comment_range})
+            DeleteCommentModifier, DeleteCommentModifierConfig(comment_range={comment_range}, comment_text="{comment_text}")
         )"""
         )
         await self.script_builder.add_action(resource, script_str, ActionType.MOD)
         try:
             result = await resource.run(
-                DeleteCommentModifier, DeleteCommentModifierConfig(comment_range)
+                DeleteCommentModifier,
+                DeleteCommentModifierConfig(comment_range=comment_range, comment_text=comment_text),
             )
             await self.script_builder.commit_to_script(resource)
         except Exception as e: