Add more representations of unprintable bytes #380

prybicki · 2017-11-30T17:46:20Z

Convert byte to pretty unicode representation
Create gui options of default unprintable characters rendering
Save unprintable mode default settings
Add icon to view toolbar
Add instant unprintable mode changing

TODO: Find reasonable icon.

This change is

*) Convert byte to pretty unicode representation *) Create gui options of default unprintable characters rendering *) Save unprintable mode default settings *) Add icon to view toolbar *) Add instant unprintable mode changing

mkow · 2017-12-03T12:14:27Z

include/ui/hexedit.h

  QScopedPointer<util::encoders::TextEncoder> textEncoder_;
  util::EditEngine edit_engine_;

+  QString unprintablesModeString;


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

mkow · 2017-12-03T12:16:30Z

src/ui/hexedit.cc

+#include "util/settings/hexedit.h"
 #include "util/settings/theme.h"

+#include <unistd.h>


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

Just a debugging leftover, removed.

mkow · 2017-12-03T12:20:21Z

src/ui/hexedit.cc

  });
 }

+void HexEdit::updateAsciiCache() {


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

Same for HexEdit::updateHexCache()

mkow · 2017-12-03T12:24:26Z

include/util/settings/hexedit.h

 void setColumnsNumber(int number);
 bool resizeColumnsToWindowWidth();
 void setResizeColumnsToWindowWidth(bool on);
+QString unprintablesModes();


IMO it should be unprintablesMode (without s).

Yes, typo. Thanks for catching.

mkow · 2017-12-03T12:24:49Z

include/util/settings/hexedit.h

 bool resizeColumnsToWindowWidth();
 void setResizeColumnsToWindowWidth(bool on);
+QString unprintablesModes();
+void setUnprintablesMode(QString mode);


Should be const QString& mode.

mkow · 2017-12-03T13:33:59Z

src/ui/hexedit.cc

+    }
+
+    // ascii-unprintable chars
+    char a = (static_cast<char>(byte_val));


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.

mkow · 2017-12-03T13:40:17Z

src/ui/hexedit.cc

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


A leftover from debugging? ;)

Yup, removed.

mkow · 2017-12-03T13:45:17Z

src/ui/hexeditwidget.cc

+  unprintables_menu_.clear();
+  unprintables_menu_.addAction("dots");
+  unprintables_menu_.addSeparator();
+  unprintables_menu_.addAction("windows-1250");


windows-1250 -> Windows-1250

mkow · 2017-12-03T13:45:46Z

src/ui/hexeditwidget.cc

+void HexEditWidget::initUnprintablesMenu() {
+  unprintables_menu_.clear();
+  unprintables_menu_.addAction("dots");
+  unprintables_menu_.addSeparator();


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

mkow · 2017-12-03T13:46:42Z

src/ui/hexeditwidget.cc


+void HexEditWidget::initUnprintablesMenu() {
+  unprintables_menu_.clear();
+  unprintables_menu_.addAction("dots");


dots -> Dots

mkow · 2017-12-07T14:54:04Z

include/ui/hexedit.h

 public:
+  enum class UnprintablesMode {
+    Dots,
+    Windows_1250,


Use uppercase names (see Google style guide)

mkow · 2017-12-07T14:54:52Z

include/ui/hexedit.h

+  enum class UnprintablesMode {
+    Dots,
+    Windows_1250,
+  };  // do not change the order as settings may invalidate.


Please put this comment right after the enum class ... line.

mkow · 2017-12-07T14:57:44Z

src/ui/hexedit.cc

+
+void HexEdit::setUnprintablesMode(UnprintablesMode mode) {
+  unprintables_mode_ = mode;
+  if (mode == UnprintablesMode::Windows_1250 && windows1250_codec_ == nullptr) {


You should do this check before assigning to unprintables_mode_.

mkow · 2017-12-07T14:58:21Z

src/ui/hexedit.cc

+void HexEdit::setUnprintablesMode(UnprintablesMode mode) {
+  unprintables_mode_ = mode;
+  if (mode == UnprintablesMode::Windows_1250 && windows1250_codec_ == nullptr) {
+    QMessageBox::warning(this, "Error", "Windows-1250 is unavailable.",


"Windows-1250 is -> "Windows-1250 encoding is

mkow · 2017-12-07T15:01:11Z

src/ui/hexedit.cc

  saveDataToFile(0, edit_engine_.dataSize(), file_name);
 }

+QString HexEdit::unprintablesModeToString(UnprintablesMode mode) {


This method can be static.

mkow · 2017-12-07T15:34:31Z

src/ui/hexedit.cc

+
+    bool is_whitespace = unicode_repr.isSpace() || unicode_repr.isNull();
+
+    if (is_whitespace || is_undefined_windows1250) {


Please add a check for 0x7f here, it decodes to DEL and it's not printable.

mkow · 2017-12-07T15:34:48Z

src/ui/hexedit.cc

+    }
+
+    // unprintable ASCII chars
+    return byte_val >= 0x7f ? windows1250_codec_->toUnicode(&a, 1)


>= 0x7f -> > 0x7f

mkow · 2017-12-07T15:41:01Z

src/ui/hexeditwidget.cc

+    QAction* action = new QAction(hex_edit_->unprintablesModeToString(mode),
+                                  &unprintables_menu_);
+    connect(action, &QAction::triggered,
+            [=]() { this->hex_edit_->setUnprintablesMode(mode); });


Please use [this, mode], it's less error-prone and more explicit.

mkow · 2017-12-07T15:41:47Z

src/ui/optionsdialog.cc


 #include <QMessageBox>

+#include <ui/hexedit.h>


Please fix:

Include grouping

Use of <>

mkow · 2017-12-07T15:43:17Z

src/util/misc.cc

 *
 */
 #include "util/misc.h"
+#include <cstdint>


Fix include grouping.

mkow

Looks good now. We can merge after I find a more reasonable icon for the button.

Add more representations of unprintable bytes

52968d6

*) Convert byte to pretty unicode representation *) Create gui options of default unprintable characters rendering *) Save unprintable mode default settings *) Add icon to view toolbar *) Add instant unprintable mode changing

prybicki requested a review from mkow November 30, 2017 17:46

mkow suggested changes Dec 3, 2017

View reviewed changes

Peter Rybicki added 2 commits December 6, 2017 16:46

Review fixup: minor issues (wip)

51561bb

Review fixup: enumized unprintablesMode

6a98497

mkow suggested changes Dec 7, 2017

View reviewed changes

mkow changed the title ~~[WIP] Add more representations of unprintable bytes~~ Add more representations of unprintable bytes Dec 9, 2017

Peter Rybicki added 2 commits December 13, 2017 14:09

Review fixup 3

57a8a2f

Jenkins fixup

f894426

mkow reviewed Dec 13, 2017

View reviewed changes

mkow mentioned this pull request Jan 26, 2018

Add more encodings to the text representation in the hex view #401

Open


		bool is_whitespace = unicode_repr.isSpace() \|\| unicode_repr.isNull();

		if (is_whitespace \|\| is_undefined_windows1250) {

Add more representations of unprintable bytes #380

Are you sure you want to change the base?

Add more representations of unprintable bytes #380

Uh oh!

Conversation

prybicki commented Nov 30, 2017 • edited by mkow Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prybicki Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prybicki Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prybicki commented Nov 30, 2017 •

edited by mkow

Loading

prybicki Dec 6, 2017 •

edited

Loading

prybicki Dec 6, 2017 •

edited

Loading