726500 - 'space' on watched event list should watch/unwatch selected event

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Comment 1

[Javi Rueda :javirid]

Attachment: patch

Explanations at the code.

I'm sorry if there are any language error. I am not an English speaker, but if any is found, I will fix it.

Thank you for reviewing it.

Comment 2

[Javi Rueda :javirid]

Attachment: removing tabs from patch

There were some tabs instead of spaces.

Comment 3

[[e-mail]]

Comment on attachment 631565 [details] [diff] [review]
removing tabs from patch

You're duplicating code, which is wrong. Instead, the getCellValue and setCellValue methods of mWatchView already do the right thing. All you need is the right row index and column.

Comment 4

[Javi Rueda :javirid]

Attachment: patch 2

Could you take a look into this new patch, Neil? Thank you.

The function that is called when a key is pressed has been changed to onKeyPressed to make it easier any future related modification.

A problem I found when testing the patch was when only one child event is unwatched and then SPACE key is pressed, it becomes watched -as expected- but the parent doesn't change to watched. At the setCellValue() function, it seems that the only case it should be changed is when parent is checked and child is unchecked. Is this a bug?

Comment 5

[Javi Rueda :javirid]

Comment on attachment 631768 [details] [diff] [review]
patch 2

There is a line which is longer that the 80 characters limit.

Comment 6

[Javi Rueda :javirid]

Attachment: patch 3

Line length in attachment 631768 [details] [diff] [review] corrected and removed unnecessary invalidate() call.

Comment 7

[[e-mail]]

Comment on attachment 631771 [details] [diff] [review]
patch 3

>+  onKeyPressed: function onKeyPressed(anEvent)
Nobody else calls it anEvent. Please rename it to aEvent. (Same below.)

>+  this.toggleEventWatching = function watchview_toggleEventWatching(anEvent)
[My preference would have been for toggleEventWatched]

>+    var cols = this.mTree.columns;
>+    var colWatch = cols.getColumnFor(document.getElementById("welIsWatched"));
Actually it's a lot easier than that, just use
var colWatched = this.mTree.columns.welIsWatched;

>+    var cws = (!this.getCellValue(idx, colWatch)).toString();
>+
>+    // setCellValue() needs the new value to be passed as a string.
>+    this.setCellValue(idx, colWatch, cws);
I wish there was a better way to write this...
Perhaps renaming "cws" to "newValue" would make the code more readable.
Also move the .toString() to the setCellValue line like this:
var newValue = !this.getCellValue(idx, colWatched);
// setCellValue() needs the new value to be passed as a string.
this.setCellValue(idx, colWatched, newValue.toString());

>                 onselect="viewer.onWatchViewItemSelected();"
>+                onkeypress="viewer.onKeyPressed(event);"
Maybe "onWatchViewKeyPressed" would be better?

Asking surkov for feedback on my variable renaming suggestions.

Comment 8

[Javi Rueda :javirid]

I realized that the event object is in fact not needed in the toggleEventWatched() function, so it is now removed. Waiting for Alexander feedback.

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 631771 [details] [diff] [review]
patch 3

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

> Asking surkov for feedback on my variable renaming suggestions.

I agree on each suggestion

>>+  onKeyPressed: function onKeyPressed(anEvent)
> Nobody else calls it anEvent. Please rename it to aEvent. (Same below.)

right, because 'a' is not an indefinite article but a first letter of 'argument' I believe

Comment 10

[[e-mail]]

Comment on attachment 631771 [details] [diff] [review]
patch 3

OK, r=me with my comment #7 changes addressed. Thanks!

Comment 11

[Javi Rueda :javirid]

Attachment: patch 4. reviewed in comment 7

I think this is the way to attach a patch when it has been reviewed ok except it needed some changes.

Comment 12

[Javi Rueda :javirid]

I addressed the problems in comment 7. Furthermore, I removed the unneeded call to invalidate(). This last change wasn't in the reviewed patch 3, if it is ok, could you check-in patch 4, please Neil? Thank you.

Comment 13

[[e-mail]]

(In reply to Javi Rueda from comment #12)
> I addressed the problems in comment 7. Furthermore, I removed the unneeded
> call to invalidate(). This last change wasn't in the reviewed patch 3
Actually patch 3 had already removed the call, so that's OK anyway.

Comment 14

[[e-mail]]

Pushed dom-inspector changeset 2d993155b488.