Closed Bug 1153956 Opened 5 years ago Closed 5 years ago

Persist the selected revision in the url on perfherder

Categories

(Tree Management :: Perfherder, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mishravikas, Assigned: sabergeass)

References

Details

(Keywords: ateam-summer-of-contribution)

Attachments

(3 files, 3 obsolete files)

We should persist the selected revision (When you click on a particular revision and the tooltip opens) in the url to make it easier to share.
Summary: Persist the selected revision on perfherder in the url → Persist the selected revision in the url on perfherder
This would be really useful indeed.
Assignee: nobody → akhileshpillai
Working locally, will submit a pull request on Wednesday/Thursday when Will is back for review.

updated ui/js/graph.js as

$('#graph').bind('plotclick', function(e, pos, item) {
          if (item) {
            $scope.selectedDataPoint = getSeriesDataPoint(item);
            showTooltip($scope.selectedDataPoint);
            updateSelectedItem();
            if ($.inArray($scope.tooltipContent.revision, $scope.highlightedRevisions) < 0) $scope.highlightedRevisions.push($scope.tooltipContent.revision);
            $scope.updateHighlightedRevisions()
          }
Attached file Fix for bug 1153956 (obsolete) —
Attachment #8620420 - Flags: review?(wlachance)
Comment on attachment 8620420 [details] [review]
Fix for bug 1153956

Hi Akhil, this isn't quite what we want -- you're persisting the tooltip as a selected revision, but we actually want to persist the tooltip itself. I'll post a screenshot of what I want the persisted state to look like when someone loads the page from a URL, just so it's 100% clear.
Attachment #8620420 - Flags: review?(wlachance) → review-
Attached image perfherder tooltip displayed (obsolete) —
This is the view we want to be able to persist. i.e. we want to persist the tooltip that pops up for the datapoint.
Ok sounds good, I will start working on that.
Will, Updated the pull request. Please have a look when you get a chance. Thanks.
Attachment #8620420 - Attachment is obsolete: true
Attachment #8620700 - Flags: review?(wlachance)
Comment on attachment 8620700 [details] [review]
Fix for bug 1133519 - Tooltip instead of Datapoint revision.

Coming along well! This still needs a bit more work to cover edge cases before it can go in.
Attachment #8620700 - Flags: review?(wlachance) → review-
Comment on attachment 8620700 [details] [review]
Fix for bug 1133519 - Tooltip instead of Datapoint revision.

Hi Will,

Apologies, this one is taking longer. 

Still not a complete fix. However, it addresses the following:

1)Working : Zoomed into a particular area, the tooltip comes up. 
  Not Ideal: The tooltip is not locked, as soon as I move the cursor the tooltip hides. Not sure how to fix that. Any pointers are welcome

2) Working : If I change the time range of points selected or just click on the graph to dismiss the tooltip, the tooltip state is removed from the URL.
  Not Working/Eratic Behavior: two consecutive zoom to different areas removes tooltip state from url. However just one zoom does not.
Attachment #8620700 - Flags: review- → review?(wlachance)
(In reply to Akhilesh Pillai from comment #11)
> Comment on attachment 8620700 [details] [review]
> Fix for bug 1133519 - Tooltip instead of Datapoint revision.
> 
> Hi Will,
> 
> Apologies, this one is taking longer. 

Hi Akhil, no problem. It turns out this is more involved. The plus side is that these sorts of bugs are great learning opportunities. :)

> Still not a complete fix. However, it addresses the following:
> 
> 1)Working : Zoomed into a particular area, the tooltip comes up. 
>   Not Ideal: The tooltip is not locked, as soon as I move the cursor the
> tooltip hides. Not sure how to fix that. Any pointers are welcome

Try running $scope.$digest() after creating the tooltip. The locked state is keyed off of that, I think we just need to update the UI to reflect internal changes. Flot's events don't trigger angular digest cycles, so we need to handle that ourselves.

This article might help a bit: http://www.sitepoint.com/understanding-angulars-apply-digest/

> 2) Working : If I change the time range of points selected or just click on
> the graph to dismiss the tooltip, the tooltip state is removed from the URL.
>   Not Working/Eratic Behavior: two consecutive zoom to different areas
> removes tooltip state from url. However just one zoom does not.

Not sure about this one-- the zoom event is keyed off of the "plotselected" event in flot. Maybe digging in there would be helpful: https://github.com/mozilla/treeherder/blob/master/ui/js/graphs.js#L246

Also, (1) your patch needs to be rebased against master before it can be merged and (2) I left some minor comments to be addressed in reviewable.io.

Great progress, looking forward to seeing this go in once we've shaved off the rough edges. :)
Comment on attachment 8620700 [details] [review]
Fix for bug 1133519 - Tooltip instead of Datapoint revision.

Good progress, a bit more work still required.
Attachment #8620700 - Flags: review?(wlachance) → review-
Hi Will,

Updated the pull request. 

There is still one thing that I am unable to solve. If I zoom in and select a tooltip the tooltip persists. However on zooming to a different area the tooltip is still visible.
Attachment #8620441 - Attachment is obsolete: true
Attachment #8620700 - Attachment is obsolete: true
Attachment #8627171 - Flags: review?(wlachance)
(In reply to Akhilesh Pillai from comment #14)
> Hi Will,
> 
> Updated the pull request. 
> 
> There is still one thing that I am unable to solve. If I zoom in and select
> a tooltip the tooltip persists. However on zooming to a different area the
> tooltip is still visible.

I actually couldn't reproduce this problem, but I think you should just remove the tooltip if the user zooms to a different area. Doing so implies that you're dismissing it. 

Also, same problem as bug 1174259: your PR contains changes for two seperate issues. Let's land bug 1174259 first and rebase this work on top of it. :)

Thanks for your efforts! These features will be really nice to get in.
Comment on attachment 8627171 [details] [review]
Fix for bug Bug 1153956 - Almost there

See above.
Attachment #8627171 - Flags: review?(wlachance) → review-
I don't think :akhil has time to work on this, but I think it would be really useful for the alerts view. We should finish this.
Assignee: akhileshpillai → nobody
Blocks: 1201154
(In reply to William Lachance (:wlach) from comment #18)
> I don't think :akhil has time to work on this, but I think it would be
> really useful for the alerts view. We should finish this.

Ok Will, I would like to work on this, but what should I do? I mean should I rewrote it from beginning(which i think it's no necessary) or just fork :akhil's branch?
Attached file PR for bug 1153956
Hi Will,

Due to I failed to fork and rebase branch from :akhil, I decide to create a new PR for it and ask review. I hope it's ok with you and akhil. :)
Assignee: nobody → sabergeass
Status: NEW → ASSIGNED
Attachment #8658571 - Flags: review?(wlachance)
Hi Mike, thank you for working on this, please do the needful.
Comment on attachment 8658571 [details] [review]
PR for bug 1153956

One minor issue I noticed in the PR, aside from that this is super close. :)
Attachment #8658571 - Flags: review?(wlachance) → review-
Attachment #8658571 - Flags: review- → review?(wlachance)
Comment on attachment 8658571 [details] [review]
PR for bug 1153956

I think this might be the wrong patch?
Attachment #8658571 - Flags: review?(wlachance)
Attachment #8658571 - Flags: review?(wlachance)
Comment on attachment 8658571 [details] [review]
PR for bug 1153956

The code looks fine, however I just realized we probably need to implement this slightly differently if it's going to work in all cases (especially for implementing things like bug 1161249, where it's probable the same performance job might have multiple retriggers). I left a comment in the PR, let me know if you have any questions.
Attachment #8658571 - Flags: review?(wlachance)
(In reply to William Lachance (:wlach) from comment #24)
> Comment on attachment 8658571 [details] [review]
> PR for bug 1153956
> 
how could i get the job_id? are there any api can help me to retrieve it from remote? otherwise I should add one.
Comment on attachment 8658571 [details] [review]
PR for bug 1153956

This PR is not ready to review I think, still some problem in there need to be fixed. But it can persist job id and find the position of data point by job id. Could you give me some advice about it?
Attachment #8658571 - Flags: feedback?(wlachance)
Ok, maybe I'm sure the problem I said in last comment not comes from my PR. You can reproduce it also in remote side of perfherder:

1) copy a url with at least one series (e.g https://treeherder.mozilla.org/perf.html#/graphs?timerange=5184000&series=[mozilla-inbound,fdad6ae27544b0dd52113fce3184968100190e76,1])

2) open a new tab and paste it into the url address line(and jump to it of course)

3) it seems all ok at first, but you will find you couldn't click any datapoint appropriate(e.g like the screenshot shows, the data point haven't been highlight and some errors been found in the console)

Maybe we should left it alone for this PR and file a new one for it? I'm totally defer to you opinion:)

Thank you Will
Attachment #8666440 - Flags: feedback?(wlachance)
Sorry, the step 2 should be changed to *delete that series we just add to serieslist and then paste the url to address link. You should see that error after that.
Comment on attachment 8666440 [details]
Screen Shot 2015-09-27 at 3.55.58 PM.png

Haven't tested but this looks like it's on the right track. Left a few comments, feel free to ask for another review when you're happy with the behaviour and fixed the nits. :)
Attachment #8666440 - Flags: feedback?(wlachance) → feedback+
Comment on attachment 8658571 [details] [review]
PR for bug 1153956

See comments on the screenshot:

Haven't tested but this looks like it's on the right track. Left a few comments, feel free to ask for another review when you're happy with the behaviour and fixed the nits. :)
Attachment #8658571 - Flags: feedback?(wlachance) → feedback+
Comment on attachment 8666440 [details]
Screen Shot 2015-09-27 at 3.55.58 PM.png

Eek, didn't mean to provide feedback on this.
Attachment #8666440 - Flags: feedback+
Attachment #8658571 - Flags: review?(wlachance)
Comment on attachment 8658571 [details] [review]
PR for bug 1153956

Ok I tested this and it mostly works really well! Just a few problems I noticed in testing. Fortunately they're easy to fix. :) Address them, rebase, and let's get this merged!

Thanks so much for your efforts on this!
Attachment #8658571 - Flags: review?(wlachance) → review-
Comment on attachment 8658571 [details] [review]
PR for bug 1153956

np, thank you for your help!

I think actually I should thank you :)  I had learn a ton of things from you and other mozillians. Your attitude and passions to open source make the Internet become better! That's the reason why I keep work on treeherder and other stuff of Mozilla. When I was in hospital, I had read a lot of news and story before about mozilla which including Rust and Servo.

However, I think what you people doing here are awesome and hope I can contribute my pygmy effort to A-team. The shame is the things I know for now is too callow to help, sometimes may takes you guys a lot of time to explain idea to me. I certainly can work on much more things and more efficient than now if I can become a knowledgeable people like other stuff in Mozilla like you and Joel! I will keep learning anyway, hope I can become a better mozillian some days :)

MikeLing
Attachment #8658571 - Flags: review- → review?(wlachance)
Comment on attachment 8658571 [details] [review]
PR for bug 1153956

Some more required here, there need to be some followup changes to go along with the ones I explicitly requested.
Attachment #8658571 - Flags: review?(wlachance)
Comment on attachment 8658571 [details] [review]
PR for bug 1153956

Sorry about hurry to ask review last time. I believe it's good to review this time.
Attachment #8658571 - Flags: review?(wlachance)
Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/882daff157ef66e7121f7c37cffee2356ad28eba
Bug 1153956 - Persist the selected revision in the url on perfherder

Based on work from Akhilesh Pillai (akhileshpillai@gmail.com)
Comment on attachment 8658571 [details] [review]
PR for bug 1153956

Yup, this is good. Thanks Mike!
Attachment #8658571 - Flags: review?(wlachance) → review+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.