-
Notifications
You must be signed in to change notification settings - Fork 201
8373: Fix tooltip on Flamegraph not showing after switching to Stack Trace view #635
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
|
👋 Welcome back RoiSoleil! A progress list of the required criteria for merging this PR into |
|
@aptmac Could you open a ticket ? |
|
@RoiSoleil This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 4 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@bric3, @aptmac) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
Ticket opened. |
Thx. |
|
At first glance the changes looks good to me! |
| tooltip.setPopupDelay(500); | ||
| tooltip.setShift(new Point(10, 5)); | ||
|
|
||
| embeddingComposite.addListener(SWT.MouseExit, event -> Display.getDefault().timerExec(300, tooltip::hide)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: I wonder if this listener is still needed, in case the mouse event is not received on the swing component. E.g. in weird situations like : Alt Tab (on macOs) to change app but in the end selecting back to JMC. Not sure it's a real scenario, but i could imagine the Swing component not properly receiving mouse events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the listener i still have the bug.
Yes just click on the stack trace on Windows (didn't try on linux or macos). |
|
@bric3 it's ok for you ? |
|
@RoiSoleil I tested on macos this morning. The chat get looks good. |
Thx |
aptmac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I noticed that not all of the usual checks are happening on the PR. IIRC these were enabled in the actions tab of the user's fork, @RoiSoleil could you see if there's anything for you to enabled at: https://github.com/RoiSoleil/jmc/actions ?
|
On https://github.com/RoiSoleil/jmc/actions : Workflows aren’t being run on this forked repository |
|
/integrate |
|
@RoiSoleil |
|
/sponsor |
|
@RoiSoleil Only Committers are allowed to sponsor changes. |
|
@aptmac could you sponsor ? |
Ah, would you be able to try and enable the actions? If the enable option isn't on that page, it may be here: https://github.com/RoiSoleil/jmc/settings/actions I'm not sure if when enabled Skara will kick off the other actions for this PR, but it would be nice to make sure you're set up for any PRs in the future. |
I did it and run validate.yml |
Great, thanks! I see they are running on the PR, I'll sponsor once the runs complete |
|
/sponsor |
|
Going to push as commit 6a048a8.
Your commit was automatically rebased without conflicts. |
|
@aptmac @RoiSoleil Pushed as commit 6a048a8. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Fix tooltip on Flamegraph not showing after switching to Stack Trace view
The bug : Open a JFR file, open the Flamegraph view, see the tooltip on Flamegraph working, activate the Stack Trace view, get back to Flamegraph ==> the tooltip is not showing anymore on Flamegraph
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jmc.git pull/635/head:pull/635$ git checkout pull/635Update a local copy of the PR:
$ git checkout pull/635$ git pull https://git.openjdk.org/jmc.git pull/635/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 635View PR using the GUI difftool:
$ git pr show -t 635Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jmc/pull/635.diff
Using Webrev
Link to Webrev Comment