• 
      

    [WIP] Add ability to collapse entire files in the diff view.

    Review Request #9337 — Created Oct. 27, 2017 and discarded

    Information

    Review Board
    release-3.0.x

    Reviewers

    When viewing the diff of a review request not all files need to be
    viewed, for example minified files don't need to be reviewed. It is
    also helpful to have a way to hide files you have already reviewed.

    Now files can be collapsed and expanded as the user wishes. Also,
    minified files are collapsed by default.

    In a previous commit clicking the new button whould hide the diff
    information as well as the links to download the file. I have changed
    it no longer hide the download links so that it matches files hidden
    with the "Hide/Show whitespace changes" button.

    I modified the function toggleWhitespaceOnlyChunks in
    diffReviewableView.es6.js so it now takes in a boolean value to indicate
    if the files should be shown or hidden. I made this change because I needed
    the "Hide/Show whitespace changes" button to work with the button to hide/show
    a specific file, and without specifying if files should be shown or hidden it
    would have caused issues where clicking "Hide whitespace changes" would hide
    some but show others if they were already individually hidden with the new
    button.

    Unit tests have not been written yet.

    Also it might be worth mentioning that because the existing
    "Collapse changes" button reloads the page, any files that were
    manually collapsed or expanded will return to their default state.


    Description From Last Updated

    If there's a newly-added minified filename, I think depot_filename will be None, which might crash. This should also use single …

    daviddavid

    This comment should have an "Args" section explaining what e is.

    daviddavid

    Because this is ES6, we can use a slightly shorter syntax: _toggleCollapseDiff(e) {

    daviddavid

    These should use const instead of var.

    daviddavid

    Should be indented only 1 space within its parent.

    daviddavid

    E127 continuation line over-indented for visual indent

    reviewbotreviewbot

    E127 continuation line over-indented for visual indent

    reviewbotreviewbot

    This is really difficult to read. Since collapse_by_default is a boolean value, I think it makes more sense to assign …

    bleblan2bleblan2

    E127 continuation line over-indented for visual indent

    reviewbotreviewbot

    E127 continuation line over-indented for visual indent

    reviewbotreviewbot

    Looking at the other if tags in the file, they all have a space before closing the tag: ...show_deleted %}

    bleblan2bleblan2

    E128 continuation line under-indented for visual indent

    reviewbotreviewbot

    E128 continuation line under-indented for visual indent

    reviewbotreviewbot

    E128 continuation line under-indented for visual indent

    reviewbotreviewbot

    E128 continuation line under-indented for visual indent

    reviewbotreviewbot

    E126 continuation line over-indented for hanging indent

    reviewbotreviewbot

    Col: 22 Expected '===' and instead saw '=='.

    reviewbotreviewbot

    Col: 46 Missing semicolon.

    reviewbotreviewbot
    david
    1. 
        
    2. reviewboard/diffviewer/diffutils.py (Diff revision 1)
       
       
      Show all issues

      If there's a newly-added minified filename, I think depot_filename will be None, which might crash. This should also use single quotes, and probably test for .min..

      1. I did some of my dev testing with minified files that I added into an empty repo and didn't have any issues with it being a newly added file. Please let me know if this doesn't cover the situation you had in mind.

    3. Show all issues

      This comment should have an "Args" section explaining what e is.

    4. Show all issues

      Because this is ES6, we can use a slightly shorter syntax:

      _toggleCollapseDiff(e) {
      
    5. Show all issues

      These should use const instead of var.

    6. Show all issues

      Should be indented only 1 space within its parent.

    7. 
        
    RC
    TB
    1. 
        
    2. Don't know if this is helpful at all, or even a part of the requirements of your project, but if you did want to maintain the collapsed state of your elements even after page reload, you could save a list of collapsed file ids under an object in browser storage (probably local storage) indexed by the current review request id. If the list under the review request id exists, use it to determine which elements should be collapsed on client-side loading. If it doesn't, remove all other currently saved review request collapsed-lists (to not use up a bunch of local storage space).

    3. 
        
    RC
    Review request changed
    Change Summary:

    Revamped the collapsing logic to work better with cases like deleted files and whitespace changes

    Commit:
    02d1efa20482b3a4b43a5b9aaaf61691ae20ae49
    30d79da1a372d48d4df907b78bcae686a1e8f36a

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    RC
    RC
    Review request changed
    Change Summary:

    Added the new button to the diffs for files that have been moved or copied

    Commit:
    30d79da1a372d48d4df907b78bcae686a1e8f36a
    59b1cff70918b1dba86a7bcf318fd89e17521b3d

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    RC
    bleblan2
    1. Just a couple code formatting things I noticed.

    2. reviewboard/diffviewer/renderers.py (Diff revision 4)
       
       
      Show all issues

      This is really difficult to read. Since collapse_by_default is a boolean value, I think it makes more sense to assign it as a boolean instead of changing it's value in an if statement.

      I'm thinking something like this:

              collapse_by_default = (self.diff_file['minified'] and
                                     not self.diff_file['binary'] and
                                     not self.diff_file['deleted'] and
                                     (not self.diff_file['moved_or_copied'] or
                                      self.diff_file['num_changes'] != 0) and
                                     (not self.diff_file['new_file'] or
                                      self.diff_file['num_chunks'] != 0))
      
    3. Show all issues

      Looking at the other if tags in the file, they all have a space before closing the tag: ...show_deleted %}

    4. 
        
    RC
    Review request changed
    Change Summary:

    Fixed bugs created earier in this project

    Commit:
    59b1cff70918b1dba86a7bcf318fd89e17521b3d
    d1e05cef275de1760d2176fa878e73e5c5742036

    Checks run (2 failed)

    flake8 failed.
    JSHint failed.

    flake8

    JSHint

    david
    Review request changed
    Status:
    Discarded