File Attachment Revision Slider

Review Request #6591 — Created Nov. 15, 2014 and submitted

Information

Review Board
master
884ccba...

Reviewers

The slider is now functional for image attachments and text attachments (including rendered text), initiating the controls for different diff modes. The slider actions trigger a hard redirect for now (window.location.replace).
With enough time I may be able to do a soft redirect using router and ajax (like _loadRevision in diffViewerPageView.js)

The URL argument is what is used to specify the files being diffed. ".../r/<rrID>/<FileAttachmentID>-<DiffID>/"

The first option in the slider is the No Diff option, which will redirect to
.../r/<rrID>/<FileAttachmentID>/
This is how switching back to single files works.
This mirrors the way the slider works in review requests. (diffRevisionSelectorView.js).

A title has been added to the slider that behaves the same as the title for the diffviewer revision slider. The class implementation is more or less the same except is refers to the fileAttachmentRevisionModel instead of the diffRevisionModel.

Clicking on labels other than the no diff will redirect to that single file view.

I'm having trouble changing the text attachment title header to have each file title allign with their respective file, so for now they are just spaced out with an & symbol.

TODO:
- Text File title allignment (columns?)
- CSS Spacing issues?

For another Review Request
- Adding coverage to unit tests
- soft redirect

Verified by hand that url argument <ID>-<diffID> throws a 404 if the two IDs do not share attachment history for both images and text attachments.

All unit tests pass

Manually tested the slider for text, markdown and images.


Description From Last Updated

Why does the revision slider make the footer of the table beige? Can it be modified so that the footer …

brenniebrennie

Col: 80 E501 line too long (188 > 79 characters)

reviewbotreviewbot

Should only have one var statement.

chipx86chipx86

Spaces around the -.

chipx86chipx86

This would be easier to read if you only did the conditional once, and computed the proper set of values …

chipx86chipx86

Here too.

chipx86chipx86

I don't think the default UI should be deciding where this goes, or should even place it anywhere. It should …

chipx86chipx86

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

We're doing this twice. Best to get the results once, convert to a list, then use it both here and …

chipx86chipx86

Alphabetical order.

chipx86chipx86

Blank line between these.

chipx86chipx86

Indentation error on the mark_safe line.

chipx86chipx86

And between these.

chipx86chipx86

Blank line here.

chipx86chipx86

And here.

chipx86chipx86

Can you add comments on exactly what's happening here? It's unclear.

chipx86chipx86

Blank line here.

chipx86chipx86

We're doing two requests needlessly here, and database requests are expensive. I think we should just do a get_object_or_404 with …

chipx86chipx86

Arguments should align.

chipx86chipx86

This is a bit long. How about just _revisionSelectorView?

chipx86chipx86

Arguments must align.

chipx86chipx86

Looks like a missing ` on base.

chipx86chipx86

No blank line here.

chipx86chipx86

This should go before the variables without values.

chipx86chipx86

Blank line here.

chipx86chipx86

Multi-line comments should use the: /* * */ form.

chipx86chipx86

Let's compute a URL once, then use window.location.replace().

chipx86chipx86

Same comment about the name.

chipx86chipx86

Same comment about alignment. Above comments also all apply to this file as well.

chipx86chipx86
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/ui/base.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/templates/reviews/ui/default.html
        reviewboard/static/rb/js/views/revisionSelectorView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/ui/base.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/templates/reviews/ui/default.html
        reviewboard/static/rb/js/views/revisionSelectorView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
    
    
  2. reviewboard/reviews/ui/base.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (188 > 79 characters)
    
  3. 
      
RM
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/ui/base.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/templates/reviews/ui/default.html
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/ui/base.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/templates/reviews/ui/default.html
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
    
    
  2. 
      
chipx86
  1. 
      
  2. Show all issues

    Should only have one var statement.

  3. Show all issues

    Spaces around the -.

  4. Show all issues

    This would be easier to read if you only did the conditional once, and computed the proper set of values for this._values once depending on that.

  5. Show all issues

    Here too.

  6. 
      
chipx86
  1. 
      
  2. Show all issues

    I don't think the default UI should be deciding where this goes, or should even place it anywhere. It should be up to the specific review UI implementation to create an element for use in the revision slider.

  3. 
      
RM
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/ui/base.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/ui/base.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
    
    
  2. 
      
RM
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/ui/base.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/ui/base.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
    
    
  2. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/urls.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/urls.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
    
    
  2. 
      
RM
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/ui/text.py
        reviewboard/reviews/urls.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/ui/text.py
        reviewboard/reviews/urls.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
    
    
  2. 
      
RM
RM
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/ui/text.py
        reviewboard/reviews/urls.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/ui/text.py
        reviewboard/reviews/urls.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
    
    
  2. reviewboard/reviews/ui/text.py (Diff revision 7)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
RM
RM
RM
brennie
  1. 
      
  2. Show all issues

    Why does the revision slider make the footer of the table beige? Can it be modified so that the footer won't be modified in this way?

    1. I changed the position of the slider to above the file attachment, something that was suggested in the meeting.

  3. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/ui/text.py
        reviewboard/reviews/urls.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/pages/text-review-ui.less
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/ui/text.py
        reviewboard/reviews/urls.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/pages/text-review-ui.less
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/ui/base.py (Diff revision 8)
     
     
    Show all issues

    We're doing this twice. Best to get the results once, convert to a list, then use it both here and for the IDs list.

  3. reviewboard/reviews/ui/text.py (Diff revision 8)
     
     
     
    Show all issues

    Alphabetical order.

  4. reviewboard/reviews/ui/text.py (Diff revision 8)
     
     
     
    Show all issues

    Blank line between these.

  5. reviewboard/reviews/ui/text.py (Diff revision 8)
     
     
     
     
    Show all issues

    Indentation error on the mark_safe line.

  6. reviewboard/reviews/ui/text.py (Diff revision 8)
     
     
     
    Show all issues

    And between these.

  7. reviewboard/reviews/ui/text.py (Diff revision 8)
     
     
     
    Show all issues

    Blank line here.

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

    And here.

  9. reviewboard/reviews/ui/text.py (Diff revision 8)
     
     
     
     
     
    Show all issues

    Can you add comments on exactly what's happening here? It's unclear.

  10. reviewboard/reviews/ui/text.py (Diff revision 8)
     
     
     
    Show all issues

    Blank line here.

  11. reviewboard/reviews/views.py (Diff revision 8)
     
     
     
     
     
     
    Show all issues

    We're doing two requests needlessly here, and database requests are expensive. I think we should just do a get_object_or_404 with the entire query in the if statement.

  12. Show all issues

    Arguments should align.

  13. Show all issues

    This is a bit long. How about just _revisionSelectorView?

  14. Show all issues

    Arguments must align.

  15. Show all issues

    Looks like a missing ` on base.

  16. Show all issues

    No blank line here.

  17. Show all issues

    This should go before the variables without values.

  18. Show all issues

    Blank line here.

  19. Show all issues

    Multi-line comments should use the:

    /*
     *
     */
    

    form.

    1. I fixed it, but this will likely be a comment I trim out before this change gets finalized.

  20. Show all issues

    Let's compute a URL once, then use window.location.replace().

  21. Show all issues

    Same comment about the name.

  22. Show all issues

    Same comment about alignment.

    Above comments also all apply to this file as well.

  23. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/ui/text.py
        reviewboard/reviews/urls.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/pages/text-review-ui.less
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/ui/text.py
        reviewboard/reviews/urls.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/pages/text-review-ui.less
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
    
    
  2. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/ui/text.py
        reviewboard/reviews/urls.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/pages/text-review-ui.less
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionLabelView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/ui/base.py
        reviewboard/reviews/ui/text.py
        reviewboard/reviews/urls.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/css/pages/text-review-ui.less
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js
        reviewboard/static/rb/js/views/fileAttachmentRevisionLabelView.js
    
    
  2. 
      
RM
david
  1. Ship It!
  2. 
      
RM
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to ucosp/rmdone/file-attachment-revisions (c73a7ad)
Loading...