diff --git a/frontend/src/ofrak/remote_resource.js b/frontend/src/ofrak/remote_resource.js index 8b7dd70c3..059d32ba7 100644 --- a/frontend/src/ofrak/remote_resource.js +++ b/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, comment) { + 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, comment]), + body: JSON.stringify([optional_range, comment_text]), }).then(async (r) => { if (!r.ok) { throw Error(JSON.stringify(await r.json(), undefined, 2)); diff --git a/frontend/src/ofrak/resource.js b/frontend/src/ofrak/resource.js index 428c5273a..f442ae2f1 100644 --- a/frontend/src/ofrak/resource.js +++ b/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, comment) { + async delete_comment(optional_range, comment_text) { throw new NotImplementedError("delete_comment"); } diff --git a/ofrak_core/ofrak/core/comments.py b/ofrak_core/ofrak/core/comments.py index 0b751ceeb..1d9139571 100644 --- a/ofrak_core/ofrak/core/comments.py +++ b/ofrak_core/ofrak/core/comments.py @@ -47,8 +47,6 @@ 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. if config.comment[0] not in comments: comments[config.comment[0]] = [] @@ -59,36 +57,24 @@ async def modify(self, resource: Resource, config: AddCommentModifierConfig) -> @dataclass class DeleteCommentModifierConfig(ComponentConfig): """ - comment_range: Tuple[Optional[Range], comment_text=Optional[str]] - If comment_text is provided, deletes the matching comment with the same Optional[Range] - If comment_text is None, deletes ALL comments with the same Optional[Range] + 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: Tuple[Optional[Range], Optional[str]] - - def __post_init__(self): - # Ensure there's always a two-element Tuple - if type(self.comment_range) == tuple: - # New format - self.comment_range = (*self.comment_range, None)[:2] - elif type(self.comment_range) == Range: - # Old format: Range - self.comment_range = (self.comment_range, None) - else: - # Old format: None (no Range provided) - self.comment_range = (None, None) + 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. """ @@ -97,24 +83,22 @@ async def modify(self, resource: Resource, config: DeleteCommentModifierConfig) except NotFoundError: comments = {} try: - if len(config.comment_range) == 1: - config.comment_range = (config.comment_range[0], None) - - if config.comment_range[1] is None: - del comments[config.comment_range[0]] + if config.comment_text is None: + # Delete ALL comments in this range + del comments[config.comment_range] else: - comments[config.comment_range[0]].remove(config.comment_range[1]) - # Clean up if this was the last comment at this range - if len(comments[config.comment_range[0]]) == 0: - del comments[config.comment_range[0]] + 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[0]} not found in " + f"Comment range {config.comment_range} not found in " f"resource {resource.get_id().hex()}" ) except ValueError: raise NotFoundError( - f"Comment {config.comment_range[1]} with range {config.comment_range[0]}" + 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)) diff --git a/ofrak_core/ofrak/gui/server.py b/ofrak_core/ofrak/gui/server.py index 90638369f..37cd62eaf 100644 --- a/ofrak_core/ofrak/gui/server.py +++ b/ofrak_core/ofrak/gui/server.py @@ -735,19 +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 = self._serializer.from_pjson( + 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}) + DeleteCommentModifier, DeleteCommentModifierConfig({comment_range}, {comment_text}) )""" ) await self.script_builder.add_action(resource, script_str, ActionType.MOD) try: - result = await resource.run(DeleteCommentModifier, DeleteCommentModifierConfig(comment)) + result = await resource.run( + DeleteCommentModifier, DeleteCommentModifierConfig(comment_range, comment_text) + ) await self.script_builder.commit_to_script(resource) except Exception as e: await self.script_builder.clear_script_queue(resource) diff --git a/ofrak_core/test_ofrak/components/test_comments.py b/ofrak_core/test_ofrak/components/test_comments.py index 3161d8881..2b5956669 100644 --- a/ofrak_core/test_ofrak/components/test_comments.py +++ b/ofrak_core/test_ofrak/components/test_comments.py @@ -196,7 +196,9 @@ async def test_deleting_comments(executable_resource: Resource): # Test deletion of specific comments await executable_resource.run( DeleteCommentModifier, - DeleteCommentModifierConfig(comment_range=(Range(0, 1), "first range comment 1")), + DeleteCommentModifierConfig( + comment_range=Range(0, 1), comment_text="first range comment 1" + ), ) comment_attributes = executable_resource.get_attributes(CommentsAttributes) comments = comment_attributes.comments @@ -206,7 +208,9 @@ async def test_deleting_comments(executable_resource: Resource): # Test specific deletion of last comment in a range await executable_resource.run( DeleteCommentModifier, - DeleteCommentModifierConfig(comment_range=(Range(0, 1), "first range comment 2")), + DeleteCommentModifierConfig( + comment_range=Range(0, 1), comment_text="first range comment 2" + ), ) comment_attributes = executable_resource.get_attributes(CommentsAttributes) comments = comment_attributes.comments @@ -217,7 +221,7 @@ async def test_deleting_comments(executable_resource: Resource): # Test deletion of entire ranges with new DeleteCommentModifierConfig format await executable_resource.run( DeleteCommentModifier, - DeleteCommentModifierConfig(comment_range=(Range(1, 2), None)), + DeleteCommentModifierConfig(comment_range=Range(1, 2), comment_text=None), ) comment_attributes = executable_resource.get_attributes(CommentsAttributes) assert get_comment_count(comment_attributes) == 5 @@ -255,12 +259,12 @@ async def test_deleting_non_existing_comment(executable_resource: Resource): with pytest.raises(NotFoundError): await executable_resource.run( DeleteCommentModifier, - DeleteCommentModifierConfig(comment_range=(None, "this doesn't exist")), + DeleteCommentModifierConfig(comment_range=None, comment_text="this doesn't exist"), ) await executable_resource.run( DeleteCommentModifier, - DeleteCommentModifierConfig(comment_range=(None, "this exists")), + DeleteCommentModifierConfig(comment_range=None, comment_text="this exists"), ) comment_attributes = executable_resource.get_attributes(CommentsAttributes)