576544 - Provide UI for user/unstarred filters in tbpl and make them non-destructive

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, enhancement)

Description

[Daniel Holbert [:dholbert]]

Often, a developer who's watching TBPL actually only wants to follow (or look up) a single push's 'row'.  This is particularly the case on TryServer, where consecutive pushes have absolutely nothing to do with one another & just get in the way.

It would be *awesome* if tbpl offered a view to show only one 'row' at a time -- e.g. http://tests.themasta.com/tinderboxpushlog/?tree=MozillaTry&rev=f20253b116ff would just show the push that includes that changeset.

This could hopefully speed up load time at least a little bit, and it would de-clutter the interface a lot, for the use cases where it'd be appropriate.

Comment 1

[Daniel Holbert [:dholbert]]

Note that if this bug were fixed, it would help tremendously for describing filing future TBPL bugs.

That is to say: If I notice an issue on TBPL for a particular pushlog-line and want to file a bug on it, it's difficult to give steps to reproduce.  ("find the pushlog line for cset XXX on day MM/DD")  However, if I could provide a link directly to that tbpl pushlog-line, it would be much easier for others to test & debug the issue.

Comment 2

[(no longer active)]

This is not easy to fix given our current way of retrieving the data (by "not easy" I mean "impossible"!).  We will have support for a real server-based solution in the future, which would make this easy to fix.

Comment 3

[Dave Townsend [:mossop] (PTO until July 20)]

I'm not sure how it'd be impossible. At the very least you could do this:

Load and display all the data as normal (probably for a longer timerange than normal).
Put an attribute on the <li> for each push, say changest="<cset>"
Add a stylesheet that says:

li:not([changeset="<wantedcset>"]) {
  display: none;
}


Hacky and inefficient yes, but it'd do the job.

Comment 4

[(no longer active)]

Currently we can't load any tindebrox json except for the cached one reliably more than 50% of the time.  To demonstrate, try clicking the green "bottom" arrow on m-c's tbpl (or better yet, on try).

Comment 6

[(no longer active)]

Bug 598556 is sort of the back-end support for this.  We need to expose this somehow in the UI.

Comment 7

[Arpad Borsos [:Swatinem]]

Also, with bug 566870 in place, I believe non-destructive filtering (as in keeping all the data in the dom, just hidden, similar to what comment 3 said) is a better approach than removing the pushes before they reach Data.js.

Comment 8

[Mounir Lamouri (:mounir)]

I agree that CSS filtering might be nicer even more considering the approach in bug 566870 doesn't work anymore (it has been broken a few commits after).

Comment 9

[Arpad Borsos [:Swatinem]]

We already have "push-$changeset" as id for the pushes, adding the author as data attribute is trivial. I would also prefer to just jQuery .hide() the nodes instead of generating css rules on the fly. Should be really trivial, I just have no good idea for a nice UI.

Comment 10

[Arpad Borsos [:Swatinem]]

(In reply to comment #8)
> I agree that CSS filtering might be nicer even more considering the approach in
> bug 566870 doesn't work anymore (it has been broken a few commits after).
My bad, fixed by http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/58f485db128d

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #9)
> have no good idea for a nice UI.

How about a search field (labeled "Filter") between the Legend/Tree Info/Etc. block and the N Jobs are failing bock?  Or even just a regular input field (with the button labeled "Filter"), since I assume you'd have to do a submit, rather than it live-filtering the results on paste/typing.  You'd have to arrange the field and the loading indicator to share that space, but that should be possible.

Comment 12

[Mounir Lamouri (:mounir)]

(In reply to comment #11)
> (In reply to comment #9)
> > have no good idea for a nice UI.
> 
> How about a search field (labeled "Filter") between the Legend/Tree Info/Etc.
> block and the N Jobs are failing bock?  Or even just a regular input field
> (with the button labeled "Filter"), since I assume you'd have to do a submit,
> rather than it live-filtering the results on paste/typing.  You'd have to
> arrange the field and the loading indicator to share that space, but that
> should be possible.

Why not a drop down menu like "Tree Info" and "Legend" named "Filters" which would have two input fields: one for revision and one for pusher email. We could update the list on "onchange" or "submit" events for example.

This would keep consistency with the current UI and let us add new filtering parameters.

Comment 13

[Arpad Borsos [:Swatinem]]

(In reply to comment #12)
> (In reply to comment #11)
> > How about a search field (labeled "Filter") between the Legend/Tree Info/Etc.
> > block and the N Jobs are failing bock?  Or even just a regular input field
> > (with the button labeled "Filter"), since I assume you'd have to do a submit,
> > rather than it live-filtering the results on paste/typing.  You'd have to
> > arrange the field and the loading indicator to share that space, but that
> > should be possible.
> 
> Why not a drop down menu like "Tree Info" and "Legend" named "Filters" which
> would have two input fields: one for revision and one for pusher email. We
> could update the list on "onchange" or "submit" events for example.
> 
> This would keep consistency with the current UI and let us add new filtering
> parameters.

That seems like the way forward. We don’t want to reload the the whole page anymore, thats what I mean by non-destructive.

Comment 14

[Mounir Lamouri (:mounir)]

It would be nice ta have an UI for bug 619280 in the meantime.

Comment 15

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - UI to filter unstarred failures

That's one step in the direction of fixing this bug: add the UI to filter unstarred builds. I will look at how to do that for revision and commiter filtering.

Comment 16

[Arpad Borsos [:Swatinem]]

Comment on attachment 504153 [details] [diff] [review]
Part 1 - UI to filter unstarred failures

It’s fine as is, but I would prefer jQuery over dom methods.
Also we would need an accessor for data._pushes as to not access a private member.

As a side note: is "for each" spidermonkey specific?

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - UI to filter unstarred failures

This is adding the getPushes() in Data object and don't use for each (which is indeed not compatible with all browsers).
I'm still not using jQuery given that I don't know the syntax and I don't think using DOM API is harmful...

Comment 18

[Mounir Lamouri (:mounir)]

Part 1 pushed:
https://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/c123db9d81ad

Comment 19

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Field to filter by pusher email address

Comment 20

[Mounir Lamouri (:mounir)]

BTW, this patch removes the filtering from Data because this wasn't really useful.

Comment 21

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Field to filter by revision id

Like part 2, the previous way to filter by revision (from Controller) is removed given that it was not preventing data load.

I think an interesting follow up would be to not show push when filtered.

Comment 22

[Arpad Borsos [:Swatinem]]

Comment on attachment 504155 [details] [diff] [review]
Part 1 - UI to filter unstarred failures

>+    <div class="dropdown"><h2>Filters</h2>
>+      <ul id="filters">
>+        <li><label>Only unstarred<input type='checkbox' id='onlyUnstarred'></label></li>
>+    </ul></div>

Just a small style nit, sorry for overlooking it at first: keep the quoting consistent, as in double quotes.

>+      var pushes = self._controller.valuesFromObject(self._data.getPushes());
>+      for (var i=0; i<pushes.length; ++i) {
>+        self.handleUpdatedPush(pushes[i]);
>+      }

Another style nit: spaces around = and <

Comment 23

[Arpad Borsos [:Swatinem]]

Comment on attachment 504160 [details] [diff] [review]
Part 2 - Field to filter by pusher email address

>   _buildHTMLForPushResults: function UserInterface__buildHTMLForPushResults(push) {
>     var self = this;
>     return Controller.keysFromObject(Config.OSNames).map(function buildHTMLForPushResultsOnOS(os) {
>+      if (self._shouldPushBeFiltered(push))
>+        return '';

This function only generates the html for the push "results", meaning the right part where the jobs are shown, it does not touch the left part, the patch view at all.
It means you would still see all the pushes, but the results only for the filtered one.

Comment 24

[Arpad Borsos [:Swatinem]]

Comment on attachment 504167 [details] [diff] [review]
Part 3 - Field to filter by revision id

>   _getInitialPushRangeParams: function Controller__getInitialPushRangeParams(params) {
>-    if ("rev" in params)
>-      return {
>-        range: { rev: params.rev },
>-        trackTip: false,
>-      };
>-
>     if ("fromchange" in params && "tochange" in params)
>       return {
>         range: { fromchange: params.fromchange, tochange: params.tochange },
>         trackTip: false,
>       };

This actually removes a very useful feature: linking to a single push and making sure that pushlog loads that single push no matter when it was. As I think about it, I’m not sure a UI for this particular filter would be very useful?

Also I forgot to mention the style nits identified in patch 1 are also there for patches 2 and 3.

Comment 25

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Field to filter by pusher email address

This one should be better.

Comment 26

[Mounir Lamouri (:mounir)]

(In reply to comment #24)
> Comment on attachment 504167 [details] [diff] [review]
> Part 3 - Field to filter by revision id
> 
> >   _getInitialPushRangeParams: function Controller__getInitialPushRangeParams(params) {
> >-    if ("rev" in params)
> >-      return {
> >-        range: { rev: params.rev },
> >-        trackTip: false,
> >-      };
> >-
> >     if ("fromchange" in params && "tochange" in params)
> >       return {
> >         range: { fromchange: params.fromchange, tochange: params.tochange },
> >         trackTip: false,
> >       };
> 
> This actually removes a very useful feature: linking to a single push and
> making sure that pushlog loads that single push no matter when it was. As I
> think about it, I’m not sure a UI for this particular filter would be very
> useful?

Interesting. I didn't know this feature (which doesn't seem to be in tbpl.mozilla.org yet). Maybe we could have no UI for rev or something destructive (by reloading the page)?

Comment 27

[Arpad Borsos [:Swatinem]]

Well the "UI" for rev is that the date of a push is a link to the rev=… url.
And yes, the live version was reset to an older version because the newer features actually make things worse. It loads 10 pushes from pushlog and tries to load appropiate tinderbox data for it, which can be A LOT when the 10 pushes are spaced farther apart.

Comment 28

[Mounir Lamouri (:mounir)]

Part 2 pushed:
https://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/d93641372899

Comment 29

[Mounir Lamouri (:mounir)]

Maybe we could have a specific UI for rev, fromchange/tochange and startdate/enddate? These three things seem to behave the same way, do they?

Comment 30

[Arpad Borsos [:Swatinem]]

(In reply to comment #29)
> Maybe we could have a specific UI for rev, fromchange/tochange and
> startdate/enddate? These three things seem to behave the same way, do they?

They behave the same in the sense that they directly map to pushlog calls. Only after pushlog is loaded, we load tinderbox data based on the timestamps of the pushes.
The reason I think a UI for these is not useful is because these options are only relevant for the initial load. So they are not "filters" in that sense, they can’t be made non-destructive.

Comment 32

[Mounir Lamouri (:mounir)]

Marking this resolved. We can open a new bug if we want a UI for rev/range/date filtering.