Skip to content
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

profiling: add wasd controls to flamegraphview #202

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arfio
Copy link
Contributor

@arfio arfio commented Jan 7, 2025

What it does

Fix issue #201 by using the context service already used by the timegraphview and the xyview.

How to test

  1. Open a trace with a call stack analysis
  2. Open the flamegraph view
  3. Use the keys 'w', 'a', 's', 'd' to move in the view

Follow-ups

As the flamegraph view reuses a lot of the timegraph view code. Most of the code for this patch is copied from the timegraph implementation. Refactoring the timegraph view so that it is not coupled to a time based x-axis would fix that issue.

Review checklist

  • As an author, I have thoroughly tested my changes and carefully followed the instructions in this template

Copy link
Contributor

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to have some UI tests added to for this. There is already a SwtBot test for this view that could be used for that (see below). This could be done in a separate PR.

https://github.com/eclipse-tracecompass/org.eclipse.tracecompass/blob/master/analysis/org.eclipse.tracecompass.analysis.profiling.ui.swtbot.tests/src/org/eclipse/tracecompass/analysis/profiling/ui/swtbot/tests/flamegraph/FlameGraphTest2.java

@arfio
Copy link
Contributor Author

arfio commented Jan 14, 2025

I will be adding tests in a few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants