-
Notifications
You must be signed in to change notification settings - Fork 293
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
Highlight changed bytes in Frames view when Overwrite mode is enabled #708
base: master
Are you sure you want to change the base?
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.
Looks usable to me :)
const QModelIndex &index) const | ||
{ | ||
const auto frame = index.data().value<CANFrame>(); | ||
const unsigned char *data = reinterpret_cast<const unsigned char *>(frame.payload().constData()); |
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.
Isn't it better to continue to use QByteArray?
bool overwriteDups = model->getOverwriteMode(); | ||
bool markChangedBytes = model->getMarkChangedBytes(); | ||
int bytesPerLine = model->getBytesPerLine(); | ||
bool useHexMode = model->getHexMode(); |
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.
bool overwriteDups = model->getOverwriteMode(); | |
bool markChangedBytes = model->getMarkChangedBytes(); | |
int bytesPerLine = model->getBytesPerLine(); | |
bool useHexMode = model->getHexMode(); | |
const bool overwriteDups = model->getOverwriteMode(); | |
const bool markChangedBytes = model->getMarkChangedBytes(); | |
const int bytesPerLine = model->getBytesPerLine(); | |
const bool useHexMode = model->getHexMode(); |
auto pen = painter->pen(); | ||
const auto defaultColor = pen.color(); |
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.
auto pen = painter->pen(); | |
const auto defaultColor = pen.color(); | |
const auto defaultColor = painter->pen().color(); |
int dataLen = frame.payload().count(); | ||
if (dataLen < 0) dataLen = 0; |
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.
int dataLen = frame.payload().count(); | |
if (dataLen < 0) dataLen = 0; | |
int dataLen = frame.payload().count(); | |
if (dataLen < 0) dataLen = 0; | |
const auto dataLen = std::max(frame.payload().count(), qsizetype{0}); |
@@ -184,6 +184,7 @@ class Utility | |||
return (double)timestamp / 1000.0; | |||
break; | |||
case TS_MICROS: | |||
default: |
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.
is this really intentional? It should move to be the last entry if that's the case.
Description:
This change introduces a
QItemDelegate
that is used to paint the Data column of the main window's Frames view. Before the change, the value of the Data column was calculated as a string byCANFrameModel::data()
and displayed using the defaultQStyledItemDelegate
. In the current version of the changes, theCANFrame
is returned as Data instead and the parsing and display logic has been moved toCanDataItemDelegate
. Most of the structure of the code was maintained, but now it is orchestratingQPainter
draw calls rather than building up the string representation of the bytes.There are a lot of ways this solution could be tweaked (improved?). For example:
CANFrameModel::data()
could parse the CANFrame into a streamlined data type with just the byte data, changed bytes, error message text, and some settings (like bits per line) and return that as aQVariant
for the delegate to display rather than this design which returnsQVariant::fromValue(CANFrame)
and moves all of the parsing and display logic to the delegateCANFrameModel
to read settings inCanDataItemDelegate
but this coupling could be reduced with another representationPreviously discussed as: #84 Color code changed bits in overwrite mode
While working on this, I found this already closed suggestion for the feature. I agree the sniffer window can be used to show this data, but I've come to like the visual feedback of having byte diffs in overwrite mode. Feel free to close this PR if you don't want to add this extra complexity to the project.
Screenshot
Example of the UI receiving Fuzzed frames using "Sweep" Bit Scanning
Validated:
Not tested:
Probably needs some updatesmaybe works after commit "Increase size of changed bytes bit field" still needs testingNotes:
It's been a little while since I've messed around with someone else's C++ codebase and I don't use it professionally atm. Please let me know if you have feedback (even if trivial).