Summary:

Add Bookmarking feature to Diff Viewer.

Review Request #7658 — Created Sept. 25, 2015 and updated Feb. 28, 2023, 2:58 p.m.

Information
Owner:	david
Repository:	Review Board
Branch:	master
Bugs:
Depends On:
Reviewers
Groups:	reviewboard, students
People:

Description

Add bookmarking feature in diff viewer so users can create and quickly jump
between bookmarks. Intended to help users moving back and forth between
different parts in the diff viewer when doing code reviews.

A "ghost" bookmark will follow the cursor on the right margin of the diff
viewer, which a user can click on to create new bookmarks on the hovered line
of the diff file. Clicking on the created bookmark will scroll the window
to where the bookmarked line is. Bookmarks are saved per diff using browser
Cookie, and will persist for one week.

Usability features include assigning different colors to bookmarks for easy
differentiation, positioning bookmarks on where the target line is relative
to the whole page, and animating the position at bookmark creation so users
can see where the newly created bookmark will reside.

Testing Done

Automated JS test cases:

Ghost bookmark
- should move along with the cursor on the diff
- becomes invisible when the cursor is not on diff
- expands when the cursor approaches
- shrinks when the cursor moves away
- creates a new bookmark when clicked on it
- no ghost bookmark when there already is a bookmark on the hovered line

Bookmarks
- appears in front of ghost bookmark
- expands when the cursor approaches
- shrinks when the cursor moves away
- will animate on creation
- on file header should only display the filename
- on a line in a file should display the filename and the line number
- on an expanded chunk will hide when the chunk is collapsed
- on a collapsed chunk will appear when the chunk is expanded
- should have different colors (for first 10 bookmarks)
- will be removed when clicked on [x]
- will scroll the page when clicked
- can be created from cookies
- will trigger cookie save event on creation or removal

Following features are tested manually on local devserver:

Bookmarks
- on an empty line should display nicely
- on a line that wraps on browser as multiple lines should display nicely
- added bookmark will appear again after refreshing on the page
- removed bookmarks will not appear again after refreshing on the page
- should be positioned where the main bookmark is relative to the whole page
- are repositioned when window is resized
- are repositioned when a chunk is expanded/collapsed
- are repositioned when a file is loaded

Files

Issues

Description	From	Last Updated
Space after selector.	brennie	Oct. 2, 2015, 6:46 p.m.
Alphabetize these.	brennie	Oct. 2, 2015, 6:46 p.m.
We should set a constant in defs.less for this. e.g. @ghost-bookmark-background-color: rgba(173, 173, 173, 0.5);	brennie	Oct. 2, 2015, 6:46 p.m.
Can we call this RB.MookmarkModel?	brennie	Oct. 2, 2015, 6:46 p.m.
There should be a defaults object that contains safe default values (although fileName would probably be null in that case)	brennie	Oct. 2, 2015, 6:46 p.m.
diffID	brennie	Oct. 2, 2015, 6:46 p.m.
interdiffID	brennie	Oct. 2, 2015, 6:46 p.m.
We really don't want to have models stored in models. Backbone doesn't support it that well. Instead, you can pass …	brennie	Oct. 2, 2015, 6:46 p.m.
This should be in collections/bookmarkCollection.js	brennie	Oct. 2, 2015, 6:46 p.m.
Likewise, RB.BookmarkCollection ?	brennie	Oct. 2, 2015, 6:46 p.m.
Missing semicolon.	brennie	Oct. 2, 2015, 6:46 p.m.
RB.BookmarkContainerView Also, this should be in its own separate file.	brennie	Oct. 2, 2015, 6:46 p.m.
We use lowerCamelCase for JS names and hyphen-case for HTML names, so this should be bookmark-container.	brennie	Oct. 2, 2015, 6:46 p.m.
Is this supposed to return something? If not, we do not need the return statement.	brennie	Oct. 10, 2015, 6:44 p.m.
Please format this like: if (!noRender) { bookmarkerView.render(); }	brennie	Oct. 2, 2015, 6:46 p.m.
We use one var statement as the first statement in any function.	brennie	Oct. 2, 2015, 6:46 p.m.
You are likely going to want to use _.throttle here. Listening for mousemoves is expensive.	brennie	Oct. 10, 2015, 6:44 p.m.
We put a space between the ) and the {. Please fix this throughout.	david	Oct. 17, 2015, 8:33 p.m.
Should be bookmark (not Bookmark)	david	Oct. 17, 2015, 8:33 p.m.
This line isn't necessary.	david	Oct. 17, 2015, 8:33 p.m.
Variables should start with a lower-case letter.	david	Oct. 17, 2015, 8:34 p.m.
You don't need the !! here since it's just a truthy-test. All conditionals should also have {}, regardless of whether …	david	Oct. 17, 2015, 8:34 p.m.
It's probably better to format this with one call per line: this.$ghostBookmark .removeClass('open') .find('.bookmark-fold') .css('width', 0);	david	Oct. 17, 2015, 8:34 p.m.
This stuff should probably happen in render rather than initialize.	david	Oct. 17, 2015, 8:34 p.m.
Make sure you're indenting 4 spaces in JavaScript code.	david	Oct. 17, 2015, 8:34 p.m.
Make sure you're indenting 4 spaces in JavaScript code.	david	Oct. 17, 2015, 8:34 p.m.
The ?: operator will already convert to a bool, so you don't need !!	david	Oct. 17, 2015, 8:34 p.m.
You can use _.bind here, too.	david	Oct. 17, 2015, 8:34 p.m.
This should be the last line in the file. This is a vim modeline which gives the editor settings for …	brennie	Nov. 7, 2015, 7:48 p.m.
You can nest this in the .close rule with &:hover.	brennie	Nov. 7, 2015, 7:48 p.m.
You should use .transition(). It does a prefixed version of transition.	brennie	Nov. 7, 2015, 7:48 p.m.
"the diffviewer" Needs a period.	brennie	Nov. 7, 2015, 7:48 p.m.
I'm not sure if the cookie data should be passed in via options or not. You should maybe ping Christian …	brennie	Nov. 7, 2015, 7:49 p.m.
Put these on seperate lines.	brennie	Nov. 7, 2015, 7:49 p.m.
Remove this comment.	brennie	Nov. 7, 2015, 7:49 p.m.
We use jquery.cookie. You can use it for reading/writing cookies. Check out the docs	brennie	Nov. 7, 2015, 7:49 p.m.
Can we call this cookie ?	brennie	Nov. 7, 2015, 7:49 p.m.
Blank line between these.	brennie	Nov. 7, 2015, 7:49 p.m.
Remove this comment.	brennie	Nov. 7, 2015, 7:49 p.m.
Replace this with a for loop.	brennie	Nov. 7, 2015, 7:49 p.m.
Since we're not actually transmuting the models or options, I feel like we should do this in a signal handler. …	brennie	Nov. 7, 2015, 7:49 p.m.
Needs a doc comment.	brennie	Nov. 7, 2015, 7:50 p.m.
Put these on seperate lines, with defined variables first.	brennie	Nov. 7, 2015, 7:50 p.m.
Can we call this bookmark ?	brennie	Nov. 7, 2015, 7:50 p.m.
remove this comment.	brennie	Nov. 7, 2015, 7:50 p.m.
This overwrites all of the cookie that is set, doesn't it? We really should use $.cookie here.	brennie	Nov. 7, 2015, 7:50 p.m.
Needs a period.	brennie	Nov. 7, 2015, 7:51 p.m.
Needs a doc comment.	brennie	Nov. 7, 2015, 7:51 p.m.
Just call it template. That is the convention we use.	brennie	Nov. 7, 2015, 7:51 p.m.
Space after , between items.	brennie	Nov. 7, 2015, 7:51 p.m.
Remove this comment.	brennie	Nov. 7, 2015, 7:51 p.m.
This should use sentence casing, e.g., begin with a capital and end with a period. "from a clone of the"	brennie	Nov. 7, 2015, 7:51 p.m.
Missing a var statement. e isn't a descriptive variable name; we should use bookmark instead.	brennie	Nov. 7, 2015, 7:51 p.m.
Since you're doing a filter & map on the values and only using them once, we can just do a …	brennie	Nov. 7, 2015, 7:51 p.m.
Backbone.Model.get only takes one key at a time, so this should be: e.get('fileDiffID'), e.get('interFileDiffID')	brennie	Nov. 7, 2015, 7:51 p.m.
If this is private it should be prefixed with an underscore. Also, this needs a doccomment.	brennie	Nov. 14, 2015, 3:15 p.m.
Remove this line.	brennie	Nov. 7, 2015, 7:52 p.m.
view is more readable than bview.	brennie	Nov. 7, 2015, 7:52 p.m.
We format multi-line comments in JS as follows: /* * Line 1. * Line 2. * ... */	brennie	Nov. 7, 2015, 7:52 p.m.
You can do view.remove().render() because remove should return the view.	brennie	Nov. 21, 2015, 7:19 p.m.
Same comment as above regards to public/private-ness and doc comment.	brennie	Nov. 14, 2015, 3:17 p.m.
Should be prefixed with underscore. Needs doc comment.	brennie	Nov. 14, 2015, 3:16 p.m.
Should be prefixed with underscore. Needs doc comment.	brennie	Nov. 14, 2015, 3:16 p.m.
Blank line between these.	brennie	Nov. 7, 2015, 7:53 p.m.
Doc comment.	brennie	Nov. 7, 2015, 7:53 p.m.
var $target = $(e.target), fileName, line, top;	brennie	Nov. 7, 2015, 7:53 p.m.
Blank line between statement and block.	brennie	Nov. 7, 2015, 7:53 p.m.
See above re: multi-line comments.	brennie	Nov. 7, 2015, 7:54 p.m.
This is quite hard to read. I'm not sure what nodeType == 3 means? Is there a constnat we can …	brennie	Nov. 14, 2015, 3:16 p.m.
Should be indented 4 spaces not 2.	brennie	Nov. 7, 2015, 7:54 p.m.
What do you mean, without updating obj?	brennie	Nov. 7, 2015, 7:54 p.m.
Sentence casing.	brennie	Nov. 7, 2015, 7:54 p.m.
} else {	brennie	Nov. 7, 2015, 7:54 p.m.
doc comment.	brennie	Nov. 7, 2015, 7:54 p.m.
Doc comment.	brennie	Nov. 7, 2015, 7:54 p.m.
We prefix jQuery objects, so this should be $diffViewElem. Also, you can shorten Elem to El.	brennie	Nov. 7, 2015, 7:54 p.m.
Needs doc comment.	brennie	Nov. 14, 2015, 3:17 p.m.
Should be indented 4.	brennie	Nov. 7, 2015, 7:54 p.m.
var fileDiff = file.get('filediff'), interFileDiff = file.get('interfilediff');	brennie	Nov. 14, 2015, 3:17 p.m.
We can write this more clearly as : return (filediff.id === fileDiffID && ((interFileDiff === null && interFileDiffID === null) …	brennie	Nov. 14, 2015, 3:17 p.m.
===	brennie	Nov. 14, 2015, 3:17 p.m.
Indent multiples of 4 spaces.	brennie	Nov. 7, 2015, 7:55 p.m.
Missing a period.	brennie	Nov. 7, 2015, 7:55 p.m.
Missing a period.	brennie	Nov. 7, 2015, 7:55 p.m.
Col: 27 W601 .has_key() is deprecated, use 'in'	reviewbot	Nov. 21, 2015, 7:19 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Nov. 26, 2015, 11:37 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Nov. 28, 2015, 1:36 p.m.
Can we rename the cookie to "diff-bookmarks" ? Or maybe we should just have it be "bookmarks" (as we may …	chipx86	Dec. 4, 2015, 8:55 p.m.
Just some tweaks to this. The argument name should include the type, and for request (a very common argument), we …	chipx86	Dec. 4, 2015, 8:55 p.m.
This will be faster as: bookmark_cookie = request.COOKIES.get('diffbookmarks', '')	chipx86	Dec. 4, 2015, 8:55 p.m.
We namespace these names, so these would be best as @bookmark-ghost-bg-color and @bookmark-default-bg-color, in alphabetical order.	chipx86	Dec. 4, 2015, 8:55 p.m.
"Bookmarks"	chipx86	Dec. 4, 2015, 8:55 p.m.
I know we don't have this in many places, but I'm trying to get in the habit: Can you add …	chipx86	Dec. 4, 2015, 8:55 p.m.
We always use the long form: #FFFF00. Should also have a constant.	chipx86	Dec. 4, 2015, 8:55 p.m.
All z-indexes must be registered in defs.less as constants.	chipx86	Dec. 4, 2015, 8:55 p.m.
Not sure what e() is, but elsewhere, we do: .transition(padding-left 0.2s linear, top 0.2s ease-out;); (Note the inner ; near …	chipx86	Dec. 4, 2015, 8:56 p.m.
We use the form of: 1px #333 solid This also needs a constant defined. Same with all other colors.	chipx86	Dec. 4, 2015, 8:56 p.m.
All repeating sizes for padding, fonts, etc. should have a constant.	chipx86	Dec. 4, 2015, 8:56 p.m.
Here and elsewhere, make sure there's a space before {.	chipx86	Dec. 4, 2015, 8:56 p.m.
Selector groups are always in this order: & { ... } &#id { ... } &:pseudo-class { ... } &.class-name …	chipx86	Dec. 4, 2015, 8:57 p.m.
No capitalization of "bookmarks" needed. Can you have this go into detail on what the class does? What it's tracking, …	chipx86	Dec. 4, 2015, 8:57 p.m.
Multi-line comments must always be in this form: /* * line * line */ Same with other comments below.	chipx86	Dec. 4, 2015, 8:57 p.m.
Function/method docs must be in this form: /* * One-line summary. * * Multi-line description. */ When there are arguments …	chipx86	Dec. 8, 2015, 6:57 p.m.
Blank line between these.	chipx86	Dec. 4, 2015, 9:05 p.m.
Blank line between these.	chipx86	Dec. 4, 2015, 9:05 p.m.
Space before {.	chipx86	Dec. 4, 2015, 9:06 p.m.
Let's pull out model.get('fileDiffID') just once.	chipx86	Dec. 4, 2015, 9:06 p.m.
This would be better named as "updatedCookie". Make sure this is documented in the method docs.	chipx86	Dec. 4, 2015, 9:06 p.m.
No blank line here.	chipx86	Dec. 4, 2015, 9:07 p.m.
We only use one var statement per function, with one variable per line, like: var collection, listenerObj;	chipx86	Dec. 4, 2015, 9:44 p.m.
I don't know that this is really needed. If you just need to check that the event emitted properly, you …	chipx86	Dec. 4, 2015, 9:42 p.m.
Space before {. No need for return;, since that's implied.	chipx86	Dec. 4, 2015, 9:42 p.m.
Ideally, you should use .toBe instead of .toEqual in tests. The reason is that .toBe is equivalent to strict JavaScript …	chipx86	Dec. 5, 2015, 1:37 p.m.
Can you add docs for each of these?	chipx86	Dec. 4, 2015, 9:43 p.m.
Just like methods, this should be in the form of: /* * One-line summary. * * Multi-line description. */ This …	chipx86	Dec. 5, 2015, 1:37 p.m.
This is the default, so you don't need this here.	chipx86	Dec. 5, 2015, 1:42 p.m.
When named template, it implies it's the template for the view itself. You probably want to call this ghostBookmarkTemplate.	chipx86	Dec. 5, 2015, 1:42 p.m.
The strings must align. Indentation should be within the string, 1 space.	chipx86	Dec. 5, 2015, 1:43 p.m.
Each method needs a docstring.	chipx86	Dec. 5, 2015, 1:51 p.m.
Element-related initialization should go in render.	chipx86	Dec. 5, 2015, 1:53 p.m.
Any member variables that don't need public access should be made private (prefix the name with a _).	chipx86	Dec. 5, 2015, 1:54 p.m.
render must return this at the end.	chipx86	Dec. 5, 2015, 1:55 p.m.
render shouldn't be responsible for deciding where its main element is put. That's the responsibility of the caller, only.	chipx86	Dec. 8, 2015, 6:57 p.m.
These should instead go in events, like: events: { 'click .ghost': '_onGhostBookmarkClicked', ... }	chipx86	Dec. 6, 2015, 6:27 p.m.
That sounds odd to me, but you should definitely be doing: return _super(this).remove.call(this); instead of removing it manually. The parent …	chipx86	Dec. 5, 2015, 2:18 p.m.
Can you tell me what the body query is about? Is this about whether it's currently packed into the page …	chipx86	Dec. 5, 2015, 2:19 p.m.
What's this for? Ideally, you wouldn't need to do this. I want to be sure I understand it well.	chipx86	Dec. 5, 2015, 5:24 p.m.
The list of variables should begin with defined variables, so: var cookie = JSON.parse(cookieString), bookmark;	chipx86	Dec. 5, 2015, 5:25 p.m.
i must be declared above, in the existing var list.	chipx86	Dec. 5, 2015, 5:25 p.m.
All jQuery-wrapped elememnts should be named like $diffReviewable.	chipx86	Dec. 5, 2015, 5:30 p.m.
This is a bit confusing. Can you instead just do this work when binding the element? If you're just looking …	chipx86	Dec. 5, 2015, 5:30 p.m.
One var statement.	chipx86	Dec. 5, 2015, 5:43 p.m.
.parents() is very expensive. Is there any way we can be smarter about what we're looing up? Can you explain …	chipx86	Dec. 5, 2015, 5:43 p.m.
Blank line after this.	chipx86	Dec. 5, 2015, 5:43 p.m.
This can be one statement, and you can probably use .width(). How about: var $bookmarkText = this._$ghostBookmark.find('.bookmark-text'); this._$ghostBookmark.find('.bookmark-fold') .width($bookmarkText.outerWidth()); Also, …	chipx86	Dec. 5, 2015, 5:49 p.m.
Sentence capitalization and period.	chipx86	Dec. 5, 2015, 5:49 p.m.
The .css should be indented one.	chipx86	Dec. 6, 2015, 6:28 p.m.
These should go first, since they're defined.	chipx86	Dec. 6, 2015, 6:28 p.m.
.parents() is very expensive, so we shoud minimize this however we can. Certainly, we should never call it more than …	chipx86	Dec. 6, 2015, 8:42 p.m.
This is very awkwardly formatted. can we pull some of this out somehow?	chipx86	Dec. 6, 2015, 8:42 p.m.
This has already been pulled out of diffModel.	chipx86	Dec. 6, 2015, 6:28 p.m.
Only one blank line here.	chipx86	Dec. 6, 2015, 6:28 p.m.
Space before {.	chipx86	Dec. 6, 2015, 6:29 p.m.
filename +=.	chipx86	Dec. 6, 2015, 6:29 p.m.
You can chain these.	chipx86	Dec. 6, 2015, 6:29 p.m.
Blank line between these.	chipx86	Dec. 6, 2015, 6:29 p.m.
Values should align.	chipx86	Dec. 6, 2015, 6:30 p.m.
Blank line between these.	chipx86	Dec. 6, 2015, 6:30 p.m.
Needs docs.	chipx86	Dec. 8, 2015, 6:57 p.m.
Same comments as above with the formatting.	chipx86	Dec. 7, 2015, 7:49 p.m.
This will cause problems. You need to instead do: this.$el.html(this.template()); Also, this must happen in render().	chipx86	Dec. 7, 2015, 7:49 p.m.
Anything that isn't public API should have a _ in front of it.	chipx86	Dec. 7, 2015, 7:49 p.m.
Blank line before comments.	chipx86	Dec. 7, 2015, 7:49 p.m.
Would be best to pull these out into variables, since other things seem to use these.	chipx86	Dec. 7, 2015, 7:52 p.m.
Blank lines before comments.	chipx86	Dec. 7, 2015, 7:52 p.m.
If it's just one property, do: this.$el.css('top', topPosition);	chipx86	Dec. 7, 2015, 7:53 p.m.
Blank line between these. vars and returns generally should be thought of as their own "paragraphs" of code.	chipx86	Dec. 7, 2015, 7:54 p.m.
Parameters must align.	chipx86	Dec. 7, 2015, 7:55 p.m.
Blank line between these.	chipx86	Dec. 7, 2015, 7:55 p.m.
Need a space before {.	chipx86	Dec. 7, 2015, 7:55 p.m.
Because of how JavaScript works, this can pick up data you don't expect. You need to use this form: for …	chipx86	Dec. 7, 2015, 7:56 p.m.
Best to only pull this out once.	chipx86	Dec. 7, 2015, 7:59 p.m.
Indentation is wrong. !== should be used instead of !=.	chipx86	Dec. 7, 2015, 8 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Nov. 28, 2015, 2:40 p.m.
Should ideally be in alphabetical order.	chipx86	Dec. 7, 2015, 8 p.m.
Here too.	chipx86	Dec. 7, 2015, 8:01 p.m.
And these.	chipx86	Dec. 7, 2015, 8:01 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Dec. 6, 2015, 8:45 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Dec. 8, 2015, 7:47 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Dec. 8, 2015, 7:55 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Dec. 8, 2015, 8:03 p.m.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js
    reviewboard/static/rb/css/pages/diffviewer.less

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 1)
Show all issues
```
Space after selector.
```

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 1)

Show all issues

Alphabetize these.

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 1)

Show all issues

We should set a constant in defs.less for this.
e.g.

@ghost-bookmark-background-color: rgba(173, 173, 173, 0.5);

reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js (Diff revision 1)
Show all issues
```
Can we call this RB.MookmarkModel?
```

reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js (Diff revision 1)

Show all issues

There should be a defaults object that contains safe default values (although fileName would probably be null in that case)

reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js (Diff revision 1)
Show all issues
```
diffID
```
reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js (Diff revision 1)
Show all issues
```
interdiffID
```

reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js (Diff revision 1)

Show all issues

We really don't want to have models stored in models. Backbone doesn't support it that well. Instead, you can pass diffModel in options, e.g.

var bookmarkModel = new RB.BookmarkerModel({
    diffModel: diffModel
});


and then access it either through this.options or options (which is passed as the first argument to initialize).

reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js (Diff revision 1)
Show all issues
```
This should be in collections/bookmarkCollection.js
```
reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js (Diff revision 1)
Show all issues
```
Likewise, RB.BookmarkCollection ?
```
reviewboard/static/rb/js/diffviewer/models/bookmarkerModel.js (Diff revision 1)
Show all issues
```
Missing semicolon.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js (Diff revision 1)
Show all issues
```
RB.BookmarkContainerView
Also, this should be in its own separate file.
```

reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js (Diff revision 1)

Show all issues

We use lowerCamelCase for JS names and hyphen-case for HTML names, so this should be bookmark-container.

reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js (Diff revision 1)
Show all issues
```
Is this supposed to return something? If not, we do not need the return statement.
```

reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js (Diff revision 1)

Show all issues

Please format this like:

if (!noRender) {
    bookmarkerView.render();
}

reviewboard/static/rb/js/diffviewer/views/bookmarkerView.js (Diff revision 1)
Show all issues
```
We use one var statement as the first statement in any function.
```

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

Show all issues

You are likely going to want to use _.throttle here. Listening for mousemoves is expensive.

Can you upload screenshots showing how this works?

Change Summary:

WIP ver.0.2: Ghost bookmarker shrinking and expanding.

Description:

		Add Bookmarking feature in Diff viewer so users can
		create and quickly jump between bookmarkers. Should help
		users moving back and forth between different parts of
		diffs when doing code reviews.

		9/25 Starter code for the feature, with ghost bookmarker
		chasing cursor and displaying which line in which file
		it is looking at.
	+
	+	10/3 Ghost bookmarker will now shrink and expand in
	+	renponse to cursor position. Clicking on the
	+	ghost bookmarker will add bookmark DOM to the page.

Diff:

Revision 2 (+335)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
	reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
	reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
	reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
    reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
    reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
    reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
    reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Change Summary:

Added screenshots of ghost bookmark (before and after expansion)

Added Files:

Change Summary:

Updated description with the most recent commit on styling thumbnail and actual bookmarks

Description:

		Add Bookmarking feature in Diff viewer so users can
		create and quickly jump between bookmarkers. Should help
		users moving back and forth between different parts of
		diffs when doing code reviews.

		9/25 Starter code for the feature, with ghost bookmarker
		chasing cursor and displaying which line in which file
		it is looking at.

		10/3 Ghost bookmarker will now shrink and expand in
		renponse to cursor position. Clicking on the
		ghost bookmarker will add bookmark DOM to the page.
	+
	+	10/10 Added styling for the actual bookmark as well as the
	+	thumbnail. The thumbnail bookmark will appear in the position
	+	relative to the bookmark's position within the entire page,
	+	and will jump to where the bookmark is when clicked on it.

Diff:

Revision 3 (+406)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
	reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
	reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
	reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
    reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
    reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
    reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
    reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

So I don't have a lot of comments on the code yet, since it looks like you're still in early exploration.

However, I do want to help guide you into our coding style now, before you write too much code. These comments are primarily about that.

reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 3)
Show all issues
```
We put a space between the ) and the {. Please fix this throughout.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 3)
Show all issues
```
Should be bookmark (not Bookmark)
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 3)
Show all issues
```
This line isn't necessary.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 3)
Show all issues
```
Variables should start with a lower-case letter.
```

reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 3)

Show all issues

You don't need the !! here since it's just a truthy-test.

All conditionals should also have {}, regardless of whether the block therein is only one line.

reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 3)

Show all issues

It's probably better to format this with one call per line:

this.$ghostBookmark
    .removeClass('open')
    .find('.bookmark-fold')
    .css('width', 0);

reviewboard/static/rb/js/diffviewer/views/bookmarkView.js (Diff revision 3)
Show all issues
```
This stuff should probably happen in render rather than initialize.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 3)
Show all issues
```
Make sure you're indenting 4 spaces in JavaScript code.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 3)
Show all issues
```
Make sure you're indenting 4 spaces in JavaScript code.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 3)
Show all issues
```
The ?: operator will already convert to a bool, so you don't need !!
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 3)
Show all issues
```
You can use _.bind here, too.
```

Change Summary:

Add to students group

Groups:

+	students

Change Summary:

Updated code (Cookie support, thumbnail shrink/expand), added screen shots

Description:

		Add Bookmarking feature in Diff viewer so users can
		create and quickly jump between bookmarkers. Should help
		users moving back and forth between different parts of
		diffs when doing code reviews.

		9/25 Starter code for the feature, with ghost bookmarker
		chasing cursor and displaying which line in which file
		it is looking at.

		10/3 Ghost bookmarker will now shrink and expand in
		renponse to cursor position. Clicking on the
		ghost bookmarker will add bookmark DOM to the page.

		10/10 Added styling for the actual bookmark as well as the
		thumbnail. The thumbnail bookmark will appear in the position
		relative to the bookmark's position within the entire page,
		and will jump to where the bookmark is when clicked on it.
	+
	+	10/18 The thumbnail will also shrink/expand when the
	+	cursor approaches. Added Cookie support so the bookmarks
	+	will persist over page refresh, so the basics of the
	+	feature are all in. Still need to bugfix for refreshing
	+	the view on window resize, revision change, etc. The
	+	extra for using random colors so bookmarks are easy to
	+	differentiate is also to be implemented.

Diff:

Revision 4 (+481)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
	reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
	reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
	reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Added Files:

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
    reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
    reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
    reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
    reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Change Summary:

Updated with bugfix commits and random bookmark coloring. Also added test cases.

Description:

		Add Bookmarking feature in Diff viewer so users can
		create and quickly jump between bookmarkers. Should help
		users moving back and forth between different parts of
		diffs when doing code reviews.

		9/25 Starter code for the feature, with ghost bookmarker
		chasing cursor and displaying which line in which file
		it is looking at.

		10/3 Ghost bookmarker will now shrink and expand in
		renponse to cursor position. Clicking on the
		ghost bookmarker will add bookmark DOM to the page.

		10/10 Added styling for the actual bookmark as well as the
		thumbnail. The thumbnail bookmark will appear in the position
		relative to the bookmark's position within the entire page,
		and will jump to where the bookmark is when clicked on it.

		10/18 The thumbnail will also shrink/expand when the
		cursor approaches. Added Cookie support so the bookmarks
		will persist over page refresh, so the basics of the
		feature are all in. Still need to bugfix for refreshing
		the view on window resize, revision change, etc. The
		extra for using random colors so bookmarks are easy to
		differentiate is also to be implemented.
	+
	+	10/23 Bugfixes with bookmark positions and preventing
	+	invalid bookmarks. Supports repositioning thumbnails at
	+	window resize. Added a feature where bookmarks will have
	+	random colors assigned from pre-defined list of colors
	+	so they are easy to differentiate. May need more testing
	+	but otherwise should be functional.

Testing Done:

~		So far only tested on local devserver.
	~	Test cases (so far only tested manually on local devserver):
	+
	+	Ghost bookmark
	+	- should move along with the cursor on the diff
	+	- becomes invisible when the cursor is not on diff
	+	- expands when the cursor approaches
	+	- shrinks when the cursor moves away
	+	- creates a new bookmark when clicked on it
	+	- no ghost bookmark when there already is a bookmark on the hovered line
	+
	+	Adding bookmark
	+	- on file header should only display the filename
	+	- on a line in a file should display the filename and the line number
	+	- on an empty line should display with good styling
	+	- on a line that wraps on browser as multiple lines should display with good styling
	+	- added bookmark will appear again after refreshing on the page
	+	- bookmark on an expanded chunk should appear again after collapsing the chunk and expanding again
	+	- should have different colors for first 10 bookmarks
	+
	+	Removing bookmark
	+	- from a diff successfully removes bookmark and thumbnail
	+	- from a new file successfully removes bookmark and thumbnail
	+	- removed bookmark is still gone after refreshing on the page
	+
	+	Bookmark thumbnail
	+	- should appear where the main bookmark is relative to the whole page
	+	- should have same color as the main bookmark
	+	- expands when the cursor approaches
	+	- shrinks when the cursor moves away
	+	- jumps to where the main bookmark is when clicked
	+	- appears in front of ghost bookmark
	+	- is repositioned when window is resized
	+	- is repositioned when a chunk is expanded/collapsed
	+	- is repositioned when a file is loaded

Diff:

Revision 5 (+520)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
	reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
	reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
	reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Added Files:

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
    reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
    reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
    reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
    reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Change Summary:

Deleted duplictate screenshot

Removed Files:

Change Summary:

Added initial draft (+mock image) of usability improvements including unified bookmark representation.

Description:

		Add Bookmarking feature in Diff viewer so users can
		create and quickly jump between bookmarkers. Should help
		users moving back and forth between different parts of
		diffs when doing code reviews.

		9/25 Starter code for the feature, with ghost bookmarker
		chasing cursor and displaying which line in which file
		it is looking at.

		10/3 Ghost bookmarker will now shrink and expand in
		renponse to cursor position. Clicking on the
		ghost bookmarker will add bookmark DOM to the page.

		10/10 Added styling for the actual bookmark as well as the
		thumbnail. The thumbnail bookmark will appear in the position
		relative to the bookmark's position within the entire page,
		and will jump to where the bookmark is when clicked on it.

		10/18 The thumbnail will also shrink/expand when the
		cursor approaches. Added Cookie support so the bookmarks
		will persist over page refresh, so the basics of the
		feature are all in. Still need to bugfix for refreshing
		the view on window resize, revision change, etc. The
		extra for using random colors so bookmarks are easy to
		differentiate is also to be implemented.

		10/23 Bugfixes with bookmark positions and preventing
		invalid bookmarks. Supports repositioning thumbnails at
		window resize. Added a feature where bookmarks will have
		random colors assigned from pre-defined list of colors
		so they are easy to differentiate. May need more testing
		but otherwise should be functional.
	+
	+	10/31 Usability improvements. Changed cookie expiry to
	+	one week and ghost bookmark color is slightly lighter.
	+	Unified the representation of a bookmark to a single
	+	object (previously called thumbnail), but tweaking directly
	+	from previous implementation seem to cause few glitches,
	+	therefore refactoring is needed for bugfix and polish.
	+	The bookmark when created will animate, but at a folded
	+	state; ideally we want the bookmark expanded at first
	+	and then fold as it slides, which is still to be
	+	implemented.

Diff:

Revision 6 (+533)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
	reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
	reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
	reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Added Files:

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
    reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
    reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js
    reviewboard/static/rb/js/diffviewer/views/bookmarkView.js
    reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

This is looking good and a lot of these issues are style nits. However, I have some general comments:

You should use sentence casing for all comments. That means they should begin with a capital and end with a period.
You should prefer === over == unless you are intentionally doing a type-coercing comparision. In that case, you should document why you are using coercion over expicit equality.
Your functions need doc comments unless they're overriding functions like render, initialize, etc.

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 6)

Show all issues

This should be the last line in the file.

This is a vim modeline which gives the editor settings for editing the file. Vim expects it to occur at the end of the file.

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 6)
Show all issues
```
You can nest this in the .close rule with &:hover.
```

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 6)

Show all issues

You should use .transition(). It does a prefixed version of transition.

reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)
Show all issues
```
"the diffviewer"
Needs a period.
```

reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)

Show all issues

I'm not sure if the cookie data should be passed in via options or not. You should maybe ping Christian and ask him.

reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)
Show all issues
```
Put these on seperate lines.
```
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)
Show all issues
```
Remove this comment.
```
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)
Show all issues
```
We use jquery.cookie. You can use it for reading/writing cookies. Check out the docs
```
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)
Show all issues
```
Can we call this cookie ?
```
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)
Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)
Show all issues
```
Remove this comment.
```

reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)

Show all issues

Replace this with a for loop.

reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)

Show all issues

Since we're not actually transmuting the models or options, I feel like we should do this in a signal handler.

In initialize we can do:

this.listenTo(this, 'update', this.saveToCookie);

That way, saveToCookie will be called whenever add/remove/set is called and we don't need to define these functions.

(Just make sure this is in initialize after the cookie data is loaded).

reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)
Show all issues
```
Needs a doc comment.
```
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)
Show all issues
```
Put these on seperate lines, with defined variables first.
```
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)
Show all issues
```
Can we call this bookmark ?
```
reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)
Show all issues
```
remove this comment.
```

reviewboard/static/rb/js/diffviewer/collections/bookmarkCollection.js (Diff revision 6)

Show all issues

This overwrites all of the cookie that is set, doesn't it? We really should use $.cookie here.

reviewboard/static/rb/js/diffviewer/models/bookmarkModel.js (Diff revision 6)
Show all issues
```
Needs a period.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
Needs a doc comment.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
Just call it template. That is the convention we use.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
Space after , between items.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
Remove this comment.
```

reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)

Show all issues

This should use sentence casing, e.g., begin with a capital and end with a period.

"from a clone of the"

reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)

Show all issues

Missing a var statement.
e isn't a descriptive variable name; we should use bookmark instead.

reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)

Show all issues

Since you're doing a filter & map on the values and only using them once, we can just do a map and do the filtering inline as an if:

this.collection.each(function(bookmark) {
    var fileDiffID = bookmark.get('fileDiffID'),
        interFileDiffID = bookmark.get('interFileDiffID');

    if (this.pageView.showsDiffFile(fileDiffID, interFileDiffID)) {
        this.createBookMarkView(bookmark, true);
    }
}, this);


This will be more efficient becuase there will be O(n) functions calls instead of ~2n.

reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)

Show all issues

Backbone.Model.get only takes one key at a time, so this should be:

e.get('fileDiffID'), e.get('interFileDiffID')

reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)

Show all issues

If this is private it should be prefixed with an underscore.
Also, this needs a doccomment.

reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
Remove this line.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
view is more readable than bview.
```

reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)

Show all issues

We format multi-line comments in JS as follows:

/*
 * Line 1.
 * Line 2.
 * ...
 */

reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
You can do view.remove().render() because remove should return the view.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
Same comment as above regards to public/private-ness and doc comment.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
Should be prefixed with underscore. Needs doc comment.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
Should be prefixed with underscore. Needs doc comment.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
Doc comment.
```

reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)

Show all issues


var $target = $(e.target),
    fileName,
    line,
    top;

reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
Blank line between statement and block.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
See above re: multi-line comments.
```

reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)

Show all issues

This is quite hard to read.

I'm not sure what nodeType == 3 means? Is there a constnat we can use.

Really, I'm not sure what this is doing in general. Ping me in slack and we can discuss this.

reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
Should be indented 4 spaces not 2.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
What do you mean, without updating obj?
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
Sentence casing.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
} else {
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
doc comment.
```
reviewboard/static/rb/js/diffviewer/views/bookmarkContainerView.js (Diff revision 6)
Show all issues
```
Doc comment.
```

reviewboard/static/rb/js/diffviewer/views/bookmarkView.js (Diff revision 6)

Show all issues

We prefix jQuery objects, so this should be $diffViewElem.

Also, you can shorten Elem to El.

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 6)
Show all issues
```
Needs doc comment.
```

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

Show all issues

Should be indented 4.

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

Show all issues


var fileDiff = file.get('filediff'),
    interFileDiff = file.get('interfilediff');

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

Show all issues

We can write this more clearly as :

return (filediff.id === fileDiffID &&
        ((interFileDiff === null && interFileDiffID === null) ||
         interFileDiff.id === interFileDiffID));


This should use === not ==.

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 6)
Show all issues
```
===
```

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

Show all issues

Indent multiples of 4 spaces.

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 6)
Show all issues
```
Missing a period.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 6)
Show all issues
```
Missing a period.
```

Change Summary:

Animation at bookmark creation, moving cookie management to UserSession, code styles.

Description:

		Add Bookmarking feature in Diff viewer so users can
		create and quickly jump between bookmarkers. Should help
		users moving back and forth between different parts of
		diffs when doing code reviews.

		9/25 Starter code for the feature, with ghost bookmarker
		chasing cursor and displaying which line in which file
		it is looking at.

		10/3 Ghost bookmarker will now shrink and expand in
		renponse to cursor position. Clicking on the
		ghost bookmarker will add bookmark DOM to the page.

		10/10 Added styling for the actual bookmark as well as the
		thumbnail. The thumbnail bookmark will appear in the position
		relative to the bookmark's position within the entire page,
		and will jump to where the bookmark is when clicked on it.

		10/18 The thumbnail will also shrink/expand when the
		cursor approaches. Added Cookie support so the bookmarks
		will persist over page refresh, so the basics of the
		feature are all in. Still need to bugfix for refreshing
		the view on window resize, revision change, etc. The
		extra for using random colors so bookmarks are easy to
		differentiate is also to be implemented.

		10/23 Bugfixes with bookmark positions and preventing
		invalid bookmarks. Supports repositioning thumbnails at
		window resize. Added a feature where bookmarks will have
		random colors assigned from pre-defined list of colors
		so they are easy to differentiate. May need more testing
		but otherwise should be functional.

		10/31 Usability improvements. Changed cookie expiry to
		one week and ghost bookmark color is slightly lighter.
		Unified the representation of a bookmark to a single
		object (previously called thumbnail), but tweaking directly
		from previous implementation seem to cause few glitches,
		therefore refactoring is needed for bugfix and polish.
		The bookmark when created will animate, but at a folded
		state; ideally we want the bookmark expanded at first
		and then fold as it slides, which is still to be
		implemented.
	+
	+	11/7 Animation when a new bookmark is created.
	+	Code style fixes, including renaming 'bookmark' to
	+	'diffBookmark', moving cookie management to UserSession,
	+	and adding doc comments. Few style issues, such as loose
	+	equality checks, and confusing codes are yet to be
	+	addressed.

Diff:

Revision 7 (+632)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
	reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
	reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
	reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
	reviewboard/static/rb/js/models/userSessionModel.js
	1 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Change Summary:

Updated with rewritten cookie management and other code style cleanups.

Description:

		Add Bookmarking feature in Diff viewer so users can
		create and quickly jump between bookmarkers. Should help
		users moving back and forth between different parts of
		diffs when doing code reviews.

		9/25 Starter code for the feature, with ghost bookmarker
		chasing cursor and displaying which line in which file
		it is looking at.

		10/3 Ghost bookmarker will now shrink and expand in
		renponse to cursor position. Clicking on the
		ghost bookmarker will add bookmark DOM to the page.

		10/10 Added styling for the actual bookmark as well as the
		thumbnail. The thumbnail bookmark will appear in the position
		relative to the bookmark's position within the entire page,
		and will jump to where the bookmark is when clicked on it.

		10/18 The thumbnail will also shrink/expand when the
		cursor approaches. Added Cookie support so the bookmarks
		will persist over page refresh, so the basics of the
		feature are all in. Still need to bugfix for refreshing
		the view on window resize, revision change, etc. The
		extra for using random colors so bookmarks are easy to
		differentiate is also to be implemented.

		10/23 Bugfixes with bookmark positions and preventing
		invalid bookmarks. Supports repositioning thumbnails at
		window resize. Added a feature where bookmarks will have
		random colors assigned from pre-defined list of colors
		so they are easy to differentiate. May need more testing
		but otherwise should be functional.

		10/31 Usability improvements. Changed cookie expiry to
		one week and ghost bookmark color is slightly lighter.
		Unified the representation of a bookmark to a single
		object (previously called thumbnail), but tweaking directly
		from previous implementation seem to cause few glitches,
		therefore refactoring is needed for bugfix and polish.
		The bookmark when created will animate, but at a folded
		state; ideally we want the bookmark expanded at first
		and then fold as it slides, which is still to be
		implemented.

		11/7 Animation when a new bookmark is created.
		Code style fixes, including renaming 'bookmark' to
		'diffBookmark', moving cookie management to UserSession,
		and adding doc comments. Few style issues, such as loose
		equality checks, and confusing codes are yet to be
		addressed.
	+
	+	11/15 Rewriting cookie management so cookies are saved
	+	per fragment for more space as well as compatibility
	+	with not just revision switch but also with pagination.
	+	More code style cleanup and fixing confusing codes.
	+	Needs to be tested again since many parts have been
	+	rewritten, and automated tests are still on queue.

Diff:

Revision 8 (+676)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
	reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
	reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
	reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
	2 more

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/diffviewer/views.py (Diff revision 8)
Show all issues
```
Col: 27
 W601 .has_key() is deprecated, use 'in'
```

Change Summary:

Bug and code style fixes, start adding automated tests.

Description:

		Add Bookmarking feature in Diff viewer so users can
		create and quickly jump between bookmarkers. Should help
		users moving back and forth between different parts of
		diffs when doing code reviews.

		9/25 Starter code for the feature, with ghost bookmarker
		chasing cursor and displaying which line in which file
		it is looking at.

		10/3 Ghost bookmarker will now shrink and expand in
		renponse to cursor position. Clicking on the
		ghost bookmarker will add bookmark DOM to the page.

		10/10 Added styling for the actual bookmark as well as the
		thumbnail. The thumbnail bookmark will appear in the position
		relative to the bookmark's position within the entire page,
		and will jump to where the bookmark is when clicked on it.

		10/18 The thumbnail will also shrink/expand when the
		cursor approaches. Added Cookie support so the bookmarks
		will persist over page refresh, so the basics of the
		feature are all in. Still need to bugfix for refreshing
		the view on window resize, revision change, etc. The
		extra for using random colors so bookmarks are easy to
		differentiate is also to be implemented.

		10/23 Bugfixes with bookmark positions and preventing
		invalid bookmarks. Supports repositioning thumbnails at
		window resize. Added a feature where bookmarks will have
		random colors assigned from pre-defined list of colors
		so they are easy to differentiate. May need more testing
		but otherwise should be functional.

		10/31 Usability improvements. Changed cookie expiry to
		one week and ghost bookmark color is slightly lighter.
		Unified the representation of a bookmark to a single
		object (previously called thumbnail), but tweaking directly
		from previous implementation seem to cause few glitches,
		therefore refactoring is needed for bugfix and polish.
		The bookmark when created will animate, but at a folded
		state; ideally we want the bookmark expanded at first
		and then fold as it slides, which is still to be
		implemented.

		11/7 Animation when a new bookmark is created.
		Code style fixes, including renaming 'bookmark' to
		'diffBookmark', moving cookie management to UserSession,
		and adding doc comments. Few style issues, such as loose
		equality checks, and confusing codes are yet to be
		addressed.

		11/15 Rewriting cookie management so cookies are saved
		per fragment for more space as well as compatibility
		with not just revision switch but also with pagination.
		More code style cleanup and fixing confusing codes.
		Needs to be tested again since many parts have been
		rewritten, and automated tests are still on queue.
	+
	+	11/21 Fixed a bug with bookmarks not being saved when
	+	on hashed pages, and few more coding style cleanups.
	+	Added some automated tests for DiffBookmarkCollection
	+	and for DiffBookmarksContainerView.
	+	More tests to be added.

Diff:

Revision 9 (+987)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
	reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
	reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
	reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
	4 more

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
    reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
    reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/staticbundles.py (Diff revision 9)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Change Summary:

Add JS test cases, Update description and testing done.

Summary:

-	(WIP) Add Bookmarking feature to Diff Viewer.
+	Add Bookmarking feature to Diff Viewer.

Description:

~		Add Bookmarking feature in Diff viewer so users can
~		create and quickly jump between bookmarkers. Should help
~		users moving back and forth between different parts of
	~	Add bookmarking feature in diff viewer so users can create and quickly jump
	~	between bookmarks. Intended to help users moving back and forth between
	~	different parts in the diff viewer when doing code reviews.
-		diffs when doing code reviews.

~		9/25 Starter code for the feature, with ghost bookmarker
~		chasing cursor and displaying which line in which file
~		it is looking at.
	~	A "ghost" bookmark will follow the cursor on the right margin of the diff
	~	viewer, which a user can click on to create new bookmarks on the hovered line
	~	of the diff file. Clicking on the created bookmark will scroll the window
	+	to where the bookmarked line is. Bookmarks are saved per diff using browser
	+	Cookie, and will persist for one week.

~		10/3 Ghost bookmarker will now shrink and expand in
~		renponse to cursor position. Clicking on the
~		ghost bookmarker will add bookmark DOM to the page.
~
	~	Usability features include assigning different colors to bookmarks for easy
	~	differentiation, positioning bookmarks on where the target line is relative
	~	to the whole page, and animating the position at bookmark creation so users
	~	can see where the newly created bookmark will reside.
-		10/10 Added styling for the actual bookmark as well as the
-		thumbnail. The thumbnail bookmark will appear in the position
-		relative to the bookmark's position within the entire page,
-		and will jump to where the bookmark is when clicked on it.
-
-		10/18 The thumbnail will also shrink/expand when the
-		cursor approaches. Added Cookie support so the bookmarks
-		will persist over page refresh, so the basics of the
-		feature are all in. Still need to bugfix for refreshing
-		the view on window resize, revision change, etc. The
-		extra for using random colors so bookmarks are easy to
-		differentiate is also to be implemented.
-
-		10/23 Bugfixes with bookmark positions and preventing
-		invalid bookmarks. Supports repositioning thumbnails at
-		window resize. Added a feature where bookmarks will have
-		random colors assigned from pre-defined list of colors
-		so they are easy to differentiate. May need more testing
-		but otherwise should be functional.
-
-		10/31 Usability improvements. Changed cookie expiry to
-		one week and ghost bookmark color is slightly lighter.
-		Unified the representation of a bookmark to a single
-		object (previously called thumbnail), but tweaking directly
-		from previous implementation seem to cause few glitches,
-		therefore refactoring is needed for bugfix and polish.
-		The bookmark when created will animate, but at a folded
-		state; ideally we want the bookmark expanded at first
-		and then fold as it slides, which is still to be
-		implemented.
-
-		11/7 Animation when a new bookmark is created.
-		Code style fixes, including renaming 'bookmark' to
-		'diffBookmark', moving cookie management to UserSession,
-		and adding doc comments. Few style issues, such as loose
-		equality checks, and confusing codes are yet to be
-		addressed.
-
-		11/15 Rewriting cookie management so cookies are saved
-		per fragment for more space as well as compatibility
-		with not just revision switch but also with pagination.
-		More code style cleanup and fixing confusing codes.
-		Needs to be tested again since many parts have been
-		rewritten, and automated tests are still on queue.
-
-		11/21 Fixed a bug with bookmarks not being saved when
-		on hashed pages, and few more coding style cleanups.
-		Added some automated tests for DiffBookmarkCollection
-		and for DiffBookmarksContainerView.
-		More tests to be added.

Testing Done:

~		Test cases (so far only tested manually on local devserver):
	~	Automated JS test cases:

		Ghost bookmark
		- should move along with the cursor on the diff
		- becomes invisible when the cursor is not on diff
		- expands when the cursor approaches
		- shrinks when the cursor moves away
		- creates a new bookmark when clicked on it
		- no ghost bookmark when there already is a bookmark on the hovered line

~		Adding bookmark
	~	Bookmarks
	+	- appears in front of ghost bookmark
	+	- expands when the cursor approaches
	+	- shrinks when the cursor moves away
	+	- will animate on creation
		- on file header should only display the filename
		- on a line in a file should display the filename and the line number
~		- on an empty line should display with good styling
~		- on a line that wraps on browser as multiple lines should display with good styling
~		- added bookmark will appear again after refreshing on the page
~		- bookmark on an expanded chunk should appear again after collapsing the chunk and expanding again
~		- should have different colors for first 10 bookmarks
	~	- on an expanded chunk will hide when the chunk is collapsed
	~	- on a collapsed chunk will appear when the chunk is expanded
	~	- should have different colors (for first 10 bookmarks)
	~	- will be removed when clicked on [x]
	~	- will scroll the page when clicked
	+	- can be created from cookies
	+	- will trigger cookie save event on creation or removal

~		Removing bookmark
	~
-		- from a diff successfully removes bookmark and thumbnail
-		- from a new file successfully removes bookmark and thumbnail
-		- removed bookmark is still gone after refreshing on the page

~		Bookmark thumbnail
~		- should appear where the main bookmark is relative to the whole page
~		- should have same color as the main bookmark
~		- expands when the cursor approaches
~		- shrinks when the cursor moves away
~		- jumps to where the main bookmark is when clicked
~		- appears in front of ghost bookmark
~		- is repositioned when window is resized
~		- is repositioned when a chunk is expanded/collapsed
~		- is repositioned when a file is loaded
	~	Following features are tested manually on local devserver:
	~
	~	Bookmarks
	~	- on an empty line should display nicely
	~	- on a line that wraps on browser as multiple lines should display nicely
	~	- added bookmark will appear again after refreshing on the page
	~	- removed bookmarks will not appear again after refreshing on the page
	~	- should be positioned where the main bookmark is relative to the whole page
	~	- are repositioned when window is resized
	~	- are repositioned when a chunk is expanded/collapsed
	+	- are repositioned when a file is loaded

Diff:

Revision 10 (+1302)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
	reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
	reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
	reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
	4 more

Added Files:

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
    reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
    reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/staticbundles.py (Diff revision 10)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Change Summary:

Fix bug with loaded cookie not being up to date due to etag identification

Diff:

Revision 11 (+1317 -4)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
	reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
	reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
	reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
	4 more

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
    reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
    reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/staticbundles.py (Diff revision 11)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

First off, this feature is awesome and boy do I wish I had it while reviewing this change :)
Second, I have a lot of comments here. The good news is, most of these are stylistic things that I repeat over and over. You'll be able to plow through most of these.
However, I did not highlight every occurrence, so I'll want you to go through and find, for instance, cases where you have function(){ instead of function() {, and comments that need a blank line before them, etc.
There also needs to be more documentation. We're working on fleshing these out, and we want them to follow the Python standards. I'm happy to answer questions on this.
I also have some implementation comments, and comments about performance. For instance, I point out several cases where .parents() is used, which is an expensive function that should largely be avoided if at all possible, so I'd like to work with you to better understand these and see if we can come up with a better method.

reviewboard/diffviewer/views.py (Diff revision 11)

Show all issues

Can we rename the cookie to "diff-bookmarks" ? Or maybe we should just have it be "bookmarks" (as we may later want to port this to text-based file attachments, perhaps).

It's also cheaper to fetch the data out, and set the header if that data is not None. Further cheaper if you assume it exists in a try/except, like:

try:
    response['X-ReviewBoard-Bookmarks'] = request.COOKIES['bookmarks']
except KeyError:
    # There aren't any bookmark cookies set. Ignore.

reviewboard/diffviewer/views.py (Diff revision 11)

Show all issues

Just some tweaks to this. The argument name should include the type, and for request (a very common argument), we don't need to go into details of how it'll be used. It just runs the risk of going stale as we use more of it down the road.

request (django.http.HttpRequest):
    The HTTP request from the client.

reviewboard/diffviewer/views.py (Diff revision 11)

Show all issues

This will be faster as:

bookmark_cookie = request.COOKIES.get('diffbookmarks', '')

reviewboard/static/rb/css/defs.less (Diff revision 11)

Show all issues

We namespace these names, so these would be best as @bookmark-ghost-bg-color and @bookmark-default-bg-color, in alphabetical order.

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11)
Show all issues
```
"Bookmarks"
```

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11)

Show all issues

I know we don't have this in many places, but I'm trying to get in the habit: Can you add a comment for each of these selectors documenting what part of the UI this is?

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11)
Show all issues
```
We always use the long form: #FFFF00. Should also have a constant.
```
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11)
Show all issues
```
All z-indexes must be registered in defs.less as constants.
```

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11)

Show all issues

Not sure what e() is, but elsewhere, we do:

.transition(padding-left 0.2s linear, top 0.2s ease-out;);


(Note the inner ; near the end. This forces LessCSS to consider our commas as literal text, and not argument separators.)

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11)

Show all issues

We use the form of:

1px #333 solid

This also needs a constant defined. Same with all other colors.

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11)

Show all issues

All repeating sizes for padding, fonts, etc. should have a constant.

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11)
Show all issues
```
Here and elsewhere, make sure there's a space before {.
```

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 11)

Show all issues

Selector groups are always in this order:

& { ... }
&#id { ... }
&:pseudo-class { ... }
&.class-name { ...}
element { ... }
#id { ... }
.class-name { ... }


Those should be in alphabetical order within a grouping.
This applies to all the CSS in this change.

reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11)

Show all issues

No capitalization of "bookmarks" needed.
Can you have this go into detail on what the class does? What it's tracking, how it's used, etc.

reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11)

Show all issues

Multi-line comments must always be in this form:

/*
 * line
 * line
 */

Same with other comments below.

reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11)

Show all issues

Function/method docs must be in this form:

/*
 * One-line summary.
 *
 * Multi-line description.
 */


When there are arguments or return values, add:

/*
 * ...
 *
 * Args:
 *     bookmark (the type):
 *         The description.
 *
 *     next_argument (type):
 *         ...
 *
 * Returns:
 *     the return type:
 *     The description.
 */


Same with all other methods.

reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11)
Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11)
Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11)
Show all issues
```
Space before {.
```
reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11)
Show all issues
```
Let's pull out model.get('fileDiffID') just once.
```
reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11)
Show all issues
```
This would be better named as "updatedCookie".
Make sure this is documented in the method docs.
```
reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js (Diff revision 11)
Show all issues
```
No blank line here.
```

reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js (Diff revision 11)

Show all issues

We only use one var statement per function, with one variable per line, like:

var collection,
    listenerObj;

reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js (Diff revision 11)

Show all issues

I don't know that this is really needed. If you just need to check that the event emitted properly, you should spy on ollection.trigger and check that it was called with the expected parameters for emitting that event.

reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js (Diff revision 11)
Show all issues
```
Space before {.
No need for return;, since that's implied.
```

reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js (Diff revision 11)

Show all issues

Ideally, you should use .toBe instead of .toEqual in tests. The reason is that .toBe is equivalent to strict JavaScript comparison (===), which means that 1 === 1 but 1 !== "1".
If you use .toEqual, it's like ==, so 1 == "1".
Unit tests should be strict, so we want to catch future issues like the possibility of an integer turning into a string.

reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js (Diff revision 11)
Show all issues
```
Can you add docs for each of these?
```

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)

Show all issues

Just like methods, this should be in the form of:

/*
 * One-line summary.
 *
 * Multi-line description.
 */


This should also go into more detail of what this class is in charge of doing.

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
This is the default, so you don't need this here.
```

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)

Show all issues

When named template, it implies it's the template for the view itself. You probably want to call this ghostBookmarkTemplate.

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)

Show all issues

The strings must align. Indentation should be within the string, 1 space.

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
Each method needs a docstring.
```
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
Element-related initialization should go in render.
```

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)

Show all issues

Any member variables that don't need public access should be made private (prefix the name with a _).

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
render must return this at the end.
```

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)

Show all issues

render shouldn't be responsible for deciding where its main element is put. That's the responsibility of the caller, only.

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)

Show all issues

These should instead go in events, like:

events: {
    'click .ghost': '_onGhostBookmarkClicked',
    ...
}

ynkym Dec. 8, 2015, 7:28 p.m.

$ghostBookmark is actually not a child of the container $el, so event listeners need to be registered manually.

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)

Show all issues

That sounds odd to me, but you should definitely be doing:

return _super(this).remove.call(this);


instead of removing it manually. The parent remove() does more than this.

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)

Show all issues

Can you tell me what the body query is about? Is this about whether it's currently packed into the page somewhere?

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)

Show all issues

What's this for? Ideally, you wouldn't need to do this. I want to be sure I understand it well.

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)

Show all issues

The list of variables should begin with defined variables, so:

var cookie = JSON.parse(cookieString),
    bookmark;

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
i must be declared above, in the existing var list.
```
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
All jQuery-wrapped elememnts should be named like $diffReviewable.
```

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)

Show all issues

This is a bit confusing. Can you instead just do this work when binding the element? If you're just looking to bind additional arguments, you can pass them to _.bind().

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
One var statement.
```

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)

Show all issues

.parents() is very expensive. Is there any way we can be smarter about what we're looing up? Can you explain these checks in more detail to me?

I'm wondering if hasClass() would be enough.

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
Blank line after this.
```

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)

Show all issues

This can be one statement, and you can probably use .width(). How about:

var $bookmarkText = this._$ghostBookmark.find('.bookmark-text');

this._$ghostBookmark.find('.bookmark-fold')
    .width($bookmarkText.outerWidth());


Also, how about pulling out things like .bookmark-text in render() above, so that we don't have to keep re-fetching?

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
Sentence capitalization and period.
```
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
The .css should be indented one.
```
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
These should go first, since they're defined.
```

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)

Show all issues

.parents() is very expensive, so we shoud minimize this however we can. Certainly, we should never call it more than once for the same element with the same selector.

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)

Show all issues

This is very awkwardly formatted. can we pull some of this out somehow?

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
This has already been pulled out of diffModel.
```

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)

Show all issues

Only one blank line here.

reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
Space before {.
```
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
filename +=.
```
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
You can chain these.
```
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
Values should align.
```
reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js (Diff revision 11)
Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11)
Show all issues
```
Needs docs.
```

reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11)

Show all issues

Same comments as above with the formatting.

reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11)

Show all issues

This will cause problems. You need to instead do:

this.$el.html(this.template());


Also, this must happen in render().

reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11)
Show all issues
```
Anything that isn't public API should have a _ in front of it.
```
reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11)
Show all issues
```
Blank line before comments.
```

reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11)

Show all issues

Would be best to pull these out into variables, since other things seem to use these.

reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11)
Show all issues
```
Blank lines before comments.
```

reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js (Diff revision 11)

Show all issues

If it's just one property, do:

this.$el.css('top', topPosition);

reviewboard/static/rb/js/models/userSessionModel.js (Diff revision 11)

Show all issues

Blank line between these.

vars and returns generally should be thought of as their own "paragraphs" of code.

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11)
Show all issues
```
Parameters must align.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11)
Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11)
Show all issues
```
Need a space before {.
```

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

Show all issues

Because of how JavaScript works, this can pick up data you don't expect. You need to use this form:

for (var fileID in data) {
    if (data.hasOwnProperty(fileID)) {
        ...
    }
}


That assumes that data is an object and not an array.
Also, space before {.

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11)
Show all issues
```
Best to only pull this out once.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 11)
Show all issues
```
Indentation is wrong.
!== should be used instead of !=.
```
reviewboard/staticbundles.py (Diff revision 11)
Show all issues
```
Should ideally be in alphabetical order.
```
reviewboard/staticbundles.py (Diff revision 11)
Show all issues
```
Here too.
```
reviewboard/staticbundles.py (Diff revision 11)
Show all issues
```
And these.
```

Change Summary:

More documentations, coding styles, few small performance optimizations. Documentations and other polishes still wip.

Diff:

Revision 12 (+1491 -4)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
	reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
	reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
	reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
	4 more

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
    reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
    reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/staticbundles.py (Diff revision 12)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Change Summary:

Fixes for coding styles, readability, documentations and micro-optimizations.

Diff:

Revision 13 (+1593 -4)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
	reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
	reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
	reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
	4 more

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
    reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
    reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/staticbundles.py (Diff revision 13)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Change Summary:

Add forgotten documentations for diffViewerPageView and userSessionModel.

Diff:

Revision 14 (+1622 -4)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
	reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
	reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
	reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
	4 more

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
    reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
    reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/staticbundles.py (Diff revision 14)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Change Summary:

Add forgotten spaces between parensthesis and bracket.

Diff:

Revision 15 (+1622 -4)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
	reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
	reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
	reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
	4 more

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
    reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/models/diffBookmarkModel.js
    reviewboard/static/rb/js/diffviewer/collections/diffBookmarkCollection.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/tests/diffBookmarkContainerViewTests.js
    reviewboard/static/rb/js/diffviewer/collections/tests/diffBookmarkCollectionTests.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkView.js
    reviewboard/static/rb/js/diffviewer/views/diffBookmarkContainerView.js
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/models/userSessionModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/staticbundles.py (Diff revision 15)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Status: Discarded

Status: Re-opened

Owner:

-	ynkym
+	david

Add Bookmarking feature to Diff Viewer.

Change Summary:

Description:

Diff:

Change Summary:

Added Files:

Change Summary:

Description:

Diff:

Change Summary:

Groups:

Change Summary:

Description:

Diff:

Added Files:

Change Summary:

Description:

Testing Done:

Diff:

Added Files:

Change Summary:

Removed Files:

Change Summary:

Description:

Diff:

Added Files:

Change Summary:

Description:

Diff:

Change Summary:

Description:

Diff:

Change Summary:

Description:

Diff:

Change Summary:

Summary:

Description:

Testing Done:

Diff:

Added Files:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Discarded

Status: Re-opened

Owner: