Display commits in the diffviewer based on the selected revision

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

Information

Review Board
dvcs
9c6d138...

Reviewers

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.

Description From Last Updated

We can reduce the work needed with: diff_context['diff_commits'] = \ diffset.diff_commits.values('commit_id', 'summary', 'author_name')

chipx86chipx86

Can you sort these alphabetically, with comments?

chipx86chipx86

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

chipx86chipx86

No space after function.

chipx86chipx86

Can this be one line?

chipx86chipx86

You can use this.$el for this.

chipx86chipx86

Should be inserted alphabetically.

chipx86chipx86

Can we put the comments before the attributes, like in other models? That helps to prevent having to modify other …

chipx86chipx86

What if this is empty?

daviddavid

These need to be localized. Given that, we should just use _.template for this.

chipx86chipx86

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

chipx86chipx86

Did you test this? I don't think <%- %> can work this way (since it pulls values out of the …

daviddavid

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

daviddavid

These two lines can be combined.

daviddavid

Now that you've moved the creation into here, you don't need the .empty() call.

daviddavid
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)
     
     
     
     
     
     
     
    Show all issues

    We can reduce the work needed with:

    diff_context['diff_commits'] = \
        diffset.diff_commits.values('commit_id', 'summary', 'author_name')
    
  3. Show all issues

    Can you sort these alphabetically, with comments?

  4. Show all issues

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

  5. Show all issues

    No space after function.

  6. Show all issues

    Can this be one line?

  7. Show all issues

    You can use this.$el for this.

  8. Show all issues

    Should be inserted alphabetically.

  9. 
      
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. 
      
  2. Show all issues

    What if this is empty?

    1. It will return the empty string because ''.split('\n') returns [''].

  3. 
      
chipx86
  1. 
      
  2. Show all issues

    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. Show all issues

    These need to be localized.

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

  4. Show all issues

    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. Show all issues

    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. Show all issues

    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. Show all issues

    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. Show all issues

    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:
Completed
Change Summary:
Pushed to dvcs (ba84d9d)