Fix issue where diffs fail to update when moving between revisions

Review Request #6261 — Created Aug. 24, 2014 and discarded

Information

Review Board
master
1837d7d...

Reviewers

Backbone.View's this.el is not getting initialized (perhaps intentionally, I cannot tell). The fix is to test for validity before using it.

Checked that loading pages with multiple revisions can be changed with slider in the process of producing diffs. Ran all unit tests.

reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
    
    
  2. 
      
david
  1. Can you provide some explanation of what you think this change accomplishes and how it fixes the bug?

    1. Hmm, I am apparently using rbt incorrectly. This is not the diff I intended to upload. I'll try again with one from a github fork.

      The problem is that Backbone.View's this.el is not getting initialized (perhaps intentionally, I cannot tell). The fix is to test for validity before using it.

  2. 
      
GU
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
    
    
  2. 
      
chipx86
  1. I don't understand the case where this happens. I haven't seen it myself.

    I'd need to see solid repro steps that I can trigger, and we should probably get a JavaScript unit test that demonstrates the behavior and shows it's fixed.

    1. Complete reproduction steps are available on the bug description. It is visible on this site (reviews.reviewboard.org). Reproduced here from the bug:

      https://reviews.reviewboard.org/r/5869/diff

      1. Scroll down to the diff slider controls
      2. Move left the slider to position 1 (load diff 1-14)
      3. Before all files can complete loading (ajax spinners still show), move the slider to position 2 (load diff/2-14)
    2. 2 should read 'Move the left slider...'

  2. 
      
david
  1. I'm going to fix this bug in a slightly different way (cancelling the diff view at a higher level rather than just fixing up the invalid accesses).

  2. 
      
GU
Review request changed
Status:
Discarded