1089737 - Add shortcuts for previous/next job navigation (that cycle through all jobs, not just unclassified ones)

Status: VERIFIED FIXED

(Tree Management :: Treeherder, defect, P3)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

I like how we expand/highlight a job when we are viewing it- unfortunately it is so much that I have to click outside of the job in order to investigate a job next to it.

It sounds like this was sort of agreed upon as ok, I am not passionate about this one way or another, but others might find it frustrating as well.

Comment 1

[Jonathan French (:jfrench)]

Maybe as some options we could try:

a) adjust the margin of the unselected jobs to reveal adjacent ones (less compact layout)
b) create some new navigation keys to step forwards/backwards one job at a time (left/right arrow)
c) switch back to the earlier, slightly less conventional transparent view for the scaled-job
https://bug1075272.bugzilla.mozilla.org/attachment.cgi?id=8503769
d) ?.. maybe others

I kind of like b) since it gives us new functionality and if you can't see the job you can just step into it, and back out of it. If we wanted to see it also, we could do some sort of combination of b) and c) with the transparent scaled job.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

I sort of like 'a', but thinking more on this 'b' could provide us with a new feature which is a good win.

Comment 3

[Rakesh]

I am working on implementing navigation system as pointed put in 'b',

Comment 4

[Rakesh]

I completed new navigation keys to step forwards/backwards one job at a time. I used parent child to select next/prev job. please review .

https://github.com/dewgeek/treeherder-ui/tree/bug1089737

Comment 5

[Jonathan French (:jfrench)]

Hi dewgeek, I tried your branch, the behavior looks pretty cool!

A few observations:

o With no job selected and issuing a left or right arrow key, it appears to yield an undefined js error. While fixing that, I think the desired behavior if the right arrow key is depressed with no job selected (both in the default multiple-resultset page view, or in a single resultset view) is to select the 'first' job on the page. Similarly, if no job is selected and the _left arrow key is depressed to go backwards, to select the 'last' job on the page. This will behave consistently with next/prev unclassified failure shortcut navigation which loops in a similar way.

o I had a quick look at the branch history. Fwiw, it might make sense to rebase on to latest master first, so it's up to date. And also remove the unrelated commits for bug 1060058 that are in the branch history. So you end up with the branch above containing: a clean upstream(master) + your changes just for this bug. You may also want to squash ae0b54d onto your primary 01a41c68, unless you really need them separated.

Before camd's review, a few things we can improve on during the changes (some are legacy and not your changes).

Function parameters should be separated by white space, as should the braces for any conditional blocks you happen to touch during the change (ie. feel free to fix them). Appropriate lines need to be terminated with a semi-colon. You can run the file through jshint if you like also.

In case this helps, Google and Mozilla have reasonable styleguides here
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests/Mozmill_Style_Guide
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_practices
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml

Comment 6

[Rakesh]

I made the changes and new PR. please pull again and review. 

https://github.com/dewgeek/treeherder-ui/tree/bug1089737
https://github.com/mozilla/treeherder-ui/pull/326

Comment 7

[Ed Morley [:emorley]]

Attachment: treeherder-ui PR #326: Add New navigation keys to step forwards/backwards one job at a time

Adding PR redirect attachment & review request on behalf of @dewgeek

Comment 8

[Jonathan French (:jfrench)]

(In reply to Rakesh from comment #6)
> I made the changes and new PR. please pull again and review. 
> 
> https://github.com/dewgeek/treeherder-ui/tree/bug1089737
> https://github.com/mozilla/treeherder-ui/pull/326

Hi Dewgeek, per my comments in the second half of comment 5, a variety of style-compliant js changes will need to be made, and a few other style tweaks for the commit. I will comment in the PR directly before camd gets to the main code review, so you can adjust it as needed.

Comment 9

[Rakesh]

thank you for the comments. I have made the changes as suggested .

Comment 10

[Jonathan French (:jfrench)]

Hi Dewgeek, after making all my preliminary PR comments I re-ran your branch locally, the new functionality seems to be working for me on both Firefox and Chrome (at least on windows). Nice! 

That js error mentioned in comment 5 is now gone.

If you have the opportunity to look at that 'wrap around' 1st/last selected job behavior also mentioned in comment 5 that would be great. It's maybe not critical, but I can envision Sheriffs eventually expecting it, since next/prev unclassified failure navigation works that way. It would probably be good from a consistency standpoint.

You can see the behavior with next/prev unclassified failure shortcuts, by clicking on the first unclassified failure (a job without a * beside it) eg.in the Try repo, and then pressing the 'p' key. It will go to the "previous" unclassified failure, which in this case will wrap around to the "last" one on the page. Same with 'next' if you are on the last unclassified failure, it will loop back to the "first" on the page.

It would be a nicety if the next/prev job navigation did the same thing.

Comment 11

[Jonathan French (:jfrench)]

(In reply to Rakesh from comment #9)
> thank you for the comments. I have made the changes as suggested .

Hi Dewgeek, I checked the new changes, there are still still some be done from my original comments. You can expand all of the 'Show outdated diff' links in the Github conversation thread to check for the remainder to be done. Thanks!

Comment 12

[Rakesh]

Yeah, I will add wrap around feature. It will better as it will be closing the loop connecting first/last element. thanks for suggestion.

Comment 13

[Rakesh]

Added spaces before/after control statements, removed trail spaces, Added wrap up functionality.
please go through changes and comment if anything is out of order.
thank you :)

Comment 14

[Jonathan French (:jfrench)]

Hi Rakesh, cool the next/prev job looping is working for me on your latest branch.

These still remain to be done, per my original Github comments:

The two emits need to still be modified to one line each, instead of 6 lines
https://github.com/mozilla/treeherder-ui/pull/326#discussion-diff-22436961

There is newline whitespace still in clonejobs.js, please set your editor to expose it
https://github.com/mozilla/treeherder-ui/pull/326#discussion_r22437069

Please review the Mozilla and Google js style guides from comment 5, and make the coding styles compliant where possible with the examples shown on those pages. Examples include, a single whitespace between the closing paren and curly brace in a function definition, if,else/while should not have open lines between them and their contents, etc. The Google style guide is particularly comprehensive and covers most of the code involved in your changes.

I will add a few minor things directly on the latest files in your PR also.

Comment 15

[Cameron Dawson [:camd]]

Rakesh - I apologize I didn't review this today.  I PROMISE I will Monday.  Today just had too many other items on my plate.  Thanks for your patience.

Comment 16

[Rakesh]

Hi cameron, I won't be online during next week. I will be back on next sunday. thank you in advance for going through my PR :)

Comment 17

[Cameron Dawson [:camd]]

Comment on attachment 8543551 [details] [review]
treeherder-ui PR #326: Add New navigation keys to step forwards/backwards one job at a time

This is a great first-effort on the problem.  Our DOM is pretty complex, so kudos for having the tenacity to figure this out!

Comment 18

[Rakesh]

Hi camd, 
I added the method you suggested for selecting next/previous job and Mousetrap callbacks in controllers/main.js as suggested by :jfrench .

Comment 19

[Cameron Dawson [:camd]]

Rakesh: awesome!  I'll take a look.  We had to revert the Mousetrap stuff due to a bug.  But when that's fixed and re-merged, then we can see about merging this, too.  Since I know this relies on Mousetrap for keys now.  Sorry for the hassle on that!

Comment 20

[Cameron Dawson [:camd]]

Comment on attachment 8543551 [details] [review]
treeherder-ui PR #326: Add New navigation keys to step forwards/backwards one job at a time

Putting this back in my review queue

Comment 21

[Jonathan French (:jfrench)]

Rakesh, Camd, if you prefer to just use the old ev.keyCode approach for now in this PR, that is fine with me. I can update the next/prev job shortcuts later to mousetrap.

Comment 22

[Rakesh]

Camd, jfrench , I am using the old ev.keyCode and updating my PR.

Comment 23

[Cameron Dawson [:camd]]

Comment on attachment 8543551 [details] [review]
treeherder-ui PR #326: Add New navigation keys to step forwards/backwards one job at a time

This is getting really close!  Thanks for tackling this!

Comment 24

[Rakesh]

Attachment: patch for Bug 1089737 - Create shortcuts for next/previous job navigation

Comment 25

[Jonathan French (:jfrench)]

Comment on attachment 8558341 [details] [review]
patch for Bug 1089737 -  Create shortcuts for next/previous job navigation

I'm seeing 404/js console errors and no jobs loading when running locally with this latest branch. Can you reproduce Rakesh?

Master is fine for me.

I am pointing to stage data at https://treeherder.allizom.org, which the known working backend at the moment, when using a local server.

Comment 26

[Cameron Dawson [:camd]]

Comment on attachment 8558341 [details] [review]
patch for Bug 1089737 -  Create shortcuts for next/previous job navigation

Almost there.  A few more updates should do it.

Comment 27

[Rakesh]

As I already mentioned in github , It will increase the duplicate code . So, please go through my PR again. waiting for it to be merged :)

Comment 28

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder-ui

https://github.com/mozilla/treeherder-ui/commit/e92ee8efa16d77fd5b8891d80d65ba97c61cc919
Bug 1089737 - Create shortcuts for next/previous job navigation

https://github.com/mozilla/treeherder-ui/commit/a44bcbbf4cfa11c4d6515188ec02a44490e2835b
Merge pull request #326 from dewgeek/bug1089737

Bug 1089737 - Create shortcuts for next/previous job navigation

Comment 29

[Jonathan French (:jfrench)]

One thing that still remains to be done is to unbind/protect/block the 'next' and 'previous' keyboard events when the classification or related-bug input fields are focused. Otherwise if the user uses < or > while typing a classification comment (eg to move their text cursor), the selected job will shift around.

Comment 30

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder-ui

https://github.com/mozilla/treeherder-ui/commit/56160d9cb1b2ca3e0a9d73e01612c0e34f5d4a92
Bug 1089737 - Tweak next/prev shortcut comment

https://github.com/mozilla/treeherder-ui/commit/b7ffd95be8566fac4aecde30de40bca7c5befd25
Merge pull request #384 from tojonmz/prevnext-tweak

Bug 1089737 - Tweak next/prev shortcut comment

Comment 31

[Jonathan French (:jfrench)]

Attachment: treeherder-ui-PR#403-supplemental

Supplemental PR for unbinding left/right keys when in mousetrap enabled inputs per comment 29. Please see PR for status and review.

Comment 32

[William Lachance (:wlach)]

Comment on attachment 8572844 [details] [review]
treeherder-ui-PR#403-supplemental

This makes sense, thanks! Assuming you've tested it, please feel free to merge.

Comment 33

[Jonathan French (:jfrench)]

Yup, I had done integration testing yesterday with all the other shortcuts and workflows, and I also re-checked it locally this morning. So I think we are fine to merge.

Comment 34

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder-ui

https://github.com/mozilla/treeherder-ui/commit/cfa8e564782ae3fe11a82defb0e0987cf27147f6
Bug 1089737 - Unbind left/right navigation in mousetrap inputs

https://github.com/mozilla/treeherder-ui/commit/5caa7956fa36f8029ef18d87abaf22277d692a15
Merge pull request #403 from tojonmz/protect-leftright-nav

Bug 1089737 - Unbind left/right navigation in mousetrap inputs

Comment 35

[Jonathan French (:jfrench)]

I will re-test the above once it has landed on stage/prod.

Comment 36

[Jonathan French (:jfrench)]

Verified the above supplemental PR403 is fixed/working on stage and prod.

Comment 37

[Jonathan French (:jfrench)]

We have one more tweak to do, the Help didn't get updated with the new shortcuts. I will do that now.

Comment 38

[Jonathan French (:jfrench)]

Attachment: treeherder-ui-PR#406

Please see above tweak PR, for review and status.

Comment 39

[William Lachance (:wlach)]

Comment on attachment 8573507 [details] [review]
treeherder-ui-PR#406

Makes sense, thanks! Feel free to merge.

Comment 40

[Jonathan French (:jfrench)]

Thanks wlach, will do.

Comment 41

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder-ui

https://github.com/mozilla/treeherder-ui/commit/39f041f9dcbca45a35b47ff4317e5e24781ee7f3
Bug 1089737 - Update Help shortcuts for left/right navigation

https://github.com/mozilla/treeherder-ui/commit/3ca9855a4815932c7ed75e45c0c1ffcdd6c06cda
Merge pull request #406 from tojonmz/update-help-leftright

Bug 1089737 - Update Help shortcuts for left/right navigation

Comment 42

[Jonathan French (:jfrench)]

Marking fixed per above merge, and thanks dewgeek for this awesome work!

The feature has been well received by sheriffs so far, and jmaher has confirmed this item b) from the bug description addresses his user problem. I will verify on the next push to stage/prod.

Comment 43

[Jonathan French (:jfrench)]

Verified the final fix for the Help update is on stage, so marking Verified.

Comment 44

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/712de15c0a7531971cb98ec1a9385e24fc4bd67b
Bug 1089737 - Create shortcuts for next/previous job navigation

https://github.com/mozilla/treeherder/commit/5e4d238146e51b1e0a755bc571feef5cefd4288c
Merge pull request #326 from dewgeek/bug1089737

Bug 1089737 - Create shortcuts for next/previous job navigation

https://github.com/mozilla/treeherder/commit/40b963b9a2c23abef628161e26cbb5b7aae31458
Bug 1089737 - Tweak next/prev shortcut comment

https://github.com/mozilla/treeherder/commit/1a789df3ffe0a8aad6d802fd9882be897319df4b
Merge pull request #384 from tojonmz/prevnext-tweak

Bug 1089737 - Tweak next/prev shortcut comment

https://github.com/mozilla/treeherder/commit/132151517b3e3dec149d0c2b4640a9ddcc21442e
Bug 1089737 - Unbind left/right navigation in mousetrap inputs

https://github.com/mozilla/treeherder/commit/2faca26c3d2ca01f5a0ab248f0d591b047c1023a
Merge pull request #403 from tojonmz/protect-leftright-nav

Bug 1089737 - Unbind left/right navigation in mousetrap inputs

https://github.com/mozilla/treeherder/commit/4db4607167b25eaa13a487afa2fd18d291cfca1b
Bug 1089737 - Update Help shortcuts for left/right navigation

https://github.com/mozilla/treeherder/commit/de10818cc3e3e1dd758476ac07a4c5a1bedee1d2
Merge pull request #406 from tojonmz/update-help-leftright

Bug 1089737 - Update Help shortcuts for left/right navigation