Skip to content

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.

@bhufmann
Copy link
Contributor

The 2025-03 release is coming soon and it would good to include it. It needs to be merged before RC1 Feb. 26 to make the release. We have to 2 options: Merge this PR and add tests in a separate PR (even after RC1 or release). Or, update this PR with the unit tests before RC1. @arfio WDYT?

@bhufmann
Copy link
Contributor

Let's merge it for TC 11.0. You can add the tests in a separated PR.

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.

LGTM. Tested manually. Thanks.

@bhufmann bhufmann merged commit 7cd8fc5 into eclipse-tracecompass:master Apr 22, 2025
4 checks passed
@arfio arfio deleted the flamegraph-wasd branch October 22, 2025 20:08
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