Change byte display gradient in Hex Edit by janczer · Pull Request #328 · codilime/veles

janczer · 2017-09-14T18:26:25Z

Also I added color picker to option. #255

This change is

mkow

This gradient has very poor readability on my monitor. IMO it would be better to lighten the color for high byte values (looking from dark theme perspective). If you want you can create two totally separate gradient scales for both themes, our colorInvertedIfDark() don't give good results here.
Default color for the dark theme should be more greenish (green is much more readable on dark background than blue).
Current scale is much too flat: 00 is not too different than FF.

The effect which I'd like to achieve is the same text readability (including contrast with background) than we currently have, but at the same time the hex view should allow easier spotting of structures and patterns in the bytes. For example, see this comparison between Veles (left) and Hexplorer (right): (click to enlarge)

Our current color scheme is much prettier than the one in Hexplorer, but it's harder to see patterns in it (because, for example, 00 is brighter than 50 in current scale). The scheme in Hexplorer isn't perfect, so you don't have to use exactly the same scale as they use ;) Just play a bit with different settings and try to find something good.

mkow · 2017-09-15T12:14:29Z

include/ui/optionsdialog.h

  void accept() override;
+
+ private:
+  QColorDialog* color_dialog;


Should be color_dialog_ (see https://google.github.io/styleguide/cppguide.html#Variable_Names).

mkow · 2017-09-15T12:21:20Z

include/util/settings/hexedit.h

 bool resizeColumnsToWindowWidth();
 void setResizeColumnsToWindowWidth(bool on);
+QColor colorOfText();
+void setColorOfText(QColor color);


This name is IMO misleading, it's not a color of a text in a hex view (e.g. it doesn't change the color of addresses). Maybe something like ColorOfBytes or ColorOfByteValues would be better?

color should be passed by const ref: setColorOfText(const QColor& color)

I changed it to colorOfBytes and setColorOfBytes.

mkow · 2017-09-15T12:24:14Z

src/ui/optionsdialog.cc

 *
 */
 #include "include/ui/optionsdialog.h"
+#include <QColorDialog>


Please add a newline before QColorDialog (see https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes).

mkow · 2017-09-15T12:34:25Z

src/ui/optionsdialog.cc

+
+  connect(ui->colorHexEdit, &QPushButton::clicked, color_dialog,
+          &QColorDialog::show);
+


remove this empty line

mkow · 2017-09-15T12:34:32Z

src/ui/optionsdialog.cc

+
+  connect(color_dialog, &QColorDialog::colorSelected, this,
+          &OptionsDialog::updateColorButton);
+


mkow · 2017-09-15T12:55:01Z

src/ui/optionsdialog.ui

+        <item>
+         <widget class="QLabel" name="labelHexEditWidgetColors">
+          <property name="text">
+           <string>Color</string>


This should be placed under "HexEdit" group in options dialog. When doing this, please change "HexEdit defaults" to "HexEdit" and "Columns" to "Default columns".

Also: "Color" -> "Byte color"

I changed it.

mkow · 2017-09-15T12:56:03Z

src/ui/optionsdialog.ui

+         </widget>
+        </item>
+        <item>
+         <widget class="QPushButton" name="colorHexEdit">


colorHexEdit -> byteColorHexEdit

I changed it.

mkow · 2017-09-15T12:56:19Z

src/util/settings/hexedit.cc

 *
 */
 #include "util/settings/hexedit.h"
+#include <QColor>


newline before QColor

mkow · 2017-09-15T12:57:23Z

src/util/settings/hexedit.cc


+QColor colorOfText() {
+  QSettings settings;
+  auto v = settings.value("hexedit.colorOfText", QColor(10, 10, 255));


colorOfText -> colorOfBytes

I changed it. Also I changed how it's store in settings. I changed from QColor to QRgb. I'm not sure if it's ok. Maybe will be better store it like separate red, green and blue channels?

I think it's better to store it as QColor. Are there any pros of using QRgb?

More pretty Veles.conf:

Stored QRgb:

hexedit.colorOfBytes=4278190592

and stored QColor:

hexedit.colorOfBytes=@Variant(\0\0\0\x43\x1\xff\xff\x87\x87\x62\x62\xff\xff\0\0)

I'd prefer the previous version (without QRgb). colorOfBytes=4278190592 is still not too readable nor useful for users. They are also not expected to change config files manually. QColor(10, 10, 255) is much more readable in the code: you see separate values for each of RGB channels and you can easily change each of them.

mkow · 2017-09-15T13:19:44Z

src/util/settings/theme.cc

-
-  return colorInvertedIfDark(QColor(red, green, blue));
+  QColor color = util::settings::hexedit::colorOfText();
+  color.setAlpha(byte / 2 + 127);


Please don't use setAlpha - we don't want to blend it with the color of the background (which is not always dark/white). IMO we should use a constant color gradient (independent of the background), the current version is hard to read when there are some chunks added (they change byte background).

Just decide on the gradient parameters and calculate the color ;) Doing this should be quite easy using getHsv/setHsv methods (or getHsl/setHsl). Or just use QColor::lighter / QColor::darker which does the same.

I'm still working on this.

Ok, I'll wait for the rest and then do next review iteration

I changed setAlpha to QColor::darker it's allow choose color with max brightness.

mkow · 2017-09-15T13:58:51Z

Fixes #325.

mkow · 2017-09-23T15:51:34Z

src/util/settings/theme.cc

-
-  return colorInvertedIfDark(QColor(red, green, blue));
+  QColor color = util::settings::hexedit::colorOfBytes();
+  return color.darker(100 + (byte ^ 0xFF) * 200 / 255);


It may not be obvious for a reader that byte ^ 0xFF preserves correct ordering of the values, IMO 0x100 - byte is more readable.

janczer · 2017-09-25T19:41:50Z

I was playing with representation color of bytes and @mkow proposed use HSV or HSL and I'm trying change value in HSV and saturation with lightness in HSL:

return QColor::fromHsv(color.hue(), color.saturation(), 40 + byte * (230 - 50) / 256);

return QColor::fromHsl(color.hue(), color.saturation(), 40 + byte * (230 - 50) / 256);

return QColor::fromHsl(color.hue(), 150 + byte * (255 - 200) / 256, 60 + byte * (230 - 50) / 256);

return QColor::fromHsl(color.hue(), 100 + byte * (255 - 200) / 256, 40 + byte * (230 - 50) / 256);

return QColor::fromHsl(color.hue(), 200 + byte * (255 - 200) / 256, 60 + byte * (230 - 50) / 256);

I'm not sure what option choose.

…dering of the values

mkow · 2018-01-26T15:01:21Z

I did some more experiments and it seems that:

The background for the hex and ASCII columns should be black (this increases contrast, so the low-value bytes are still readable).
We should disable anti-aliasing for hex and ASCII view (this can be achieved by font.setStyleStrategy(QFont::NoAntialias)). Not sure if it's a good idea on Ultra-HD displays, so it should be changeable in options. IMO it would be the best to implement this in a separate PR (it should also rename settings::font() to settings::fixed_width_font()).
The selection color should be changed, the current one makes selected text unreadable.
It would be nice to leave the old hex coloring scheme selectable as a non-default option (it's less practical but it looks nice, so maybe some users will prefer it).
I looked at Hexplorer source code and they use the following expression for their default color scheme:
```
unsigned limit = 127;
unsigned brightness = byte > limit ? byte - limit : 0;
return QColor(/*r=*/brightness, /*g=*/(byte / 2) + 0x5d, /*b=*/0x3c + brightness);
```
It looks nice, IMO we should play a bit more with it. The general idea here is to not just interpolate between two colors, but to sum two linear gradients (where one starts in the middle of the scale).
If we decide to use this idea we should leave some credit for the original authors (e.g. name this scheme Matrix (Hexplorer-like)).

Some technical remarks:

The code should be able to handle byte values > 255 well (this strange feature isn't exposed yet in the UI, but it's implemented internally; it's intended for some exotic non 8-bits-per-byte architectures/file formats and possibly a WORD/DWORD/QWORD view mode). We can deal with it later, because it's not a priority now.

mkow · 2018-01-26T15:30:38Z

Also: the third scheme suggested by @janczer also looks well on the black background with disabled anti-aliasing.

mkow self-requested a review September 15, 2017 11:19

mkow changed the title ~~Color intensity depends on byte value~~ Change byte display gradient in Hex Edit Sep 15, 2017

mkow suggested changes Sep 15, 2017

View reviewed changes

mkow reviewed Sep 23, 2017

View reviewed changes

janczer force-pushed the change-hex-view-color branch from f16b1bd to fddeba0 Compare November 14, 2017 12:48

janczer added 6 commits January 19, 2018 20:15

Add color picker to option, color intensity depends on byte value

8fca85d

Fixed after code review

ff487e3

Change default color to green, and change color with QColor::darker

4ce8f61

Update color in hexedit after change color

ae5b875

Changing type of saving color, changing method to preserve correct or…

88cbb39

…dering of the values

Update represent bytes

4de9994

janczer force-pushed the change-hex-view-color branch from fddeba0 to 4de9994 Compare January 19, 2018 19:17


		connect(ui->colorHexEdit, &QPushButton::clicked, color_dialog,
		&QColorDialog::show);


		connect(color_dialog, &QColorDialog::colorSelected, this,
		&OptionsDialog::updateColorButton);

Conversation

janczer commented Sep 14, 2017 • edited by mkow Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkow left a comment

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!

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!

mkow commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janczer commented Sep 25, 2017

Uh oh!

mkow commented Jan 26, 2018

Uh oh!

mkow commented Jan 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

janczer commented Sep 14, 2017 •

edited by mkow

Loading