Mark or check reviewed files

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: dkl, Assigned: dkl)

Tracking

Production

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

8 years ago
Created attachment 549489 [details] [diff] [review]
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.
Attachment #549489 - Flags: review?(glob)
(Assignee)

Comment 1

8 years ago
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
Status: NEW → ASSIGNED
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++) {
Attachment #549489 - Flags: review?(glob) → review-
(Assignee)

Comment 3

8 years ago
(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
(Assignee)

Comment 4

8 years ago
Created attachment 549890 [details] [diff] [review]
Patch to add checkbox to show a file as reviewed (v2)

New patch with suggested changes.

dkl
Attachment #549489 - Attachment is obsolete: true
Attachment #549890 - Flags: review?(glob)
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!
(Assignee)

Comment 6

8 years ago
Created attachment 549896 [details] [diff] [review]
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
Attachment #549890 - Attachment is obsolete: true
Attachment #549890 - Flags: review?(glob)
Attachment #549896 - Flags: review?(glob)
(Assignee)

Comment 7

8 years ago
(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
(Assignee)

Comment 8

8 years ago
Created attachment 550085 [details] [diff] [review]
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
Attachment #549896 - Attachment is obsolete: true
Attachment #549896 - Flags: review?(glob)
Attachment #550085 - Flags: review?(glob)
Comment on attachment 550085 [details] [diff] [review]
Patch to add checkbox to show a file as reviewed (v4)

r=glob

this looks good.
Attachment #550085 - Flags: review?(glob) → review+
(Assignee)

Comment 10

8 years ago
(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
(Assignee)

Comment 11

7 years ago
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
(Assignee)

Comment 12

7 years ago
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://dlawrence%40mozilla.com@bzr.mozilla.org/bmo/4.0
modified extensions/Splinter/web/splinter.css
modified extensions/Splinter/web/splinter.js                                                                    Committed revision 7853

dkl
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
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!
You need to log in before you can comment on or make changes to this bug.