• 
      

    Rewrite the comment dialog using a model/view split, with unit tests.

    Review Request #3704 — Created Dec. 27, 2012 and submitted

    Information

    Review Board
    release-1.7.x

    Reviewers

    Rewrite the comment dialog using a model/view split, with unit tests.
    
    This splits up the old commentDlg singleton and replaces it with a new
    CommentEditor model and CommentDialogView view.
    
    CommentEditor keeps track of the state of a pending comment edit. This
    incudes what actions (save, delete, cancel) are allowed, the text of the
    comment, the actual Comment backing it, opened issue state, and the
    dirty state. It also provides the actions you can make, such as saving
    the contents of the editor back to the comment and to the server, or
    deleting it.
    
    CommentDialogView replaces the old commentDlg view code, but leaves all
    the state handling and operations up to the associated CommentEditor. It
    also splits out the list of existing published comments into a separate
    sub-view.
    
    Unit tests are included to test the majority of this new functionality.
    It should help catch any major regressions going forward.
    
    There are improvements I want to make to this as we develop our
    architecture. One is to rewrite how the existing published comments list
    works. Right now, it's very unclean, since it's based on how Comments
    currently work and how we handle serialization/deserialization. Later,
    it should get a real backing and manage its own state better.
    
    It also doesn't prevent editing on draft review requests, but that's not
    a regression, and will be covered either in a revision to this, or in
    another change.
    Tested various scenarios with diff, screenshot and file attachment
    comments. This included simulating that I couldn't edit comments
    (currently only due to not being logged in), creating new comments,
    editing draft comments, and creating comments in regions with
    existing published comments.
    
    Created a bunch of new unit tests to test various state (which helped
    catch a bunch of bugs while writing this). They all pass in Chrome
    and Firefox.
    JA
    david
    1. Nothing jumps out at me, but I think this will require a lot of testing. What branch did you want to push this to?
      1. master is fine for now. We'll keep release-1.7.x in a stable state from here on out. I would like to try to get this stuff into 1.7.x releases sooner than later, though.
    2. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to master (101a39c)