Add config for Go - Githubissues

dvyukov commented 9 years ago

We need to add a separate config for Go to (1) do small UI tweaks and (2) protect from undesirable future changes to config lean which we currently use.

This is roughly what we need:

$ cat trace_viewer/extras/go_config.html

<!DOCTYPE html>
<link rel="import" href="/extras/importer/trace_event_importer.html">
<script>
'use strict';
tv.c.TimelineView.showFlowEvents = true;
tv.c.TimelineView.showFlowEventsCheckbox.style.visibility = "hidden";
tv.c.TimelineView.highlightVSyncCheckbox.style.visibility = "hidden";
</script>

Unfortunately it does not work as is, and I have no idea how to express it correctly.

dvyukov commented 9 years ago

@egonelbre This is another issue for Go trace viewer.

natduca commented 9 years ago

are you using trace2html right now? or vulcanize?

dvyukov commented 9 years ago

./vulcanize_trace_viewer --config=lean

natduca commented 9 years ago

That is fine, but we have an established design pattern for embedders customizing their trace viewer: you make the things you want to customize into public get/set members on the TraceViewer object, and then having your code set that directly when you construct the TraceViewer.

You definitely don't want things outside the TimelineView reaching inside its class and configuring it internally ... thats like accessing private fields on a class, a big no no.

egonelbre commented 9 years ago

I was going over implementing this (https://codereview.appspot.com/258850043/ adding the option).

Anyways, I noticed that currently only full_config.html contains:

<!-- Core UI configs -->
<link rel="import" href="/model/model.html">
<link rel="import" href="/ui/base/ui.html">
<link rel="import" href="/ui/timeline_view.html">
...

Shouldn't the Core UI be part of lean_config.html?

That way Go could use the lean_config.html. Alternatively I could add "go_config.html" that does all the necessary imports or should the very-project-specific configs live externally from trace-viewer?

With regards to https://github.com/google/trace-viewer/wiki/Embedding#embedding-the-easy-way info seems out of date. Maybe a link to https://github.com/google/trace-viewer/blob/master/tracing/bin/index.html and instructions to run vulcanize would be better. That way the documentation won't get out of sync.

dvyukov commented 9 years ago

That way Go could use the lean_config.html

Go uses lean config, it somehow works.

egonelbre commented 9 years ago

@dvyukov there have been changes to the project and the lean_config isn't sufficient any more.

dvyukov commented 9 years ago

doh! that's bad

natduca commented 9 years ago

Lets fix lean config so it makes sense for go.

As far as chrome config including the things noted above, I think its a bug, I filed #1111 for that. Such a pleasing bug number.

About customizing the options dropdown, I mentioned on the codereview, but I think it'd be pretty cool if the dropdown hid itself when the model doesn't have those items in it. The reason go does't want the otpions is because go traces never contain flows or vsync, basically... so lets focus on that approach. That way if we add an option down the road that is go-relevant that UI affordance is still available to us. Versus, if we solve the go issue now by hiding the options dropdown, it basically makes the dropdown affordance unavailble to us for future concepts.

egonelbre commented 9 years ago

@dvyukov I forgot to ask what were the reasoning for hiding the options? @natduca didn't like that being included in https://codereview.appspot.com/258850043/. Also the drop-down looks better than it did before so maybe hiding them is actually not necessary?

2015-07-21 17_35_17-simple embedded viewer

dvyukov commented 9 years ago

@egonelbre The less options and visual clutter the better. If we enable "Flow events" by default, there is no reason for a user to disable it, and so no reason to show it. That was my reasoning.

egonelbre commented 9 years ago

@natduca btw. Go code does contain flows, otherwise it @dvyukov wouldn't use tv.c.TimelineView.showFlowEvents = true;.

natduca commented 9 years ago

I'm not really into skinning the ui for go in that way. We can hide the stuff that doesn't make sense for go like Vsync but I'd really prefer we focus our efforts on making the ui work well for all users across the board. Sorry! On Tue, Jul 21, 2015 at 11:50 PM Egon Elbre notifications@github.com wrote:

@natduca https://github.com/natduca btw. Go code does contain flows, otherwise it @dvyukov https://github.com/dvyukov wouldn't use tv.c.TimelineView.showFlowEvents = true;.

— Reply to this email directly or view it on GitHub https://github.com/google/trace-viewer/issues/906#issuecomment-123580699 .

dvyukov commented 9 years ago

@natduca what would be a solution in this case?

If you proposed to not show the checkbox when an input does not contain any arrows, I wonder why arrows are not shown by default when an input does contain arrows?

When I tried the viewer the first time, arrows were shown by default. Then they disappeared by default. Then the checkboxes were combined into a separate windows. Now I need to do 2 clicks whenever I open a trace. Were these changes intentional? What was the intent? Can we revert the behavior to showing arrows by default?

dj2 commented 9 years ago

Arrows should be shown by default but the viewer remembers your selection. So, if you turned them off last time they'll be off this time.

You shouldn't have to turn flow arrows on for each trace unless you're explicitly turning them off. If they turn off when you had them on then that's a bug we should fix.

natduca commented 9 years ago

[@dj2 i think the persistence may be broken from file:/// urls, maybe?]

dvyukov commented 9 years ago

@dj2 Arrows are always off when I open a trace. We don't use file:///, urls look like: http://127.0.0.1:[random_port]/trace

dj2 commented 9 years ago

Interesting, can you open a new bug about the issue and attach some info on how to repro? (You're using a vulcanized HTML trace, right? Can you either attach it or send it to me?

egonelbre commented 9 years ago

https://github.com/google/trace-viewer/blob/master/tracing/tracing/ui/timeline_view.html#L169

Here the value is set to false. Go starts trace-viewer on a random port, which would mean that the origin of the document can be different each time, which means that local storage would empty.

One easy fix would be to change the initial value of showFlowEvents to true. Not sure whether you like it?

As for not opening it on a random port:

With go tool trace it's possible to open several instances of the trace-viewer from different places. The server serving trace-viewer doesn't only show trace-viewer, but has other things as well.

Few options here:

Start assigning ports in order, this would work mostly, but the annoying part is that it would work "mostly". If the port is unavailable then you still get the same problem - sometimes things are missing.

Create a deamon and multiplex into a single server to manage all the instances - this is probably more effort than it's worth, it would be much less trouble to directly poke inside trace-viewer and verify that it works on each trace-viewer update.

Initialize local storage with the options - the options should be stable; but it feels wrong to do this. Although, would it be possible to replace the local storage with a "custom storage" that would contain the correct options?

dvyukov commented 9 years ago

One easy fix would be to change the initial value of showFlowEvents to true. Not sure whether you like it?

If there are flow events in the trace, then user wants to see them (why would a random piece of information be omitted? why other pieces of information are not omitted?) And if there are no flow events, then it does not matter whether we show them or not, so showing works as well.

dvyukov commented 9 years ago

@dj2 Do you still need a repro? I believe it boils down to just using a random port every time.

dj2 commented 9 years ago

Ah, I see. Yea, that's not going to work in that case as localstorage would be per port I believe.

The reason we turn off flow events in the UI is because chrome can generate a lot of them. This makes reading the trace a lot more difficult as the arrow lines obscure what's behind them. So, we made the arrows toggleable.

I don't see any reason why we can't change the default to enabled if it's currently set to disabled.

I don't see how we'd change localStorage to be custom storage? This has to work inside Chrome so we don't have access to the filesystem or anything like that.

egonelbre commented 9 years ago

@dj2 I meant to mock the current settings storage with tr.b.Settings.setAlternativeStorageInstance that would provide other defaults. But, I would rather prefer to set the default to be enabled.

natduca commented 9 years ago

I think in the medium run we should figure out how to delegate our settings storage out to the go server instance, which could internally map it to a dotfile in ~ for instance.

The trace viewer has a deep assumption that users can have ui preferences. As long as that doesn't work for our customers, it basically removes that avenue of ui design from consideration for us.

Put another way, We should try to avoid boxing ourselves in, basically. We might be able to temporarily a avoid an issue my changing the default but this lack-of-persisted-storage will bite is in the end. On Thu, Jul 23, 2015 at 12:04 PM Egon Elbre notifications@github.com wrote:

@dj2 https://github.com/dj2 I meant to mock the current settings storage with tr.b.Settings.setAlternativeStorageInstance that would provide other defaults. But, I would rather prefer to set the default to be enabled.

— Reply to this email directly or view it on GitHub https://github.com/google/trace-viewer/issues/906#issuecomment-124212906 .

dvyukov commented 9 years ago

@natduca For Go I would prefer if we just provide sensible defaults, and show no knobs if that is possible at all. For now there are two knobs: Flow events -- that must be enabled all the time for Go; and Show VSync -- that makes no sense for Go and must be always disabled. It does not conflict with addition of more settings to trace-viewer, we can just add more defaults.

natduca commented 9 years ago

@dvyukov you have made your preferences loud and clear. But as is often the case with you, we're not in a position where we can do what you want.

The only option on the table is: 1) us hiding vsync when there are no vsync events on the model 2) changing the polarity on the flow event checkbox so that its on by default.

That's it. We are not going to consider hiding the options dropdown, nor are we going to commit to these being the only two options to the UI in the future. For instance, I think we may resolve another one of your requests by adding a ui element to pick your preferred timestamp precision. And we may add more. Options are an important escape hatch for our team: to try out cool new ideas, or add things that don't always work but are super valuable when they do work. I do not want to compromise on that agility, because we have a lot of stuff going on right now, and so agility is super important.

@egonelbre has shown great agility in finding good middle ground between your high level goals, and what this team can realistically support. I believe he understands what we can and cannot do for now, and I would encourage you to give him leeway to translate your vision into a middle ground "achievable reality." That is probably something like the checkbox is there, but its better than now, and its persistent.

If you can't find it in yourself to accomodate middle ground, compromise oriented thinking, maybe you're using the wrong viewer component...

egonelbre commented 9 years ago

@dvyukov I'm just going to add this; I know it's annoying that we cannot modify trace-viewer to be perfect for Go, but the main users for trace-viewer are and always will be Chrome and Android so anything that gets added/changed/removed will have to take them into major consideration. A general tool cannot be perfect for all use-cases -- otherwise the code either will become very hard to maintain or it will be less-perfect for some other case. I.e. if Chrome and Android can benefit from the changes then there's really no problem for adding, e.g. both of them could benefit from the nanoseconds and stack-frames addition. Of course hiding options would be only for Go, hence it is harder to justify this patch for long-term maintainability reasons, irrespective how simple the change itself is... and, yes, I would like to see that change as well.

@natduca While writing this, I had another thought, what if we simply made the button smaller. Something similar how the chrome panel looks... quick replacement mock-up below, obv needs better icon than I used. I don't think spelling out "View Options" is necessary - most of the users are programmers, who are used to the gear icon meaning configurations. This would make the panel less cluttery. Combining it with hover can keep it discoverable, without having to worry about unintended consequences while clicking it (with new users).

2015-07-24 13_06_37-simple embedded viewer

natduca commented 9 years ago

We've had some issues with the discoverability of the ?-mark button which is why we have the bigger text for the options button. But, seems like we could give it a try... Wanna ping the mock to the trace-viewer@chromium.org thread and see if people like the idea?

I think the general design of the top controls is funky. It's very organic: we just added stuff, not designed it.

In the medium term I have the hope that we could move the find controls elsewhere and have a bit more normative ui. Dunno what that'd be precisely but for instance we have talked in the past about moving the mouse mode selector to the toolbar, having a settings dialog that includes keyboard help tab and instead of having a find control directly visible making it be a control that appears below the timeline, like happens in sublime. On Fri, Jul 24, 2015 at 3:14 AM Egon Elbre notifications@github.com wrote:

@dvyukov https://github.com/dvyukov I'm just going to add this; I know it's annoying that we cannot modify trace-viewer to be perfect for Go, but the main users for trace-viewer are and always will be Chrome and Android so anything that gets added/changed/removed will have to take them into major consideration. A general tool cannot be perfect for all use-cases -- otherwise the code either will become very hard to maintain or it will be less-perfect for some other case. I.e. if Chrome and Android can benefit from the changes then there's really no problem for adding, e.g. both of them could benefit from the nanoseconds and stack-frames addition. Of course hiding options would be only for Go, hence it is harder to justify this patch for long-term maintainability reasons, irrespective how simple the change itself is... and, yes, I would like to see that change as well.

@natduca https://github.com/natduca While writing this, I had another thought, what if we simply made the button smaller. Something similar how the chrome panel looks... quick replacement mock-up below, obv needs better icon than I used. I don't think spelling out "View Options" is necessary - most of the users are programmers, who are used to the gear icon meaning configurations. This would make the panel less cluttery. Combining it with hover can keep it discoverable, without having to worry about unintended consequences while clicking it (with new users).

[image: 2015-07-24 13_06_37-simple embedded viewer] https://cloud.githubusercontent.com/assets/192964/8872358/e35e47ae-3204-11e5-9fd9-285de3201c32.png

— Reply to this email directly or view it on GitHub https://github.com/google/trace-viewer/issues/906#issuecomment-124462948 .

egonelbre commented 9 years ago

.. Wanna ping the mock to the trace-viewer@chromium.org thread and see if people like the idea?

Yup I'll post it there as well, I also made a proper one from scratch trying to make the whole UI more consistent. https://dl.dropboxusercontent.com/u/4300994/Go/trace-viewer/index.html

For the design discussion we can keep things here https://groups.google.com/a/chromium.org/forum/#!topic/tracing/RctRQs_SHBg

natduca commented 9 years ago

!!!!!!!!!!! Wow!

You rock. On Sun, Jul 26, 2015 at 3:41 AM Egon Elbre notifications@github.com wrote:

.. Wanna ping the mock to the trace-viewer@chromium.org thread and see if people like the idea?

Yup I'll post it there as well, I also made a proper one from scratch trying to make the whole UI more consistent. https://dl.dropboxusercontent.com/u/4300994/Go/trace-viewer/index.html

For the design discussion we can keep things here https://groups.google.com/a/chromium.org/forum/#!topic/tracing/RctRQs_SHBg

— Reply to this email directly or view it on GitHub https://github.com/google/trace-viewer/issues/906#issuecomment-124968690 .

kphanee commented 9 years ago

@egonelbre : If possible, can you please post these mockups images on Google drive also? dropbox is not accessible from my work location. :(

egonelbre commented 9 years ago

@kphanee sure no problem https://drive.google.com/file/d/0BzlWdYwt6L4RMmhaVWJjZXZBNU0/view

egonelbre commented 9 years ago

I moved the complete UI styling to a separate issue, so it would be clearer.

catapult-bot commented 9 years ago

Migrated to https://github.com/catapult-project/catapult/issues/906

google / trace-viewer

Add config for Go #906