1106646 - Shortcut for toggle pending/running/in-progress appears to only sporadically work.

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P3)

Description

[Jonathan French (:jfrench)]

When looking at production (or running locally) when I have a result set with either running or pending jobs, and I type "i" to toggle them, often nothing happens.

To reproduce:
o go to production
o open any result set with in-progress jobs, eg. in a single page
o type "i" to toggle them

Expected:
The jobs get toggled.

Observed:
Almost all of the time, they don't get toggled.

I wonder if it is just me, or if anyone else can reproduce it.

Comment 1

[Jonathan French (:jfrench)]

Curious if Ed can repro, adding a need info.

Comment 2

[Phil Ringnalda (:philor)]

It's not just you - for me, it's "u" that I toggle all day long, and I think it was last Thursday or Friday, whenever we got that production push for the emergency try ingestion issue that also brought along some other things, that it went from always working to only working under some mysterious circumstances and not working under other mysterious circumstances.

Comment 3

[Jonathan French (:jfrench)]

I see, thanks Phil. I wonder if this is related to all the filter commits to master on Nov 24th that I think got wrapped up in that production push. Adding camd for visibility.

Comment 4

[Ed Morley [:emorley]]

When this happens, does toggling using the toolbar button work?
Just wondering if this is a keyboard-shortcut-not-working thing (eg due to focus being somewhere that is blocking it) or not?

Comment 5

[Jonathan French (:jfrench)]

For me at least on windows, it's just 'i' that doesn't work, with focus in the main page. 'u' and interactive pending/running filter buttons work fine for me. Perhaps Phil can confirm if he's still encountering problems with 'u'.

Comment 6

[Phil Ringnalda (:philor)]

There was something in that next production push after the one which broke it, which we expected was going to fix it, which as far as I know has completely fixed the 'u' problem. I've never used 'i', but it certainly doesn't seem to work.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

So I was looking into this, and added some logging to the various functions involved in toggling the in progress stuff. And the functions are all getting called. Haven't really looked any further than that so far to figure out why they aren't doing anything, though.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: PR 756

This seems to get the toggle to actually work. It must've been broken by some refactoring or something?

Comment 9

[Jonathan French (:jfrench)]

I just tested Wes's PR branch locally on Nightly and Chrome with a variety of workflows; combining with our quick platform filter, get|next 10(preserves the filter), n/p selection, switching repos(resets the filter), and a few other things. The workflows look good to me :)

Comment 10

[Ed Morley [:emorley]]

Comment on attachment 8633538 [details] [review]
PR 756

Cameron wrote the filtering code/knows it more than I, so deferring to his better judgement :-)

Comment 11

[Cameron Dawson [:camd]]

Comment on attachment 8633538 [details] [review]
PR 756

I've made some suggestions for simplifications that can be made.  But, yeah, this sure was broken by SOME change.  Apologies if it was me...  :/

This is definitely headed in the right direction.  A few tweaks in my suggestions don't HAVE to be part of this, but it will make the functions less brittle, which I think is worth it.  :)

Comment 12

[Jonathan French (:jfrench)]

A note I removed the in-progress shortcut for now in our new html partial thShortcutTable, during the creation of our onscreen keyboard shortcuts in https://github.com/mozilla/treeherder/pull/1005/files. When this bug is fixed we can reinstate the entry then.

<tr><td class="kbd">i</td>
<td>Toggle in-progress (running/pending) jobs</td></tr>

All other related code for the shortcut remains in controllers/main, etc.

Comment 13

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/a71aa90504bd3f169a257195fab63f6d5bdd8b67
Bug 1106646 - Fixes in progress filter shortcut 'i' key

Comment 14

[Cameron Dawson [:camd]]

Hey Wes - Sorry to have stolen this one from you.  I had forgotten about your PR here.  It somehow slipped through the cracks.

Comment 15

[Ed Morley [:emorley]]

(In reply to Cameron Dawson [:camd] from comment #14)
> Hey Wes - Sorry to have stolen this one from you.  I had forgotten about
> your PR here.  It somehow slipped through the cracks.

I don't believe it was waiting on us, it needed post-review tweaks (comment 11).
But either way, it's fixed now which is great - thank you :-)