search

catapult-project / catapult

Deprecated Catapult GitHub. Please instead use http://crbug.com "Speed>Benchmarks" component for bugs and https://chromium.googlesource.com/catapult for downloading and editing source code..
https://chromium.googlesource.com/catapult
BSD 3-Clause "New" or "Revised" License
1.93k stars 563 forks source link

Implement nav by find box functionality #728

Closed catapult-bot closed 9 years ago

catapult-bot commented 9 years ago

Issue by jackjyang Wednesday Jan 21, 2015 at 15:59 GMT Originally opened as https://github.com/google/trace-viewer/pull/728

This allows navigating the timeline by entering a string in the find input box in the format of (timestamp)@(pid).(tid)x(scaleX). A line will be drawn at that ts, the pid and tid will be brought into view, and zoom will be applied.

The find string can also be generated by double clicking the timeline while in mousemode=selection. A nav line will be drawn and the nav string will be generated in the find box to be copied by the user. Single clicking anywhere on the timeline dismisses the nav line and removes the string.

This will be useful when one would want to refer to a specific point on the timeline in - for example - an email thread.

jackjyang included the following code: https://github.com/google/trace-viewer/pull/728/commits

catapult-bot commented 9 years ago

Comment by jackjyang Wednesday Feb 25, 2015 at 16:36 GMT

I've finally updated this - please take a look @natduca @dj2. I honestly think it's a little messy how I'm implementing this right now but I'm out of ideas on how to clean it up. Suggestions are always welcome!

catapult-bot commented 9 years ago

Comment by jackjyang Friday Feb 27, 2015 at 20:31 GMT

I've elected to use Number() instead of parseInt() since it handles conversion of decimal numbers as well. But I addressed all other comments. Thanks!

catapult-bot commented 9 years ago

Comment by dj2 Saturday Feb 28, 2015 at 00:41 GMT

This seems good to me as a first pass. @natduca do you have any issues?

catapult-bot commented 9 years ago

Comment by natduca Thursday Mar 05, 2015 at 05:55 GMT

Pretty swamped so lets land when you're happy Dan :) Sorry I held this up. :/

catapult-bot commented 9 years ago

Comment by jackjyang Thursday Mar 05, 2015 at 17:25 GMT

@natduca's earlier comment that was lost when I pushed:

actually this bit is a bit hacky... lets maybe do the pieces of this patch that are clear [the location etc glue] and make a new button tool in the mouse mode selector for making an annotation.... as a separate patch

So I've updated this cl with only the parts responsible for handling typing in find box, parsing, bringing into view and drawing a line.

On second thought I think it's worth having more discussion on how this second part will work. That is, the part to generate the find string so that it's easily copyable and sharable. I think the whole thing will still be a little 'hacky' without a generic way to store and navigate to the current state of the timeline. Nat I noticed you were looking into TraceViewer UI states a while ago -- I think this is something that will significantly overlap with this feature. I just fear that this code might need to be redone once we have actual UI states because even if we create a new mouse mode for this find string generation part the underlying functionality might be duplicated here.

catapult-bot commented 9 years ago

Comment by jackjyang Tuesday Mar 10, 2015 at 18:11 GMT

Hi @natduca, thanks for the review! I've updated this PR with a new fix-up commit because I didn't want to lose your comments. Please take a look. After you lgtm if you want I can squash everything into a single commit before merging.

Also since I removed my commit about drawing the line, Github lost our comments on that. I'm copying that here, mainly for my reference when adding annotations:

Eek that we're duplicating this code twice here. Gotta find a way to avoid that. We should be finding an abstraction here that can capture your use case but not duplicate code. Suppose we make an annotation class. This is a virtual base class that has draw methods just like tracks do. So its base class is constructed with a viewport argument. And, suppose we have Viewport have a list of annotations. E.g. viewport.addAnnotation, viewport.removeAnnotation. When you add/remove annotations, you call invalidate() like the tracks do, in order to cause a redraw. WHich is awesome because suppose that the drawing container iterates through the viewport's active annotations every time it draws. Now you can make a GlobalVerticalXMarker annotation that contains your drawing code that you construct from a single timestamp. Now you can have the find controller construct a marker, add it to the viewport. The only thing that remains is asking the timeline_view to focus a location. So then al you need on the timeline is a scrollLocationIntoView(location, opt_dontAnimate). Make sense?

And what I replied:

Yup, thanks, that makes sense. I'll put off this part of this feature (ie. the marker drawing at timestamp) until we have some sort of annotation drawing capability. In the meantime I'll update this pull request just to commit the first part of the change once I address all your other comments.

catapult-bot commented 9 years ago

Comment by natduca Wednesday Mar 11, 2015 at 05:14 GMT

looking solid. i think we need to squash these two commits down and move to retiveld. since its only two commits now i think its okay to have them in one. reitveld does comment preservation way better, anyway.

catapult-bot commented 9 years ago

Comment by jackjyang Wednesday Mar 11, 2015 at 17:08 GMT

Thanks Nat! I've fixed all the issues you've pointed out and uploaded the latest version on Rietveld: https://codereview.appspot.com/210660043/