Add tooltip for displaying flag and comment in hexdump (#1471) #2116

dhavalpur0hit · 2020-03-28T13:01:56Z

Your checklist for this pull request

I've read the guidelines for contributing to this repository
I made sure to follow the project's coding style
I've updated the documentation with the relevant information (if needed)

Detailed description

Functional Changes

Mark and show the available flags and comments in a tooltip in the hex dump.

Code Changes
HexWidget.cpp:

Created a new function getFlagAndComment(uint64_t address) to retrieve the flag and comment for an address.
- Fetched all the flags info for the address using r_flag_get_liststr (core->flags, address)
- Fetched the comments for the address using Core()->cmdRawAt("CC.", address)
Marked the address in the drawItemArea(QPainter &painter) function with available flag/comment.
Added the tooltip in mouseMoveEvent(QMouseEvent *event) to display flag/comment on hover.

Test plan (required)

Marked position of comment or flag using the borderColor with alpha 50% in the hex section
Both flag and comment are present:
Only the flag is present:
Only the Comment is present:

Closing issues
Closes #1471

dhavalpur0hit · 2020-03-28T13:03:51Z

Hi @karliss, please review if this conforms with the expected design behavior.

src/widgets/HexWidget.cpp

ITAYC0HEN · 2020-03-28T14:40:23Z

src/widgets/HexWidget.cpp

@@ -1010,6 +1019,11 @@ void HexWidget::drawItemArea(QPainter &painter)
        for (int j = 0; j < itemColumns; ++j) {
            for (int k = 0; k < itemGroupSize && itemAddr <= data->maxIndex(); ++k, itemAddr += itemByteLen) {
                itemString = renderItem(itemAddr - startAddress, &itemColor);
+
+                if (!getFlagAndComment(itemAddr).isEmpty()) {
+                    painter.setPen(QColor(255, 255, 0));


hardcoded color doesn't honor theme. Use the "borderColor" variable which equals to Config()->getColor("gui.border");. I suggest to try it with alpha 50% to reduce noise.

Also, the solution does not support padding, the border is too narrow and covers the font. Check the margin in the selection implementation.

Also, does not support flag-size. Some flags are too big though (like sections) and it would be crazy to have them shown. In 010 Editor flags are only shown when hovering. So you don't know that there is a flag there until hovering with the mouse. But then, it shows the mark very softly, with brackets, and I like this:

@ITAYC0HEN Your comment about size support seems somewhat contradicting. Do you want the size to be displayed only if it's small? I don't think it's worth implementing proper size handling specifically for this without making the generic mechanism described in https://github.com/radareorg/cutter/issues/1945#issuecomment-598440589

Hi, I have addressed other changes but I'm not quite clear about the way the flags should be displayed and also, about supporting flag-size. This is what it looks like now:

I have used r_flag_get_liststr(RFlag *f, ut64 off) which returns a comma-separated list of flags and also, uses an ellipsis for a flag. Would this be the right way to display them or, is something else expected here?

karliss · 2020-03-28T14:57:16Z

src/widgets/HexWidget.cpp

@@ -467,6 +468,15 @@ void HexWidget::mouseMoveEvent(QMouseEvent *event)
    QPoint pos = event->pos();
    pos.rx() += horizontalScrollBar()->value();

+    auto addr = currentAreaPosToAddr(pos, true);


For tooltip it's better to use mousePosToAddr(pos, false). mousePosToAddr because tooltip shouldn't be affect which part hex or text was last clicked. Try clicking on text side and then hover over flag or comment on the hex side to see the difference.

For middle=false see the documentation commnent.

* @param middle start next position from middle of symbol. Use middle=true for vertical cursror position between symbols, middle=false for insert mode cursor and getting symbol under cursor.

In this case you are interested in "symbol under cursor"

~~Check if mouse is in any of relevant areas would also be helpful.~~ you are already doing that. It should would be better to do that as early as possible before operating with address obtained using bad mouse position.

Also, what about using mouseHoverEvent instead of mouseMoveEvent?

I don't think mouseHover is appropriate in this case, but QEvent::ToolTip could be.

karliss

My overall opinion is that when the generic mechanism gets implemented this will have to be more or less rewritten. This PR still provides useful functionality and could be merged after dealing with the review comment but keeping in mind rewrite this isn't the right place for spending too much time on handling the tricky cases.

src/widgets/HexWidget.cpp

karliss · 2020-03-28T16:16:53Z

src/widgets/HexWidget.cpp

@@ -1010,6 +1019,11 @@ void HexWidget::drawItemArea(QPainter &painter)
        for (int j = 0; j < itemColumns; ++j) {
            for (int k = 0; k < itemGroupSize && itemAddr <= data->maxIndex(); ++k, itemAddr += itemByteLen) {
                itemString = renderItem(itemAddr - startAddress, &itemColor);
+
+                if (!getFlagAndComment(itemAddr).isEmpty()) {
+                    painter.setPen(QColor(255, 255, 0));


@ITAYC0HEN Your comment about size support seems somewhat contradicting. Do you want the size to be displayed only if it's small? I don't think it's worth implementing proper size handling specifically for this without making the generic mechanism described in https://github.com/radareorg/cutter/issues/1945#issuecomment-598440589

dhavalpur0hit · 2020-04-01T07:30:13Z

Hi, I have handled the review changes and also, updated the test plan. Please provide your feedback regarding the way the flags are displayed and in general, about the other changes too. Thanks.

ITAYC0HEN

Good work, almost done :))

src/widgets/HexWidget.cpp

ITAYC0HEN · 2020-04-01T10:03:57Z

src/widgets/HexWidget.cpp

+
+    QString metaData = getFlagAndComment(mouseAddr);
+    if (!metaData.isEmpty() && itemArea.contains(pos)) {
+        QToolTip::showText(event->globalPos(), metaData, this);


Please add a space after each comma. "item1,item2,item3" -> "item1, item2, item3"

src/widgets/HexWidget.cpp

dhavalpur0hit · 2020-04-01T23:13:54Z

Hi, I have made the remaining changes:).

ITAYC0HEN

Good work. Almost there :)

src/widgets/HexWidget.cpp

ITAYC0HEN · 2020-04-02T06:46:31Z

src/core/Cutter.cpp

@@ -775,6 +775,11 @@ void CutterCore::delComment(RVA addr)
    emit commentsChanged();
 }

+QString CutterCore::getCommentAt(RVA addr)


docstrings :) Try to not forget to document everything. This was a decision we made not so long ago, and this is why you see a lot of undocumented places in the code. Please pay attention to this :)

Sure, I'll keep that in mind. Yeah, looking at other places in the code, I wasn't sure about what all is supposed to be documented.

everything :) Every function, and every coding decision that might not be clear for the reader

dhavalpur0hit · 2020-04-02T07:25:24Z

I have added the changes:).

ITAYC0HEN

Thank you! Good work :)
In a simple PR, you got the opportunity to understand the code base of cutter, how it interacts with radare2 via commands. You learned about CmdRaw and CmdRawAt and why they're a good practice. You learned about the importance of documentation and the usage of dynamic variables instead of hard-coded (for border color)

Good work!

dhavalpur0hit · 2020-04-02T07:42:51Z

Yes, indeed. Thank you for guiding me through this:).

src/core/Cutter.cpp

Co-Authored-By: karliss <[email protected]>

Add tooltip for displaying flag and comment in hexdump (rizinorg#1471)

08867b7

ITAYC0HEN requested changes Mar 28, 2020

View reviewed changes

karliss requested changes Mar 28, 2020

View reviewed changes

Add review changes

5fd4aaf

ITAYC0HEN requested changes Apr 1, 2020

View reviewed changes

Create narrow function for comment and flag

acabc81

Document listFlagsAsStringAt function

f25106d

dhavalpur0hit force-pushed the hexdump-flag-comment branch from 6db631b to f25106d Compare April 1, 2020 23:26

ITAYC0HEN requested changes Apr 2, 2020

View reviewed changes

Fix function name to make it semantic

e872187

ITAYC0HEN approved these changes Apr 2, 2020

View reviewed changes

karliss requested changes Apr 2, 2020

View reviewed changes

src/core/Cutter.cpp Outdated Show resolved Hide resolved

Use CORE_LOCK() in Cutter.cpp

f433ad3

Co-Authored-By: karliss <[email protected]>

karliss approved these changes Apr 2, 2020

View reviewed changes

ITAYC0HEN merged commit 7110d73 into rizinorg:master Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tooltip for displaying flag and comment in hexdump (#1471) #2116

Add tooltip for displaying flag and comment in hexdump (#1471) #2116

dhavalpur0hit commented Mar 28, 2020 •

edited

Loading

dhavalpur0hit commented Mar 28, 2020

ITAYC0HEN Mar 28, 2020

karliss Mar 28, 2020

dhavalpur0hit Mar 29, 2020

karliss Mar 28, 2020

karliss Mar 28, 2020 •

edited

Loading

ITAYC0HEN Mar 28, 2020

karliss Mar 28, 2020

karliss left a comment

karliss Mar 28, 2020

dhavalpur0hit commented Apr 1, 2020

ITAYC0HEN left a comment

ITAYC0HEN Apr 1, 2020

dhavalpur0hit commented Apr 1, 2020

ITAYC0HEN left a comment

ITAYC0HEN Apr 2, 2020

dhavalpur0hit Apr 2, 2020

ITAYC0HEN Apr 2, 2020

dhavalpur0hit commented Apr 2, 2020

ITAYC0HEN left a comment

dhavalpur0hit commented Apr 2, 2020

Add tooltip for displaying flag and comment in hexdump (#1471) #2116

Add tooltip for displaying flag and comment in hexdump (#1471) #2116

Conversation

dhavalpur0hit commented Mar 28, 2020 • edited Loading

dhavalpur0hit commented Mar 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karliss Mar 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karliss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhavalpur0hit commented Apr 1, 2020

ITAYC0HEN left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhavalpur0hit commented Apr 1, 2020

ITAYC0HEN left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhavalpur0hit commented Apr 2, 2020

ITAYC0HEN left a comment

Choose a reason for hiding this comment

dhavalpur0hit commented Apr 2, 2020

dhavalpur0hit commented Mar 28, 2020 •

edited

Loading

karliss Mar 28, 2020 •

edited

Loading