Make the "Diff Revision" label dynamically rendered in javascript.

Review Request #4555 — Created Sept. 10, 2013 and submitted

Information

Review Board
master

Reviewers

Make the "Diff Revision" label dynamically rendered in javascript.

In order to be able to update the displayed revision, this content needs to be
generated on the client side instead of the server.

  • Viewed various revisions of a diff, including interdiffs. Saw the expected
    text each time.
  • Checked that the "latest diff" and "See what's changed" links worked
    correctly.
  • Ran js-tests.
  • Ran jshint.
Description From Last Updated

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

The <h1> shouldn't be part of the localized string.

chipx86chipx86

Same.

chipx86chipx86

It doesn't seem that all this should be localized. The "Diff revision XYZ" should, and the "This is not ..." …

chipx86chipx86

This should also be two strings, and should not involve tags. That's a layout thing for a template to deal …

chipx86chipx86

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

This is the only detail string without tags. Do we want to make sure we escape here?

chipx86chipx86

Maybe just have a string for the revision, and build the common parts after (or in the call to window.location). …

chipx86chipx86

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    reviewboard/settings.py
    Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
    reviewboard/static/rb/js/diffviewer/models/diffRevisionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

  2. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    reviewboard/settings.py
    Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
    reviewboard/static/rb/js/diffviewer/models/diffRevisionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

  2. reviewboard/settings.py (Diff revision 1)
     
     

    'from settings_local import *' used; unable to detect undefined names

  3. 
      
chipx86
  1. 
      
  2. The <h1> shouldn't be part of the localized string.

  3. It doesn't seem that all this should be localized. The "Diff revision XYZ" should, and the "This is not ..." should, and the "See what's changed" should, but not as one big string.

    The layout of this is going to change quite a lot soon anyway.

    1. I'll split the header and paragraph, but I disagree about splitting apart the sentences entirely or completely pulling tags out of the strings. I plan to re-do this all after making a new revision selector, though (it's crummy that it reflows the page).

    2. Completely pulling out tags isn't necessarily, but formatting and layout are different. I guess in this case, "See what's changed" is fine being part of the same sentence, but if we decided to change layout, we can affect the strings. No big deal for now though.

      Let's talk about the revision selector offline. I've spent some time thinking about it and making mockups, so I want to sync up on that.

  4. This should also be two strings, and should not involve tags. That's a layout thing for a template to deal with.

  5. 
      
david
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    reviewboard/settings.py
    Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
    reviewboard/static/rb/js/diffviewer/models/diffRevisionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

  2. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    reviewboard/settings.py
    Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
    reviewboard/static/rb/js/diffviewer/models/diffRevisionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

  2. reviewboard/settings.py (Diff revision 2)
     
     

    'from settings_local import *' used; unable to detect undefined names

  3. 
      
david
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    reviewboard/settings.py
    Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
    reviewboard/static/rb/js/diffviewer/models/diffRevisionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

  2. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    reviewboard/settings.py
    Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
    reviewboard/static/rb/js/diffviewer/models/diffRevisionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

  2. reviewboard/settings.py (Diff revision 3)
     
     

    'from settings_local import *' used; unable to detect undefined names

  3. 
      
david
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    reviewboard/settings.py
    Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
    reviewboard/static/rb/js/diffviewer/models/diffRevisionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

  2. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    reviewboard/settings.py
    Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
    reviewboard/static/rb/js/diffviewer/models/diffRevisionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

  2. reviewboard/settings.py (Diff revision 4)
     
     

    'from settings_local import *' used; unable to detect undefined names

  3. 
      
chipx86
  1. 
      
  2. reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    So here's another issue to figure out involving localization and tags and such.

    In the main template, we're using <%= detail %>, so we won't get escaping. This is clearly done because the other templates contain tags, which is fine.

    Now, way we localize and get some characters in the strings that need to be escaped. We now have no mechanism to escape them, so the translated strings themselves will have to be escaped. That means we have to trust the strings to never be wrong.

    What are your thoughts on that?

    1. I'm going to add translator comments here to explain what to do.

    2. Also note that while I haven't thought too much about it, I do want to replace all of this later. For now I wanted to keep the same functionality while enabling reload-free revision switches.

    3. Sure, that's fine. It's more of a higher-level discussion I think.

  3. Any reason we don't just operate on this.$el instead?

    (I'm assuming it's for expansion, but just wanted to check.)

  4. This is the only detail string without tags. Do we want to make sure we escape here?

  5. reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 4)
     
     
     
     
     
     
     
     

    Maybe just have a string for the revision, and build the common parts after (or in the call to window.location). No sense in duplicating the diff/ and #index_header parts.

    1. This code is extremely temporary, and will be going away when I eliminate the reload. We'll have something similar for updating the history, but I'm doing to defer improving it until then.

    2. Sure.

  6. 
      
david
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    reviewboard/settings.py
    Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
    reviewboard/static/rb/js/diffviewer/models/diffRevisionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

  2. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    reviewboard/settings.py
    Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/views/diffRevisionLabelView.js
    reviewboard/static/rb/js/diffviewer/models/diffRevisionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

  2. reviewboard/settings.py (Diff revision 5)
     
     

    'from settings_local import *' used; unable to detect undefined names

  3. 
      
chipx86
  1. Ship It!

  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (f9f8751).

Loading...