jvm-profiling-tools / honest-profiler

A sampling JVM profiler without the safepoint sample bias
https://github.com/RichardWarburton/honest-profiler/wiki
MIT License
1.25k stars 146 forks source link

Allow closing of profile tabs #206

Open PhRX opened 7 years ago

PhRX commented 7 years ago

Enable close buttons on profile and profile diff tabs, and ensure that underlying resources are cleaned up when necessary.

PhRX commented 7 years ago

Feature implemented in https://github.com/PhRX/honest-profiler-fx-ui/commit/7bca26fb523c3283bc2ffbc6a3df92657532dd4e , will create PR when previous ones have been processed.

RichardWarburton commented 7 years ago

I don't understand why you can't create an independent PR. This has been discussed and explained by both @nitsanw and me.

I started reviewing the first PR and you just rejected the feedback and refused to write tests. I don't think its likely to get merged without any improvement or effort being made to address these legitimate concerns.

PhRX commented 7 years ago

Hi @RichardWarburton I understand why it might seem from the outside that I ignore the feedback, but that isn't actually true, I do take it to heart. Unfortunately, it hasn't had any visible effect yet.

There are 2 main factors/reasons :

Anyway, I will rollback everything and separate the changes out. After that I'll add tests.

PhRX commented 7 years ago

@RichardWarburton

I've now undone all changes in the various open PRs from me (based on http://www.claassen.net/geek/blog/2011/02/git-merge-strategytheirs.html). I recreated the changes in logical batches on separate branches, creating independent PRs for each. I hope this resolves the issue of PR independence to your satisfaction.

What cannot be helped is that the "undo" shows up as a separate new commit, so each of the new PRs contains all of the undone commits, followed by the undo commit, and the single commit containing the actual changes. I've added a comment to each PR to clarify this.

I also took care to minimize the diffs by getting rid of some formatting changes that were inadvertently introduced.

I'll see what I can do about adding tests as soon as feasible.

nitsanw commented 6 years ago

Is this still an issue?

PhRX commented 6 years ago

Yes, in the current version an opened profile cannot be closed.