Show marker count next to the marker name in the marker chart when a marker is hovered.#4892
Conversation
fqueze
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4892 +/- ##
==========================================
- Coverage 88.43% 88.42% -0.01%
==========================================
Files 304 303 -1
Lines 27192 27143 -49
Branches 7329 7335 +6
==========================================
- Hits 24047 24001 -46
+ Misses 2927 2924 -3
Partials 218 218 ☔ View full report in Codecov by Sentry. |
fqueze
force-pushed
the
show-marker-count
branch
from
794829b to
751a214
Compare
julienw
changed the title
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
This works well, but there's a shortcoming: when we have several rows for one marker name, and the user hovers a marker on a row that's not the first one, I think we'd still want to see the total.
Otherwise as a user it's not clear that the count seen when hovering the first line actually contains all the markers or just the ones in the first line.
That said I understand this is more work: we need to redraw the label for another line that the one that's been hovered.
I'd like to hear your thoughts about that before approving more.
I agree the experience in this case is not perfect. I didn't try to fix it exactly for the reason you mentioned, it would involve redrawing another line, and I wasn't sure I wanted to start doing that (or if I instead made it redraw the entire set of rows related to the hovered markers, it could be slow when we have many nested markers). I think hovering the marker name itself is also a case where displaying the marker count would make sense, but it can be left for a follow-up. I still think a better long term solution might be to show a tooltip when hovering the marker name, and use this new UI area to display a lot more information (eg. marker frequency, total duration, median duration, ...). |
|
But we don't need to redraw the full first line, only its label. I also like your suggestion of a tooltip on the marker name, although we'd still need something different in the active tab view where we hide the name on hover to show the possible underlying markers... |
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
I think this looks good but I believe it would be better to extract the algorithm for more clarity. Please tell me if you don't approve.
| if (drawMarkerCount) { | ||
| let count = markerTiming.length; | ||
| for ( | ||
| let row = rowIndex + 1; | ||
| row < markerTimingAndBuckets.length; | ||
| ++row | ||
| ) { | ||
| if ( | ||
| typeof markerTimingAndBuckets[row] === 'string' || | ||
| markerTimingAndBuckets[row].name !== name | ||
| ) { | ||
| break; | ||
| } | ||
| count += markerTimingAndBuckets[row].length; | ||
| } |
There was a problem hiding this comment.
Is it worth extracting the algorithm to compute the marker count to a separate method?
countMarkerInBucketStartingAtRow(rowIndex)
…marker is hovered.
fqueze
force-pushed
the
show-marker-count
branch
from
751a214 to
dd0c6c5
Compare
[@canova Nazım Can Altınova] Return null instead of throwing an error if it fails to get the resource name in active tab view (#4905) [@fqueze Florian Quèze] Improve the precision of the getFittedText implementation. (#4893) [@fqueze Florian Quèze] Show marker count next to the marker name in the marker chart when a marker is hovered. (#4892) [@mstange Markus Stange] Convert some code to use the non-inverted call node table even while we're showing the inverted call tree (Merge PR #4899) [@canova Nazım Can Altınova] Bring the max stacking depth limit back for marker timings (#4898) [@mstange Markus Stange] Various preparations in call tree code for fast inverted tree (Merge PR #4897) [@mstange Markus Stange] Move call node path/index conversion functions into CallNodeInfo (Merge PR #4896) [@mstange Markus Stange] Make mergeFunction fast (Merge PR #4895) [@gthb Gunnlaugur Thor Briem] feat: support from-url profiles in compare (#4875) And all the locale contributors: de: Michael Köhler el: Jim Spentzos en-GB: Ian Neal es-CL: ravmn fr: Théo Chevalier fy-NL: Fjoerfoks ia: Melo46 it: Francesco Lodolo [:flod] nl: Fjoerfoks, Mark Heijl pt-BR: Marcelo Ghelman ru: Valery Ledovskoy sv-SE: Andreas Pettersson uk: Artem Polivanchuk zh-CN: 你我皆凡人, Olvcpr423, Pontoon zh-TW: Pin-guang Chen
2 participants
…
┆Issue is synchronized with this Jira Task