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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Apr 4, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
08867b7
Add tooltip for displaying flag and comment in hexdump (#1471)
dhavalpur0hit Mar 28, 2020
5fd4aaf
Add review changes
dhavalpur0hit Apr 1, 2020
acabc81
Create narrow function for comment and flag
dhavalpur0hit Apr 1, 2020
f25106d
Document listFlagsAsStringAt function
dhavalpur0hit Apr 1, 2020
e872187
Fix function name to make it semantic
dhavalpur0hit Apr 2, 2020
f433ad3
Use CORE_LOCK() in Cutter.cpp
ITAYC0HEN Apr 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 47 additions & 1 deletion src/widgets/HexWidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include <QJsonObject>
#include <QJsonArray>
#include <QRegularExpression>
#include <QToolTip>

static constexpr uint64_t MAX_COPY_SIZE = 128 * 1024 * 1024;
static constexpr int MAX_LINE_WIDTH_PRESET = 32;
Expand Down Expand Up @@ -467,6 +468,15 @@ void HexWidget::mouseMoveEvent(QMouseEvent *event)
QPoint pos = event->pos();
pos.rx() += horizontalScrollBar()->value();

auto addr = currentAreaPosToAddr(pos, true);
Copy link
Member

Choose a reason for hiding this comment

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

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"

Copy link
Member

@karliss karliss Mar 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Also, what about using mouseHoverEvent instead of mouseMoveEvent?

Copy link
Member

Choose a reason for hiding this comment

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

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


QString ret = getFlagAndComment(addr.address);
karliss marked this conversation as resolved.
Show resolved Hide resolved
if (!ret.isEmpty() && itemArea.contains(pos)) {
QToolTip::showText(event->globalPos(), ret, this);
} else {
QToolTip::hideText();
}

if (!updatingSelection) {
if (itemArea.contains(pos) || asciiArea.contains(pos))
setCursor(Qt::IBeamCursor);
Expand All @@ -480,7 +490,6 @@ void HexWidget::mouseMoveEvent(QMouseEvent *event)
pos.setX(area.left());
else if (pos.x() > area.right())
pos.setX(area.right());
auto addr = currentAreaPosToAddr(pos, true);
setCursorAddr(addr, true);

/* Stop blinking */
Expand Down Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

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

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:
image

Copy link
Member

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

hexdump-new

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?

painter.drawRect(itemRect);
}
if (selection.contains(itemAddr) && !cursorOnAscii) {
itemColor = palette().highlightedText().color();
}
Expand Down Expand Up @@ -1454,6 +1468,38 @@ QChar HexWidget::renderAscii(int offset, QColor *color)
return QChar(byte);
}

QString HexWidget::getFlagAndComment(uint64_t address)
ITAYC0HEN marked this conversation as resolved.
Show resolved Hide resolved
karliss marked this conversation as resolved.
Show resolved Hide resolved
karliss marked this conversation as resolved.
Show resolved Hide resolved
{
QString ret = "";
QString flagName = "";

RCore *core = Core()->core();
RFlagItem *f = r_flag_get_i (core->flags, address);
karliss marked this conversation as resolved.
Show resolved Hide resolved

if (f) {
// Check if Realname is enabled. If yes, show it instead of the full flag-name.
if (Config()->getConfigBool("asm.flags.real") && f->realname) {
flagName = f->realname;
} else {
flagName = f->name;
}

ret = "Flag: " + flagName.trimmed();
}

QString comment = Core()->cmd("CC." + RAddressString(address));
karliss marked this conversation as resolved.
Show resolved Hide resolved

if (!(comment.isNull() || comment.isEmpty())) {
karliss marked this conversation as resolved.
Show resolved Hide resolved
if (ret.isEmpty()) {
ret = "Comment: " + comment.trimmed();
} else {
ret += "\nComment: " + comment.trimmed();
}
karliss marked this conversation as resolved.
Show resolved Hide resolved
}

return ret;
}

void HexWidget::fetchData()
{
data.swap(oldData);
Expand Down
1 change: 1 addition & 0 deletions src/widgets/HexWidget.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,7 @@ private slots:
QVariant readItem(int offset, QColor *color = nullptr);
QString renderItem(int offset, QColor *color = nullptr);
QChar renderAscii(int offset, QColor *color = nullptr);
QString getFlagAndComment(uint64_t address);
/**
* @brief Get the location on which operations such as Writing should apply.
* @return Start of selection if multiple bytes are selected. Otherwise, the curren seek of the widget.
Expand Down