-
Notifications
You must be signed in to change notification settings - Fork 129
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
Adjust height based on subtitle. #161
Conversation
Codecov Report
@@ Coverage Diff @@
## master #161 +/- ##
=========================================
- Coverage 88.53% 88.5% -0.03%
=========================================
Files 16 16
Lines 2216 2228 +12
=========================================
+ Hits 1962 1972 +10
- Misses 254 256 +2
Continue to review full report at Codecov.
|
Interesting.. Do you have a reference to the corresponding code in the flamegraph codebase? I just want to make sure that we do the same thing they do. |
I find the automated tools claim that coverage has gone down rather implausible, but if this is important I can find something else to test 😀 |
As I mentioned, the approach taken by the Perl version is wrong in one case. However, they do have some code in there: my $ypad1 = $fontsize * 3; # pad top, include title
my $ypad2 = $fontsize * 2 + 10; # pad bottom, include labels
my $ypad3 = $fontsize * 2; # pad top, include subtitle (optional)
...
my $imageheight = (($depthmax + 1) * $frameheight) + $ypad1 + $ypad2;
$imageheight += $ypad3 if $subtitletext ne ""; I think my approach is... mathematically equivalent but more correct if there's other code that relies on ypad1. |
Haha, no, you're all good. There's some variance in there that sometimes triggers weird false positives. Looks good to me! And thanks for the perl reference: the change looks sensible 👍 |
Oh, and thanks for the quick fix! <3 |
Released as |
Fixes #159.
To test, rerun the two commands in #159 and notice if subtitle is visible.