Comment serialization cleanup part 2: Clarify serialization structures.

Review Request #13656 — Created March 21, 2024 and submitted

Information

Review Board
release-7.x

Reviewers

This is part 2 of the serialization cleanup. In this change, we now have
matching structures for serialized comments in Python (defined in the
Review UI files) and JavaScript (defined in the commentData.ts file).
These are fully documented and include cross-references to ensure that
we keep them in sync.

This also makes the serialization stage within the Review UIs more
consistent. One of the giant warts we've had is that the basic ReviewUI
implementation would serialize the comments as a list of dicts, and then
our subclasses would serialize them as a dict, where the key was some
logical grouping (such as x/y/width/height for region comments, or
line/span for text comments) and the value was a list of serialized
comments for that region. We now always serialize to the dict form, and
the base ReviewUI will just use indices for the key and single-value
lists for the values.

A new method has been added to the base ReviewUI to facilitate this, and
just filters and yields the comments. Subclasses can then use this and
sort them however they like. Using this new method fixes a bug in the
TextBasedReviewUI where we weren't doing filtering, and draft comments
from other users could show up when they shouldn't.

Ran js-tests.

Summary ID
Comment serialization cleanup part 2: Clarify serialization structures.
This is part 2 of the serialization cleanup. In this change, we now have matching structures for serialized comments in Python (defined in the Review UI files) and JavaScript (defined in the commentData.ts file). These are fully documented and include cross-references to ensure that we keep them in sync. This also makes the serialization stage within the Review UIs more consistent. One of the giant warts we've had is that the basic ReviewUI implementation would serialize the comments as a list of dicts, and then our subclasses would serialize them as a dict, where the key was some logical grouping (such as x/y/width/height for region comments, or line/span for text comments) and the value was a list of serialized comments for that region. We now always serialize to the dict form, and the base ReviewUI will just use indices for the key and single-value lists for the values. A new method has been added to the base ReviewUI to facilitate this, and just filters and yields the comments. Subclasses can then use this and sort them however they like. Using this new method fixes a bug in the `TextBasedReviewUI` where we weren't doing filtering, and draft comments from other users could show up when they shouldn't. Testing Done: Ran js-tests.
8ed7bcfe26b0f4152dab8462a36ef137db433f6a
Description From Last Updated

The second argument can be deserialized data to compare. If it's a string, it gets loaded, but we don't need …

chipx86chipx86

Maybe worth just comparing the entire payloads going forward?

chipx86chipx86

Let's use :file: for this.

chipx86chipx86

Let's use :file: for this.

chipx86chipx86

This is missing docs.

chipx86chipx86

We don't normally use the outer parens here.

chipx86chipx86

Should use :file:.

chipx86chipx86

We have the nice new ** embedding in dictionaries now, so we could do: return { **super().serialize_comment(comment), 'x': comment.x, 'y': …

chipx86chipx86

:file:

chipx86chipx86

This should be before any classes/functions.

chipx86chipx86

Version Added?

chipx86chipx86

This should use the standard multi-line comment format. Maybe add a TODO or TODO [some-tag] here?

chipx86chipx86

:file:

chipx86chipx86

:file:

chipx86chipx86

:file:

chipx86chipx86
chipx86
  1. 
      
  2. reviewboard/reviews/tests/test_review_ui.py (Diff revision 1)
     
     
     
    Show all issues

    The second argument can be deserialized data to compare. If it's a string, it gets loaded, but we don't need to provide it as a string.

  3. reviewboard/reviews/tests/test_review_ui.py (Diff revision 1)
     
     
     
     
    Show all issues

    Maybe worth just comparing the entire payloads going forward?

    1. Perhaps, but there's a lot of data there. I think for now I'd rather just keep this as-is.

  4. reviewboard/reviews/ui/base.py (Diff revision 1)
     
     
    Show all issues

    Let's use :file: for this.

  5. reviewboard/reviews/ui/base.py (Diff revision 1)
     
     
    Show all issues

    Let's use :file: for this.

  6. reviewboard/reviews/ui/base.py (Diff revision 1)
     
     
    Show all issues

    This is missing docs.

  7. reviewboard/reviews/ui/base.py (Diff revision 1)
     
     
    Show all issues

    We don't normally use the outer parens here.

  8. reviewboard/reviews/ui/image.py (Diff revision 1)
     
     
    Show all issues

    Should use :file:.

  9. reviewboard/reviews/ui/screenshot.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    We have the nice new ** embedding in dictionaries now, so we could do:

    return {
        **super().serialize_comment(comment),
        'x': comment.x,
        'y': comment.y,
        'width': comment.w,
        'height': comment.h,
    }
    
  10. reviewboard/reviews/ui/text.py (Diff revision 1)
     
     
    Show all issues

    :file:

  11. reviewboard/reviews/ui/text.py (Diff revision 1)
     
     
    Show all issues

    This should be before any classes/functions.

  12. Show all issues

    Version Added?

  13. Show all issues

    This should use the standard multi-line comment format.

    Maybe add a TODO or TODO [some-tag] here?

    1. I'm going to drop this because this goes away in a later change in the series.

  14. Show all issues

    :file:

  15. Show all issues

    :file:

  16. Show all issues

    :file:

  17. 
      
david
maubin
  1. Ship It!
  2. 
      
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-7.x (9d4b4cb)