Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dontgetfoundout
Copy link

@dontgetfoundout dontgetfoundout commented Dec 7, 2023

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 by CANFrameModel::data() and displayed using the default QStyledItemDelegate. In the current version of the changes, the CANFrame is returned as Data instead and the parsing and display logic has been moved to CanDataItemDelegate. Most of the structure of the code was maintained, but now it is orchestrating QPainter 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 a QVariant for the delegate to display rather than this design which returns QVariant::fromValue(CANFrame) and moves all of the parsing and display logic to the delegate
  • Some other data type for the changed bytes bit field
  • This implementation uses CANFrameModel to read settings in CanDataItemDelegate but this coupling could be reduced with another representation
  • etc.

Previously 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

SavvyCAN-OverwriteDiff
Example of the UI receiving Fuzzed frames using "Sweep" Bit Scanning

Validated:

  • <=8 byte CAN frames generated from Fuzzing window display properly
    • Hex
    • Decimal
  • Bytes per line of 4 splits 8 bytes over 2 lines

Not tested:

  • Error frame messages
  • DBC mapping text
    • These should probably work, but I couldn't find good example data. Let me know if there is a good example set for validation.
  • Larger frames
    • Probably needs some updates maybe works after commit "Increase size of changed bytes bit field" still needs testing

Notes:

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).

@dontgetfoundout
Copy link
Author

Updated to add a setting to enable/disable this behavior:
image

image

Copy link

@edge90 edge90 left a 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());
Copy link

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?

Comment on lines +22 to +25
bool overwriteDups = model->getOverwriteMode();
bool markChangedBytes = model->getMarkChangedBytes();
int bytesPerLine = model->getBytesPerLine();
bool useHexMode = model->getHexMode();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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();

Comment on lines +32 to +33
auto pen = painter->pen();
const auto defaultColor = pen.color();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
auto pen = painter->pen();
const auto defaultColor = pen.color();
const auto defaultColor = painter->pen().color();

Comment on lines +39 to +40
int dataLen = frame.payload().count();
if (dataLen < 0) dataLen = 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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:
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants