-
Notifications
You must be signed in to change notification settings - Fork 3
Add C Source Line View #62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
941c35f
to
e149356
Compare
This is great! The instruction block highlighting is cool. Thank you for picking up this work, @jordalgo
And "go to line" is likely useless, and start/end buttons probably too. I added those in the beginning because I needed some navigation. As for line numbers column, I am not sure. It does indeed take space. For example, if you look at a trace of a loop, the instructions and PC repeat themselves, and it may be hard to disambiguate "where" are you in the log without the line numbers. I think the best way to resolve these problems is to give users control: allow them to hide things, resize and move around. But having a gazillion settings is also not great... unless we cache them in browser local storage?.. Just thinking out loud here.
One thing we certainly want to add. When showing dependencies, we highlight upstream instructions with grey background ( Also we need to figure out what to do with unknown source lines... Maybe Last but not least: we should allow users to switch back to inline view too. |
We can get rid of start and end though I can see where they might have some utility. I don't feel strongly.
Hmm, it's a good point. I'm still not sure if the line numbers really help though after all we don't collapse repeated instructions - they're all still there in order in the log. The log line numbers don't really mean anything outside of the log anyway as they don't translate to actual source code line numbers. And because we don't have headers (yet) for the line numbers column and the PC numbers column, having them next to each other is confusing (as we've experienced personally). Maybe it's enough to just keep the actual line number in the state panel and next to it have the C Source Line, the PC, and the Frame. We can also just remove it and see if our handful of users complain 😄
Exactly, I think we need to work on the common/intuitive case first and then start adding ways to customize as we go.
Ack. Will take a look at doing that.
Yeah I went back and forth on this as well. My first instinct was to just remove them but I didn't want to remove the ending error message from the verifier which I think is important (but maybe we can just tag that to keep it)
Ugh, I was afraid you'd say that lol. But ok, I don't disagree. Let me tinker. |
e149356
to
87aed8d
Compare
@theihor I still need to update the tests but I updated some of the behavior:
|
1ed20f7
to
11610a1
Compare
@theihor Added some examples of snapshot testing. I think it's just a good smoke test but we shouldn't rely on it solely and still do manual DOM verification where it makes sense. |
This adds a collapsible C source line view on the right, which contains all C files from the log and their C lines. This view is in sync with the main log lines view; meaning if a user clicks on a line in one, we'll hightlight the associated lines in the other. This also removes C lines from the main log view by default but a checkbox was added so users can add them back in as desired. Additional changes: - remove line numbers from the main log view - remove Go To Line input box - Add C Line to State Panel
11610a1
to
3e2d41c
Compare
@jordalgo I played with this, looks good overall. I'll send a small follow up PR to not include C lines without content into the In terms of UI, I think this is a great starting point. Merging now. |
@theihor Woot! Thanks for the review. |
Some notable things to call out: