• 
      

    Rework page navigation state for the diff viewer.

    Review Request #9717 — Created March 2, 2018 and submitted

    Information

    Review Board
    release-3.0.x
    9772f82...

    Reviewers

    We use Backbone.Router and URLs to set the current view of the diff
    viewer, controlling which revisions and pages are shown. Currently, this
    is pretty basic -- we only need to worry about revisions in the path,
    and a single query string option for the page. This means that any bits
    of code updating this view state just needed to have the router navigate
    to a new URL. Soon, though, we'll need more query string options, and we
    weren't set up for this at all. This change aims to fix that.

    There's now a central _navigate() method, which takes care of setting
    the URL state based on the options provided and the existing page state.
    It's the source of truth on how page URLs set within the view should
    look. All call sites now call into this, instead of working with URLs
    themselves.

    The router itself now supports arbitrary query strings. This means
    switching to our own regex, instead of using their capture groups (which
    don't actually work well for more general usage -- there's lots of
    workarounds online, but this seems to be the best option). While the
    regex is a bit more complicated, the body of the handler is now a bit
    simpler, and will be easier to expand with more diff loading options
    going forward.

    DiffViewerPageModel was updated to use the new Djblets.buildURL()
    function for constructing the URL used to load the diff context, which
    will also help us with expanding usage soon.

    Unit tests were added that test the various navigation combinations,
    both in a generic way (testing _navigate()) and more specific ways
    (testing the handlers for revision/page selection, anchor
    navigation, and initial page states).

    All unit tests pass.

    Manually tested changing revisions (testing all combinations of single
    and interdiff revisions), changing pages, navigating anchors, and setting
    initial pages. The page state and the current URL always reflected what I
    expected to see.

    Description From Last Updated

    Col: 35 ['page'] is better written in dot notation.

    reviewbot reviewbot

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

    reviewbot reviewbot

    Col: 26 ['page'] is better written in dot notation.

    reviewbot reviewbot

    Instead of having these be optional parameters which are only used in unit tests, how about changing the only real …

    david david
    Checks run (1 failed, 1 succeeded)
    flake8 passed.
    JSHint failed.

    JSHint

    chipx86
    david
    1. 
        
    2. reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2)
       
       
       
       
       
       
       
       
      Show all issues

      Instead of having these be optional parameters which are only used in unit tests, how about changing the only real callsite to be:

      this._setInitialURL(document.location.search || '',
                          RB.getLocationHash());
      

      That way the file is a few lines shorter and there's no longer any confusion about why the parameters exist when they're not used by the actual code.

    3. 
        
    chipx86
    david
    1. Ship It!
    2. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-3.0.x (5ea13f0)