Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

Add more representations of unprintable bytes #380

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

Add more representations of unprintable bytes #380

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
52968d6
Add more representations of unprintable bytes
Nov 23, 2017
51561bb
Review fixup: minor issues (wip)
Dec 6, 2017
6a98497
Review fixup: enumized unprintablesMode
Dec 6, 2017
57a8a2f
Review fixup 3
Dec 13, 2017
f894426
Jenkins fixup
Dec 13, 2017
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
9 changes: 8 additions & 1 deletion include/ui/hexedit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <QMouseEvent>
#include <QStaticText>
#include <QStringList>
#include <QTextCodec>

#include "ui/createchunkdialog.h"
#include "ui/fileblobmodel.h"
Expand Down Expand Up @@ -71,6 +72,7 @@ class HexEdit : public QAbstractScrollArea {
void applyChanges();
void undo();
void discardChanges();
void setUnprintablesMode(QAction* action);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why it receives QAction* when it in fact uses QString from the inside? Signature of this function looks very unintuitive now, it should rather take the string argument.

Update:
You should use enum instead of a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is slot for QMenu signal triggered of type void(QAction*) . AFAIK these must have the same type.

Enum instead of string: I'd have to bind somehow action in menu with enum value. Is there any well seen method?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can connect to a lambda which translates the arguments to ones that make more sense. Regarding enum: you can bind each of these actions to a lambda which calls setUnprintablesMode with a proper argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Not sure if capturing this by value is ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's ok, because it's a pointer to an object which will live longer than the lambda.


protected:
void paintEvent(QPaintEvent* event) override;
Expand Down Expand Up @@ -177,6 +179,8 @@ class HexEdit : public QAbstractScrollArea {
QScopedPointer<util::encoders::TextEncoder> textEncoder_;
util::EditEngine edit_engine_;

QString unprintablesModeString;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a private field, so it should be named unprintables_mode_string_.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


void recalculateValues();
void initParseMenu();
void adjustBytesPerRowToWindowSize();
Expand All @@ -188,7 +192,10 @@ class HexEdit : public QAbstractScrollArea {
WindowArea pointToWindowArea(QPoint pos);
QString addressAsText(qint64 pos);
QString hexRepresentationFromByte(uint64_t byte_val);
static QString asciiRepresentationFromByte(uint64_t byte_val);

QString asciiRepresentationFromByte(uint64_t byte_val);
void updateAsciiCache();
void updateHexCache();

static QColor byteTextColorFromByteValue(uint64_t byte_val);
QColor byteBackroundColorFromPos(qint64 pos, bool modified);
Expand Down
2 changes: 2 additions & 0 deletions include/ui/hexeditwidget.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ class HexEditWidget : public View {
void createActions();
void createToolBars();
void initParsersMenu();
void initUnprintablesMenu();
void createSelectionInfo();

MainWindowWithDetachableDockWidgets* main_window_;
Expand Down Expand Up @@ -127,6 +128,7 @@ class HexEditWidget : public View {

QStringList parsers_ids_;
QMenu parsers_menu_;
QMenu unprintables_menu_;
QLabel* selection_label_;
};

Expand Down
4 changes: 4 additions & 0 deletions include/util/settings/hexedit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
*/
#pragma once

#include <QString>
Copy link
Contributor

Choose a reason for hiding this comment

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

Separate this includes with a blank line (see https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


namespace veles {
namespace util {
namespace settings {
Expand All @@ -25,6 +27,8 @@ int columnsNumber();
void setColumnsNumber(int number);
bool resizeColumnsToWindowWidth();
void setResizeColumnsToWindowWidth(bool on);
QString unprintablesModes();
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it should be unprintablesMode (without s).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, typo. Thanks for catching.

void setUnprintablesMode(QString mode);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be const QString& mode.

Copy link
Contributor Author

@prybicki prybicki Dec 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Rewritten.


} // namespace hexedit
} // namespace settings
Expand Down
67 changes: 53 additions & 14 deletions src/ui/hexedit.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,11 @@
#include "util/encoders/factory.h"
#include "util/misc.h"
#include "util/random.h"
#include "util/settings/hexedit.h"
#include "util/settings/theme.h"

#include <unistd.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

You can't use this header, it's non-standard (and as a result it doesn't compile on Windows, for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a debugging leftover, removed.


using veles::util::misc::array_size;

namespace veles {
Expand Down Expand Up @@ -142,16 +145,9 @@ HexEdit::HexEdit(FileBlobModel* dataModel, QItemSelectionModel* selectionModel,
recalculateValues();

// Initialize hex & ASCII text cache.
for (size_t i = 0; i < array_size(hex_text_cache_); i++) {
hex_text_cache_[i].setPerformanceHint(QStaticText::ModerateCaching);
hex_text_cache_[i].setText(hexRepresentationFromByte(i));
hex_text_cache_[i].setTextFormat(Qt::PlainText);
}
for (size_t i = 0; i < array_size(ascii_text_cache_); i++) {
ascii_text_cache_[i].setPerformanceHint(QStaticText::ModerateCaching);
ascii_text_cache_[i].setText(asciiRepresentationFromByte(i));
ascii_text_cache_[i].setTextFormat(Qt::PlainText);
}
unprintablesModeString = util::settings::hexedit::unprintablesModes();
updateAsciiCache();
updateHexCache();

connect(verticalScrollBar(), &QAbstractSlider::valueChanged, this,
&HexEdit::recalculateValues);
Expand Down Expand Up @@ -475,6 +471,21 @@ HexEdit::HexEdit(FileBlobModel* dataModel, QItemSelectionModel* selectionModel,
});
}

void HexEdit::updateAsciiCache() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please place this method in the same place as it was in the header file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for HexEdit::updateHexCache()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

for (size_t i = 0; i < array_size(ascii_text_cache_); i++) {
ascii_text_cache_[i].setPerformanceHint(QStaticText::ModerateCaching);
ascii_text_cache_[i].setText(asciiRepresentationFromByte(i));
ascii_text_cache_[i].setTextFormat(Qt::PlainText);
}
}

void HexEdit::updateHexCache() {
for (size_t i = 0; i < array_size(hex_text_cache_); i++) {
hex_text_cache_[i].setPerformanceHint(QStaticText::ModerateCaching);
hex_text_cache_[i].setText(hexRepresentationFromByte(i));
hex_text_cache_[i].setTextFormat(Qt::PlainText);
}
}
QModelIndex HexEdit::selectedChunk() {
if (chunkSelectionModel_ == nullptr) {
return {};
Expand Down Expand Up @@ -609,6 +620,12 @@ void HexEdit::discardChanges() {
viewport()->update();
}

void HexEdit::setUnprintablesMode(QAction* action) {
unprintablesModeString = action->text();
updateAsciiCache();
viewport()->update();
}

qint64 HexEdit::selectionStart() {
if (selection_size_ < 0) {
return current_position_ + selection_size_ + 1;
Expand Down Expand Up @@ -643,10 +660,32 @@ QString HexEdit::addressAsText(qint64 pos) {
}

QString HexEdit::asciiRepresentationFromByte(uint64_t byte_val) {
if (byte_val >= 0x20 && byte_val < 0x7f) {
return QChar::fromLatin1(byte_val);
static QTextCodec* windows1250 = QTextCodec::codecForName("windows-1250");
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check whether windows1250 == nullptr and log a warning + disable this mode when this codec is not available. As we don't have a good logging mechanism right now, just put a TODO here please.

Copy link
Contributor Author

@prybicki prybicki Dec 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Moved the codec to class. Checking for nullity added. Todo added in ctor to display warning when codec in not available.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks ok now.


if (unprintablesModeString.compare("windows-1250") == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This string is taken right from the UI string. Please don't do this - if someone will try to change the text in the UI the code will silently stop to work. Just use an enum.

Copy link
Contributor Author

@prybicki prybicki Dec 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed.

// 0x85 is NEL, 0xa0 is nbsp in unicode, but we want to display them
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a dot after the sentence, now this line and the next looks like a one log sentence ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

0x85 is NEL [...] in unicode - but you don't decode them as Unicode codepoints, you use cp1250. 0x85 in cp1250 is an ellipsis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced comments with better code.

// 0x83, ..., 0x98 are undefined in used codeing
Copy link
Contributor

Choose a reason for hiding this comment

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

codeing -> encoding + dot at the end of the sentence (btw. it should be coding not codeing ;) ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strange mistake. Fixed.

if (byte_val != 0x85 && byte_val != 0xa0 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also handle byte_val > 0xFF somehow (printing a dot for them is fine IMO). The UI doesn't expose such function yet, but all other code supports it.

Copy link
Contributor

Choose a reason for hiding this comment

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

0x81 is also undefined. Please look at the table here: https://www.charset.org/charsets/windows-1250

Copy link
Contributor Author

@prybicki prybicki Dec 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Now returns dot on the beginning.

(QChar(static_cast<uint>(byte_val)).isSpace() ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Cast it to unsigned char or and with 0xFF (so that byte_val > 0xFF won't break this code).

Copy link
Contributor

Choose a reason for hiding this comment

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

In this if you treat byte_val as an Unicode codepoint, but later you decode it as cp1250. This won't work as expected (escpecially isSpace and isNull methods).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bytes which are either undefined or translates to whitespace in CP1250 are displayed as space.

QChar(static_cast<uint>(byte_val)).isNull() || byte_val == 0x83 ||
byte_val == 0x88 || byte_val == 0x90 || byte_val == 0x98)) {
return QChar(static_cast<uint>(' '));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not return ' '; ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot cast char to char*. But " " works.

}

// printable ascii chars
Copy link
Contributor

Choose a reason for hiding this comment

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

ASCII

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

if (byte_val >= 0x20 && byte_val < 0x7f) {
return QChar::fromLatin1(byte_val);
}

// ascii-unprintable chars
Copy link
Contributor

Choose a reason for hiding this comment

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

ASCII

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

char a = (static_cast<char>(byte_val));
Copy link
Contributor

Choose a reason for hiding this comment

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

This conversion is implementation-defined for byte_val >= 0x80, please wrap the higher values by hand (or create a helper function in veles::utils::).
And yes, this seems to be a general problem with Qt APIs - they use char* everywhere for byte arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return byte_val >= 0x7f
? windows1250->toUnicode(&a, 1)
: QChar(static_cast<uint>(byte_val) + 0x180); // greek
}
return ".";
// dots fallback
Copy link
Contributor

Choose a reason for hiding this comment

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

it's rather a "dots mode" ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return (byte_val >= 0x20 && byte_val < 0x7f) ? QChar::fromLatin1(byte_val)
: QString(".");
}

QColor HexEdit::byteTextColorFromByteValue(uint64_t byte_val) {
Expand Down Expand Up @@ -814,7 +853,7 @@ void HexEdit::paintEvent(QPaintEvent* event) {
auto bgc = byteBackroundColorFromPos(
byte_idx, modified_positions[byte_idx - start_byte]);
bool redraw_hex = invalidated_rect.intersects(hex_rect);
bool redraw_ascii = invalidated_rect.intersects(ascii_rect);
bool redraw_ascii = true; // invalidated_rect.intersects(ascii_rect);
Copy link
Contributor

Choose a reason for hiding this comment

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

A leftover from debugging? ;)

Copy link
Contributor Author

@prybicki prybicki Dec 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yup, removed.

if (redraw_hex && bgc.isValid()) {
painter.fillRect(hex_rect, bgc);
}
Expand Down
24 changes: 24 additions & 0 deletions src/ui/hexeditwidget.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ HexEditWidget::HexEditWidget(
setParserIds(dynamic_cast<VelesMainWindow*>(
MainWindowWithDetachableDockWidgets::getFirstMainWindow())
->parsersList());

initUnprintablesMenu();
connect(&unprintables_menu_, &QMenu::triggered, hex_edit_,
&HexEdit::setUnprintablesMode);

selectionChanged(0, 0);
}

Expand Down Expand Up @@ -282,6 +287,18 @@ void HexEditWidget::createToolBars() {
addToolBar(edit_tool_bar_);

view_tool_bar_ = new QToolBar(tr("View"));

auto unprintables_tool_button = new QToolButton();
unprintables_tool_button->setMenu(&unprintables_menu_);
unprintables_tool_button->setPopupMode(QToolButton::InstantPopup);
unprintables_tool_button->setIcon(QIcon(":/images/brightness.png"));
unprintables_tool_button->setText(tr("&Unprintables"));
unprintables_tool_button->setToolTip(tr("Appearance of unprintable bytes"));
unprintables_tool_button->setAutoRaise(true);
auto unprintables_widget_action = new QWidgetAction(view_tool_bar_);
unprintables_widget_action->setDefaultWidget(unprintables_tool_button);

view_tool_bar_->addAction(unprintables_widget_action);
view_tool_bar_->addAction(remove_column_act_);
view_tool_bar_->addAction(add_column_act_);
view_tool_bar_->addSeparator();
Expand All @@ -302,6 +319,13 @@ void HexEditWidget::initParsersMenu() {
}
}

void HexEditWidget::initUnprintablesMenu() {
unprintables_menu_.clear();
unprintables_menu_.addAction("dots");
Copy link
Contributor

Choose a reason for hiding this comment

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

dots -> Dots

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rewritten.

unprintables_menu_.addSeparator();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a separator here? We have only two options, so it not too useful now ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

unprintables_menu_.addAction("windows-1250");
Copy link
Contributor

Choose a reason for hiding this comment

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

windows-1250 -> Windows-1250

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rewritten.

}

void HexEditWidget::createSelectionInfo() {
auto* widget_action = new QWidgetAction(this);
auto* selection_panel = new QWidget;
Expand Down
9 changes: 9 additions & 0 deletions src/ui/optionsdialog.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ void OptionsDialog::show() {
ui->hexColumnsAutoCheckBox->setCheckState(checkState);
ui->hexColumnsSpinBox->setValue(util::settings::hexedit::columnsNumber());
ui->hexColumnsSpinBox->setEnabled(checkState != Qt::Checked);
ui->unprintablesModeDots->setChecked(
util::settings::hexedit::unprintablesModes() == "dots");
ui->unprintablesModeWindows1250->setChecked(
util::settings::hexedit::unprintablesModes() == "windows-1250");

QWidget::show();
}
Expand All @@ -63,6 +67,11 @@ void OptionsDialog::accept() {
ui->hexColumnsAutoCheckBox->checkState() == Qt::Checked);
util::settings::hexedit::setColumnsNumber(ui->hexColumnsSpinBox->value());

if (ui->unprintablesModeDots->isChecked())
util::settings::hexedit::setUnprintablesMode("dots");
if (ui->unprintablesModeWindows1250->isChecked())
util::settings::hexedit::setUnprintablesMode("windows-1250");

if (restart_needed) {
QMessageBox::about(
this, tr("Options change"),
Expand Down
97 changes: 64 additions & 33 deletions src/ui/optionsdialog.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
<rect>
<x>0</x>
<y>0</y>
<width>259</width>
<height>229</height>
<width>337</width>
<height>296</height>
</rect>
</property>
<property name="windowTitle">
Expand Down Expand Up @@ -49,42 +49,73 @@
<property name="title">
<string>HexEdit defaults</string>
</property>
<layout class="QVBoxLayout" name="verticalLayout_3">
<item>
<layout class="QHBoxLayout" name="horizontalLayout_2">
<layout class="QGridLayout" name="gridLayout">
<item row="1" column="1">
<layout class="QVBoxLayout" name="verticalLayout_3">
<item>
<widget class="QLabel" name="hexColumnsLabel">
<widget class="QRadioButton" name="unprintablesModeWindows1250">
<property name="text">
<string>Columns</string>
<string>Windows-1250</string>
</property>
</widget>
</item>
</layout>
</item>
<item row="3" column="1">
<layout class="QVBoxLayout" name="verticalLayout_2">
<item>
<layout class="QVBoxLayout" name="verticalLayout_2">
<item>
<widget class="QSpinBox" name="hexColumnsSpinBox">
<property name="minimum">
<number>1</number>
</property>
<property name="maximum">
<number>1024</number>
</property>
<property name="value">
<number>16</number>
</property>
</widget>
</item>
<item>
<widget class="QCheckBox" name="hexColumnsAutoCheckBox">
<property name="text">
<string>Resize to window width</string>
</property>
</widget>
</item>
</layout>
<widget class="QSpinBox" name="hexColumnsSpinBox">
<property name="minimum">
<number>1</number>
</property>
<property name="maximum">
<number>1024</number>
</property>
<property name="value">
<number>16</number>
</property>
</widget>
</item>
<item>
<widget class="QCheckBox" name="hexColumnsAutoCheckBox">
<property name="text">
<string>Resize to window width</string>
</property>
</widget>
</item>
</layout>
</item>
<item row="3" column="0">
<widget class="QLabel" name="hexColumnsLabel">
<property name="text">
<string>Columns</string>
</property>
</widget>
</item>
<item row="2" column="0" colspan="2">
<widget class="Line" name="line">
<property name="orientation">
<enum>Qt::Horizontal</enum>
</property>
</widget>
</item>
<item row="0" column="1">
<widget class="QRadioButton" name="unprintablesModeDots">
<property name="text">
<string>Dots</string>
</property>
</widget>
</item>
<item row="0" column="0" rowspan="2">
<widget class="QLabel" name="label">
<property name="text">
<string>Unprintable characters</string>
</property>
<property name="wordWrap">
<bool>true</bool>
</property>
</widget>
</item>
</layout>
</widget>
</item>
Expand Down Expand Up @@ -112,8 +143,8 @@
<slot>reject()</slot>
<hints>
<hint type="sourcelabel">
<x>290</x>
<y>160</y>
<x>299</x>
<y>286</y>
</hint>
<hint type="destinationlabel">
<x>286</x>
Expand All @@ -128,8 +159,8 @@
<slot>accept()</slot>
<hints>
<hint type="sourcelabel">
<x>222</x>
<y>154</y>
<x>231</x>
<y>286</y>
</hint>
<hint type="destinationlabel">
<x>157</x>
Expand Down
Loading