Summary

Revision selectors

Review Request #6120 — Created July 19, 2014 and discarded Sept. 20, 2014, 11:10 a.m.

Information

Owner

volodymyr

Repository

Review Board

Branch

Bugs

Depends On

Blocks

5824

Reviewers

Groups

reviewboard, students

People

Description

Adding revision selector for attachments. To avoid code duplication with existing selector for diffs, a common parent class called CommonRevisionSelectorView was created.

Testing Done

Tested as part of https://reviews.reviewboard.org/r/5824/

Issues

Description	From	Last Updated
__super__ isn't a public part of backbone's API. Instead, we should be using our own _super method: _super(this).initialize.apply(this, arguments);	david	Aug. 2, 2014, 11:54 p.m.
This won't quite work. Instead, you should use this: { attachmentRevisionText: iterpolate( gettext('Attachment Revision %s'), [this.options.revision]) }	david	Aug. 2, 2014, 11:54 p.m.
Same comment about __super__. Because you're specifying your own arguments, call is a better choice than apply: return _super(this).render.call( this, …	david	Aug. 2, 2014, 11:54 p.m.
This comment is wrong.	david	Aug. 2, 2014, 11:54 p.m.
There's no value variable delclared in this method, which means that this is global. It also seems like this method …	david	Aug. 2, 2014, 11:54 p.m.
What is '4'?	david	Aug. 4, 2014, 12:27 p.m.
This should use _super and call	david	Aug. 2, 2014, 11:54 p.m.
This comment is wrong.	david	Aug. 2, 2014, 11:54 p.m.
This should use _super and call	david	Aug. 2, 2014, 11:54 p.m.
This fits on one line	david	Aug. 2, 2014, 11:54 p.m.
I don't know what this comment is about.	david	Aug. 2, 2014, 11:54 p.m.
Remove this line?	david	Aug. 2, 2014, 11:54 p.m.
Add a blank line between these.	david	Aug. 2, 2014, 11:54 p.m.
_super	david	Aug. 2, 2014, 11:54 p.m.
_super and call	david	Aug. 2, 2014, 11:54 p.m.
_super and call	david	Aug. 2, 2014, 11:54 p.m.
_super and call	david	Aug. 2, 2014, 11:54 p.m.
This does exactly what the parent class' _onLabelClick implementation does, so you can remove it.	david	Aug. 4, 2014, 12:27 p.m.
Indentation is super weird here.	david	Aug. 6, 2014, 3:12 p.m.
This comment can go away.	david	Aug. 6, 2014, 3:12 p.m.
Did you mean to be passing an array for the argument? I suspect not. Also, this would all fit on …	david	Aug. 6, 2014, 3:13 p.m.
Did you mean to be passing an array for the argument? I suspect not.	david	Aug. 6, 2014, 3:13 p.m.
We should have slightly different labels between these two. For diffs, we want ['orig', '1', '2', '3', ...], because the …	david	Aug. 6, 2014, 3:14 p.m.
Doesn't this end up adding one too many ticks for file attachments?	david	Aug. 4, 2014, 12:36 p.m.
You're passing one single argument to the super render, which is an array of stuff. Based on your implementation above, …	david	Aug. 6, 2014, 3:20 p.m.
This should use _super and call	david	Aug. 6, 2014, 3:20 p.m.
Shouldn't have an array around ev.	david	Aug. 6, 2014, 3:20 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
    reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
    reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js



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

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
    reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
    reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js

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

The issue has been resolved. Show all issues

__super__ isn't a public part of backbone's API. Instead, we should be using our own _super method:

_super(this).initialize.apply(this, arguments);

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

The issue has been resolved. Show all issues

This won't quite work. Instead, you should use this:

{
    attachmentRevisionText: iterpolate(
        gettext('Attachment Revision %s'),
        [this.options.revision])
}

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

The issue has been resolved. Show all issues

Same comment about __super__.
Because you're specifying your own arguments, call is a better choice than apply:

return _super(this).render.call(
    this, this.options.numRevisions, true, 1);

reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
This comment is wrong.
```

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

The issue has been resolved. Show all issues

There's no value variable delclared in this method, which means that this is global.

It also seems like this method half assumes that there can be multiple handles, and half assumes that there's a single value.

reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js (Diff revision 1)
The issue has been dropped. Show all issues
```
What is '4'?
```
reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
This should use _super and call
```

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

The issue has been resolved. Show all issues

This comment is wrong.

reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
This should use _super and call
```

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

The issue has been resolved. Show all issues

This fits on one line

reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
I don't know what this comment is about.
```
reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Remove this line?
```

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

The issue has been resolved. Show all issues

Add a blank line between these.

reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
_super
```
reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
_super and call
```
reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
_super and call
```
reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
_super and call
```

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

The issue has been dropped. Show all issues

This does exactly what the parent class' _onLabelClick implementation does, so you can remove it.

david

Aug. 4, 2014, 12:27 p.m.

Sorry, I hadn't noticed the slight difference between the two implementations.

Diff:

Revision 2 (+289 -111)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
	reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
	reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js

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

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
    reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
    reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
    reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
    reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js

Fix it!

reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js (Diff revision 2)

The issue has been resolved. Show all issues

Indentation is super weird here.

reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
This comment can go away.
```

reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js (Diff revision 2)

The issue has been resolved. Show all issues

Did you mean to be passing an array for the argument? I suspect not. Also, this would all fit on one line.

volodymyr

Aug. 6, 2014, 3:20 p.m.

All of these were arrays when apply function was used. I forgot to fix them :(

reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Did you mean to be passing an array for the argument? I suspect not.
```

reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js (Diff revision 2)

The issue has been resolved. Show all issues

We should have slightly different labels between these two.

For diffs, we want ['orig', '1', '2', '3', ...], because the options are to diff between the original file and one of the revisions, or between two of the revisions.

For file attachments, there's no 'orig', since there's no upstream version of the file. We therefore should have ['1', '2', '3', ...]

volodymyr

Aug. 6, 2014, 3:20 p.m.

I started numeration from 0 if that's OK. If not, I'll fix it.

reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js (Diff revision 2)

An issue was opened. Show all issues

Doesn't this end up adding one too many ticks for file attachments?

volodymyr

Aug. 6, 2014, 3:20 p.m.

Right now numRevisions is numTotal - 1, mimicing revisions definition for diffs. We once discussed this in https://reviews.reviewboard.org/r/5824/ (that issue is still open). I think it might be a bit misleading to have different definitions for revisions in different cases (maybe it should be called otherwise then), e.g. numTotal for attachments and numTotal - 1 for diffs. While taking the defition from diffs isn't perfect as well, I think it works better, because:
1) We actually are uprgarding a file attachement - there is an initial version and subsequent revisions to it.
2) In several places of the code we won't have to do

if (isFileAttachment) {
numRevisions = ...;
} else {
numRevisions = ...;
}

and just use the same logic in both cases.

reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 2)

The issue has been resolved. Show all issues

You're passing one single argument to the super render, which is an array of stuff. Based on your implementation above, each of these should just be additional arguments to call.

Can you thoroughly test diffs and interdiffs to make sure you're not causing any regressions?

reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
This should use _super and call
```
reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Shouldn't have an array around ev.
```

Change Summary:

Addressed feedback, retested both attachment and diff selectors - they work as expected.

Diff:

Revision 3 (+290 -112)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
	reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
	reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
    reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
    reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js



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

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
    reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
    reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js

Status:: Discarded
Change Summary:: Obsoleted by /r/6342/