Clarify Diff Snapshots Freed column is per class #8837 - #10002
Open
AzazelSensei wants to merge 1 commit into
Open
Clarify Diff Snapshots Freed column is per class #8837#10002AzazelSensei wants to merge 1 commit into
AzazelSensei wants to merge 1 commit into
Conversation
The Freed (and Allocated / Delta / Persisted) size columns already show that class only. Add header tooltips so it is not read as a running total of the table. Fixes flutter#8837
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds tooltips to the size columns (Allocated, Freed, Delta, Persisted) in the Memory Diff Snapshots table to clarify that the values represent individual rows rather than cumulative totals. The release notes have also been updated. The reviewer suggested extracting a repeated string literal in the new columnTooltip method into a local constant to adhere to the DRY principle.
Comment on lines
+187
to
+203
| static String columnTooltip(_DataPart dataPart) { | ||
| switch (dataPart) { | ||
| case _DataPart.created: | ||
| return 'Size of this class allocated between the two snapshots.\n' | ||
| 'This is the value for this row, not a cumulative total.'; | ||
| case _DataPart.deleted: | ||
| return 'Size of this class released between the two snapshots.\n' | ||
| 'This is the value for this row, not a cumulative total.'; | ||
| case _DataPart.delta: | ||
| return 'Net size change of this class between the two snapshots ' | ||
| '(allocated minus freed).\n' | ||
| 'This is the value for this row, not a cumulative total.'; | ||
| case _DataPart.persisted: | ||
| return 'Size of this class present in both snapshots.\n' | ||
| 'This is the value for this row, not a cumulative total.'; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
[CONCERN] The string literal '\nThis is the value for this row, not a cumulative total.' is repeated four times within the columnTooltip method. To adhere to the DRY principle and the repository style guide, this repeated sub-expression should be extracted into a local constant.
static String columnTooltip(_DataPart dataPart) {
const rowValueNote = '\\nThis is the value for this row, not a cumulative total.';
switch (dataPart) {
case _DataPart.created:
return 'Size of this class allocated between the two snapshots.$rowValueNote';
case _DataPart.deleted:
return 'Size of this class released between the two snapshots.$rowValueNote';
case _DataPart.delta:
return 'Net size change of this class between the two snapshots '
'(allocated minus freed).$rowValueNote';
case _DataPart.persisted:
return 'Size of this class present in both snapshots.$rowValueNote';
}
}References
- Extract repeated sub-expressions or string literals within the same method into local variables or constants to adhere to the DRY principle. (link)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Diff Snapshots size columns already report each class on its own row. Freed had no header tooltip, so 29.9MB looked like it might be a running total.
I added tooltips on Allocated, Freed, Delta, and Persisted that say the value is for that class row.
Fixes #8837
Pre-launch Checklist
General checklist
///).Issues checklist
contributions-welcome] or [good-first-issue] label.contributions-welcome] or [good-first-issue] label. I understand this means my PR might take longer to be reviewed.Tests checklist
The change is header tooltip copy. Existing table code already shows
titleTooltipon hover.AI-tooling checklist
Feature-change checklist
release-notes-not-requiredlabel or left a comment requesting the label be added.packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md.No layout change, only column header tooltips, so I did not attach screenshots.