Display commits in the diffviewer based on the selected revision

Review Request #6933 — Created Feb. 11, 2015 and submitted

brennie
Review Board
dvcs
6816, 6815
6943
9c6d138...
reviewboard
chipx86, mike_conley, smacleod

The DiffViewerView now provides information about the DiffCommits on
the selected revision. The DiffViewerPageView and associated models
have been updated to parse this information. The information on
DiffCommits is shown in the diffviewer now via the DiffCommitIndexView,
which renders a table similar to the DiffCommitListField on a review
request.

When the diff revision slider is moved to select a diff revision, the
DiffCommitListField will automatically update and populate the table
with information from the individual diff commits pertaining to the
selected revision. If the selected diff revision does not have any
commits, then the table will be hidden until a revision is selected that
does have associated commits.

Manually verified the following:

  • Selecting the base and a diff revision that had commits displayed the
    commit table.
  • Selecting the base and a diff revision that did not have commits did
    not display the table.
  • Switching from one diff revision to another with different commits
    changed the table to display the new commits.
Loading file attachments...

  • 0
  • 0
  • 13
  • 2
  • 15
Description From Last Updated
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
brennie
brennie
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/diffviewer/views.py (Diff revision 2)
     
     
     
     
     
     
     

    We can reduce the work needed with:

    diff_context['diff_commits'] = \
        diffset.diff_commits.values('commit_id', 'summary', 'author_name')
    
  3. Can you sort these alphabetically, with comments?

  4. If you append this after building all the children, you'll reduce the work performed by the browser.

  5. Can this be one line?

  6. You can use this.$el for this.

  7. Should be inserted alphabetically.

  8. 
      
brennie
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
david
  1. 
      
    1. It will return the empty string because ''.split('\n') returns [''].

  2. 
      
chipx86
  1. 
      
  2. Can we put the comments before the attributes, like in other models? That helps to prevent having to modify other lines when we add things, in order to line things back up again.

  3. These need to be localized.

    Given that, we should just use _.template for this.

  4. Can we use underscores in the ID to match the other diff ones?

  5. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
david
  1. 
      
  2. Did you test this? I don't think <%- %> can work this way (since it pulls values out of the object passed in to the template). Usually we use names here and then call the template like this:

    _tableHeader({
        summaryTitle: gettext('Summary'),
        authorTitle: gettext('Author')
    });
    
    1. Yep, I tested it and it works. It apparently will evaluate whatever is in there and return the result:

      >>> _.template('<%- gettext("Author") %>')()
      "Author"
      >>> _.template('<%- 1 + 1 %>')()
      "2"
      
    2. You learn something new every day.

  3. This line is useless because you set it again in update()

  4. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
david
  1. 
      
  2. These two lines can be combined.

  3. Just to double check, you want it to still take up room on the page? If not, this should use .show()/.hide()

    1. I've changed it to use show() and hide().

  4. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
david
  1. 
      
  2. Now that you've moved the creation into here, you don't need the .empty() call.

  3. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
        reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
        reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to dvcs (ba84d9d)
Loading...