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

RESOLVED FIXED

Status

P3
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jfrench, Assigned: camd)

Tracking

Details

(URL)

Attachments

(1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Curious if Ed can repro, adding a need info.
Flags: needinfo?(emorley)
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.
(Reporter)

Comment 3

4 years ago
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.
Flags: needinfo?(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?
(Reporter)

Comment 5

4 years ago
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'.
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.
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.
Assignee: nobody → wkocher
Created attachment 8633538 [details] [review]
PR 756

This seems to get the toggle to actually work. It must've been broken by some refactoring or something?
Attachment #8633538 - Flags: review?(emorley)
(Reporter)

Comment 9

4 years ago
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 on attachment 8633538 [details] [review]
PR 756

Cameron wrote the filtering code/knows it more than I, so deferring to his better judgement :-)
Attachment #8633538 - Flags: review?(emorley) → review?(cdawson)
(Assignee)

Comment 11

4 years ago
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.  :)
Attachment #8633538 - Flags: review?(cdawson)
(Reporter)

Comment 12

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

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

3 years ago
Hey Wes - Sorry to have stolen this one from you.  I had forgotten about your PR here.  It somehow slipped through the cracks.
(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 :-)
Assignee: wkocher → cdawson
You need to log in before you can comment on or make changes to this bug.