-
Notifications
You must be signed in to change notification settings - Fork 265
Add code for cost delegate #34
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
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.
please also reword the commit message:
Add cost delegate to visualize percentages graphically
This commit introduces the cost delegate from heaptrack
and adapts it for usage in hotspot.
Fixes: #23
src/mainwindow.cpp
Outdated
auto bottomUpCostModel = new BottomUpModel(this); | ||
setupTreeView(ui->bottomUpTreeView, ui->bottomUpSearch, bottomUpCostModel); | ||
ui->bottomUpTreeView->setItemDelegate(costDelegate); |
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.
uhm, don't we only want to set that for the cost columns? This sets it for all columns - is that correct? I don't think so.
src/models/costdelegate.cpp
Outdated
void CostDelegate::paint(QPainter* painter, const QStyleOptionViewItem& option, const QModelIndex& index) const | ||
{ | ||
// TODO: handle negative values | ||
const int64_t cost = index.data(AbstractTreeModel::SortRole).toULongLong(); |
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.
use auto, note how you get an unsigned value but store it in a signed int64_t
src/models/costdelegate.cpp
Outdated
return; | ||
} | ||
|
||
const int64_t totalCost = index.data(AbstractTreeModel::TotalCostRole).toULongLong(); |
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.
dito
src/models/costdelegate.cpp
Outdated
} | ||
|
||
const int64_t totalCost = index.data(AbstractTreeModel::TotalCostRole).toULongLong(); | ||
// top-down can miscalculate the peak cost |
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.
I think this might actually not be required anymore - could you check whether removing the std::min(1, ) wrapper? the abs and division is still fine though.
src/models/costdelegate.cpp
Outdated
|
||
#include "costdelegate.h" | ||
|
||
#include "treemodel.h" |
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.
because this delegate also needs to support the hashmodel (for caller/callee), we must not let it depend on the treemodel
instead, pass in the roles to query via the ctor, store them in member variables, and then use that below.
this also means you'll need two delegates in the mainwindow: one for all tree models, and one for all hashmodels.
This commit introduces the cost delegate from heaptrack and adapts it for usage in hotspot. Fixes: KDAB#23
Fixes made, comment updated, and I tested std:min. At least with the perf.data file I am testing with the results were identical for every result so I went ahead and removed the std::min wrapper |
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.
excellent, thank you
Bring in and adapt Heaptracks cost delegate for Hotspot
#23