Summary

Render the diff change index from JS instead of Django.

Review Request #4545 — Created Sept. 9, 2013 and submitted Sept. 11, 2013, 11:23 p.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Render the diff change index from JS instead of Django.

The new change index was about half template half javascript--we'd render the
basic table with each of the files from a django template, and then as diffs
were loaded, update it.

This change moves the initial render into the javascript as well. The data is
plumbed into javascript by passing it into the DiffViewerPageView. This change
is one step towards being able to update the displayed diff (revision/page/etc)
without reloading everything.

Testing Done


Viewed a handful of diff revisions and interdiffs. Saw the table as before in

  each case.
Ran js-tests
Ran jshint

Issues

Description	From	Last Updated
'from settings_local import *' used; unable to detect undefined names	reviewbot	Sept. 9, 2013, 5:11 p.m.
<table/>, for consistency.	chipx86	Sept. 10, 2013, 6:50 p.m.
Where does print come from? Is that something from underscore?	chipx86	Sept. 10, 2013, 6:25 p.m.
!==	chipx86	Sept. 10, 2013, 6:50 p.m.
!==	chipx86	Sept. 10, 2013, 6:50 p.m.
Can probably be one line.	chipx86	Sept. 10, 2013, 6:50 p.m.
This feels weird. Why not just use <% %> as usual for these?	chipx86	Sept. 10, 2013, 6:51 p.m.
The indentation level for the for statement doesn't seem to match where it should be respective to other tags. Same …	chipx86	Sept. 10, 2013, 6:52 p.m.
Make sure we're escaping for JavaScript. We had a security hole due to exactly this once.	chipx86	Sept. 10, 2013, 6:52 p.m.
Here too.	chipx86	Sept. 10, 2013, 6:52 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Sept. 10, 2013, 6:54 p.m.
The $ should probably be #, I'm guessing?	chipx86	Sept. 11, 2013, 4:17 p.m.
These probably need to be escaped too, right? Not sure I see much reason to use print() over underscore's standard …	chipx86	Sept. 11, 2013, 4:20 p.m.
On the model, revision is an integer. We may as well reflect that here.	chipx86	Sept. 11, 2013, 4:21 p.m.
Same here.	chipx86	Sept. 11, 2013, 4:21 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Sept. 11, 2013, 4:23 p.m.

This is a review from Review Bot.
Tool: PEP8 Style Checker
Processed Files:
reviewboard/settings.py
Ignored Files:
reviewboard/static/rb/js/diffviewer/models/diffFileModel.js
reviewboard/templates/diffviewer/view_diff.html
reviewboard/static/rb/js/diffviewer/collections/diffFileCollection.js
reviewboard/templates/diffviewer/changeindex.html
reviewboard/static/rb/js/pages/views/diffViewerPageView.js

This is a review from Review Bot.
Tool: Pyflakes
Processed Files:
reviewboard/settings.py
Ignored Files:
reviewboard/static/rb/js/diffviewer/models/diffFileModel.js
reviewboard/templates/diffviewer/view_diff.html
reviewboard/static/rb/js/diffviewer/collections/diffFileCollection.js
reviewboard/templates/diffviewer/changeindex.html
reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/settings.py (Diff revision 1)

Show all issues

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

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1)
Show all issues
```
<table/>, for consistency.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1)
Show all issues
```
Where does print come from? Is that something from underscore?
```
1. david Sept. 10, 2013, 6:26 p.m.
  Yeah.
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1)
Show all issues
```
!==
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1)
Show all issues
```
!==
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1)
Show all issues
```
Can probably be one line.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1)
Show all issues
```
This feels weird. Why not just use <% %> as usual for these?
```

reviewboard/templates/diffviewer/view_diff.html (Diff revision 1)

Show all issues

The indentation level for the for statement doesn't seem to match where it should be respective to other tags.

Same with other template tags here.

david Sept. 10, 2013, 6:26 p.m.

I really, really don't understand your system for indenting these :P

chipx86 Sept. 10, 2013, 6:45 p.m.

I'm not sure it's a great system, honestly, but I think in ways, it's better than other methods. Even if it's sometimes a bit weird or confusing.

HTML tags are aligned respective to each other. JavaScript code is aligned respective to each other. Template tags are aligned respective to each other. They do not interfere with each others' alignment.

Some projects treat template tags like HTML tags and align relative to that. It works in simple cases. However, that doesn't really mesh with template tags in javascript, or other non-HTML output, though, and also has issues when you want to do something like:

<div class="foo">
 {% if foo %}
  </div>
  <div class="bar">
 {% endif %}

The indentation is weird both here and in output.

So, since that indentation model doesn't work cleanly when mixing with outputted content, I prefer to keep template tag indentation consistent and pretend everything inside of it is a print statement of sorts.

In the case above, it would be:

{% block ... %}
...
{%  for file in files %}

Then, to minimize excess whitespace in output (which leads to more content to transfer), the indentation can go in {% .. %}.

So those are my thoughts on it, and what I've been aiming for. A lot of our templates are just not consistent though, but I want to get closer to some level of consistency.

reviewboard/templates/diffviewer/view_diff.html (Diff revision 1)

Show all issues

Make sure we're escaping for JavaScript. We had a security hole due to exactly this once.

reviewboard/templates/diffviewer/view_diff.html (Diff revision 1)
Show all issues
```
Here too.
```

Diff:

Revision 2 (+115 -37)

Show changes

	reviewboard/settings.py
	reviewboard/static/rb/js/diffviewer/collections/diffFileCollection.js
	reviewboard/static/rb/js/diffviewer/models/diffFileModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/templates/diffviewer/changeindex.html
	reviewboard/templates/diffviewer/view_diff.html

reviewboard/settings.py (Diff revision 2)

Show all issues

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

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 2)
Show all issues
```
The $ should probably be #, I'm guessing?
```

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

Show all issues

These probably need to be escaped too, right?

Not sure I see much reason to use print() over underscore's standard templating.

david Sept. 11, 2013, 4:22 p.m.

They do... sigh. The print calls feel more readable when there's a ton of jumping back and forth between js and template.

chipx86 Sept. 11, 2013, 10:23 p.m.
```
It does, I'll agree.
```

reviewboard/templates/diffviewer/view_diff.html (Diff revision 2)

Show all issues

On the model, revision is an integer. We may as well reflect that here.

reviewboard/templates/diffviewer/view_diff.html (Diff revision 2)
Show all issues
```
Same here.
```

Diff:

Revision 3 (+115 -37)

Show changes

	reviewboard/settings.py
	reviewboard/static/rb/js/diffviewer/collections/diffFileCollection.js
	reviewboard/static/rb/js/diffviewer/models/diffFileModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/templates/diffviewer/changeindex.html
	reviewboard/templates/diffviewer/view_diff.html

Fix it!

reviewboard/settings.py (Diff revision 3)

Show all issues

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

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (fa41428).