File Attachment Revision Slider

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

rmdone
Review Board
master
884ccba...
reviewboard, students

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.

Loading file attachments...

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)
     
     
    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. Should only have one var statement.

  3. 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.

  4. 
      
chipx86
  1. 
      
  2. 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)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
RM
RM
RM
brennie
  1. 
      
  2. 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)
     
     

    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)
     
     
     

    Alphabetical order.

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

    Blank line between these.

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

    Indentation error on the mark_safe line.

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

    And between these.

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

    Blank line here.

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

    And here.

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

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

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

    Blank line here.

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

    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. This is a bit long. How about just _revisionSelectorView?

  13. Arguments must align.

  14. Looks like a missing ` on base.

  15. No blank line here.

  16. This should go before the variables without values.

  17. Blank line here.

  18. 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.

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

  20. Same comment about the name.

  21. Same comment about alignment.

    Above comments also all apply to this file as well.

  22. 
      
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...