675352 - Mark or check reviewed files

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Splinter, defect)

Description

[David Lawrence [:dkl]]

Attachment: Patch to add checkbox to show a file as reviewed (v1)

+++ This bug was initially created as a clone of Bug #669529 +++

Splinter is *GREAT* for reviewing a patch with a lot of files, but now I'm finding I lose my place in the review process as I switch between files, go to lunch, use another computer, etc.

It'd be nice to have a way to save our place in the review process. Just being able to check off which files are done would be extremely helpful. For example in the old diff view you could do this by collapsing reviewed files or something.

Comment 1

[David Lawrence [:dkl]]

I have installed this on https://bugzilla-stage-tip.mozilla.org for others to try out and give feedback before going live. dvander please give it a try.

dkl

Comment 2

[:glob ✱]

Comment on attachment 549489 [details] [diff] [review]
Patch to add checkbox to show a file as reviewed (v1)

Review of attachment 549489 [details] [diff] [review]:
-----------------------------------------------------------------

[+] extensions/InlineHistory/Config.pm (new file) [ ] File Reviewed

the word 'file' is redundant, and should be removed.

the tooltip is: "If this box is checked, then a review has been completed for this file."
this needs to be reworded to indicated that the field has no baring on the generated review.
perhaps: "Indicates that a review has been completed for this file.  This is for tracking purposes only and has no effect on the published review."

::: extensions/Splinter/web/splinter.js
@@ +1151,2 @@
>          var propertyName = this._reviewPropertyName(bug, attachment);
> +        if (!extraProps) { extraProps = {}; }

remove the braces if you're going to collapse onto a single line:
  if (!extraProps) extraProps = {};

@@ +1376,5 @@
>      var draftSaved = false;
>      if (Splinter.haveDraft()) {
> +        var i, filesReviewed;
> +        filesReviewed = {};
> +        for (i = 0; i < Splinter.thePatch.files.length; i++) {

you could just define i inside the for:
  var filesReviewed = {};
  for (var i = 0; i < Splinter.thePatch.files.length; i++) {

@@ +2134,5 @@
> +                    file.fileReviewed = true;
> +                }
> +                else {
> +                    file.fileReviewed = false;
> +                }

this would be easier to write as:
  file.fileReviewed = checkbox.checked;

@@ +2275,5 @@
>                      Dom.get("restoredLastModified").innerHTML = Splinter.Utils.formatDate(new Date(storedReviews[i].modificationTime));
> +                    // Restore file reviewed checkboxes
> +                    if (storedReviews[i].filesReviewed) {
> +                        var j;
> +                        for (j = 0; j < Splinter.thePatch.files.length; j++) {

declare the loop variable inside the for:
  for (var j = 0; j < Splinter.thePatch.files.length; j++) {

Comment 3

[David Lawrence [:dkl]]

(In reply to comment #2)
> the word 'file' is redundant, and should be removed.

Fixed.
 
> the tooltip is: "If this box is checked, then a review has been completed
> for this file."
> this needs to be reworded to indicated that the field has no baring on the
> generated review.
> perhaps: "Indicates that a review has been completed for this file.  This is
> for tracking purposes only and has no effect on the published review."

Fixed.
 
> >          var propertyName = this._reviewPropertyName(bug, attachment);
> > +        if (!extraProps) { extraProps = {}; }
> 
> remove the braces if you're going to collapse onto a single line:
>   if (!extraProps) extraProps = {};

JSLint based best practices states to always use {} for blocks for readability so I changed it to:

if (!extraProps) {
    extraProps = {};
}

> >      var draftSaved = false;
> >      if (Splinter.haveDraft()) {
> > +        var i, filesReviewed;
> > +        filesReviewed = {};
> > +        for (i = 0; i < Splinter.thePatch.files.length; i++) {
> 
> you could just define i inside the for:
>   var filesReviewed = {};
>   for (var i = 0; i < Splinter.thePatch.files.length; i++) {

Fixed.

> > +                    file.fileReviewed = true;
> > +                }
> > +                else {
> > +                    file.fileReviewed = false;
> > +                }
> 
> this would be easier to write as:
>   file.fileReviewed = checkbox.checked;

Yes much cleaner. Fixed.

> >                      Dom.get("restoredLastModified").innerHTML = Splinter.Utils.formatDate(new Date(storedReviews[i].modificationTime));
> > +                    // Restore file reviewed checkboxes
> > +                    if (storedReviews[i].filesReviewed) {
> > +                        var j;
> > +                        for (j = 0; j < Splinter.thePatch.files.length; j++) {
> 
> declare the loop variable inside the for:
>   for (var j = 0; j < Splinter.thePatch.files.length; j++) {

The original reasoning for some of them was if multiple for loops inside a function or block reuse the same iterator, only declare it once. For example:

var i;

for (i = 0; i < something; i++) { }

for (i = 0; i < somethingelse; i++) { }

I got into a habit of always declaring the iterator outside the for loop. But yes it doesn't make sense for only one for loop in a function.

dkl

Comment 4

[David Lawrence [:dkl]]

Attachment: Patch to add checkbox to show a file as reviewed (v2)

New patch with suggested changes.

dkl

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Is there a way to decorate the name of reviewed files as they appear in the horizontal strip of names? If so that would be the easiest way for reviewers to scan for unmarked files, but if not this works too!

Comment 6

[David Lawrence [:dkl]]

Attachment: Patch to add checkbox to show a file as reviewed (v3)

Removed some unrelated code that fixed a different issue. Please review this patch instead.

dkl

Comment 7

[David Lawrence [:dkl]]

(In reply to comment #5)
> Is there a way to decorate the name of reviewed files as they appear in the
> horizontal strip of names? If so that would be the easiest way for reviewers
> to scan for unmarked files, but if not this works too!

Sure. We could do it low-tech and just add a strike-through or better would be to add a small checked icon next to the filename when the "Reviewed" checkbox is set. Let me try something.

dkl

Comment 8

[David Lawrence [:dkl]]

Attachment: Patch to add checkbox to show a file as reviewed (v4)

Here is a revised patch that does a strike-through of the filename in the navigation if the review has been checked off. Please let me know what you think. I will push it up to bugzilla-stage-tip.mozilla.org as well.

dkl

Comment 9

[:glob ✱]

Comment on attachment 550085 [details] [diff] [review]
Patch to add checkbox to show a file as reviewed (v4)

r=glob

this looks good.

Comment 10

[David Lawrence [:dkl]]

(In reply to David Anderson [:dvander] from comment #5)
> Is there a way to decorate the name of reviewed files as they appear in the
> horizontal strip of names? If so that would be the easiest way for reviewers
> to scan for unmarked files, but if not this works too!

I have this installed now on https://bugzilla-stage-tip.mozilla.org and also code review is complete. Can you take a look at the changes with some test patches and see if you like it? I would like to get this out soon if it looks good to you.

Thanks
dkl

Comment 11

[David Lawrence [:dkl]]

dvander, if you could give some feedback on the new Reviewed checkboxes in Splinter on https://bugzilla-stage-tip.mozilla.org, I can get this rolled out this week or next.

Thanks
Dave

Comment 12

[David Lawrence [:dkl]]

Committed to BZR for releasse with the next code push. Please open a new bug for any changes that you see need to be done.

Committing to: bzr+ssh://[e-mail]/bmo/4.0
modified extensions/Splinter/web/splinter.css
modified extensions/Splinter/web/splinter.js                                                                    Committed revision 7853

dkl

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Sorry for the late reply - this is exactly what I was looking for, noticed it was available today, and it was a huge help. Thank you!