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.

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

daviddavid
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)
     
     
     
     
     
     
     
     

    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: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (5ea13f0)
Loading...