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.

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

QHexView 5.0 Development #71

Open
Dax89 opened this issue Feb 24, 2022 · 94 comments
Open

QHexView 5.0 Development #71

Dax89 opened this issue Feb 24, 2022 · 94 comments
Assignees
Labels

Comments

@Dax89
Copy link
Owner

Dax89 commented Feb 24, 2022

QHexView 5.0 is now under development! The widget is almost rewritten from scratch in order to keep its codebase as simple as possible.
I'm interested to listen all feedbacks here in order to create the best release as possible (if anyone is interested, of course!).

The current widget's appearance is this one:
QHexView

Changes:

  • QHexView doesn't use hardcoded colors anymore, but it relies on QPalette
  • It's possible to customize (almost entirely) the widget with the new QHexOptions class
  • It's possible to create columns with groups 1, 2, 4, 8, etc bytes
  • Replaced the blinking cursor with a fixed one
  • Configurable scroll speed (through QHexOptions)

Known Bugs/Missing Features:

  • Mouse input can be improved
  • Selection highlighting can be improved
  • Horizontal scroll is not implemented

Wants:

  • Qt 5.6 support (someone asked for it)
  • Keep C++11 as minimum target
  • Windows support (I work in a Linux environment, but I can test it on WIndows too)
  • Restore highlighting support
  • Restore comments/notes support
  • More?

Branch Link: https://github.com/Dax89/QHexView/tree/5.0

Sample code

//The second argument is a QHexOption
QHexDocument* doc = QHexDocument::fromFile<QMemoryBuffer>("/home/davide/Programmazione/Cavia.exe", { }, this);
doc->setGroupLength(2); // Default is 1, see options here: https://github.com/Dax89/QHexView/blob/5.0/document/qhexoptions.h
ui->hexView->setDocument(doc);

@audetto @T-640 @neomissing @aerosoul94

If you are not interested, feel free to ignore this notification/email!

@Dax89 Dax89 added the 5.0 label Feb 24, 2022
@Dax89 Dax89 self-assigned this Feb 24, 2022
@Dax89 Dax89 pinned this issue Feb 24, 2022
@T-640
Copy link

T-640 commented Mar 2, 2022

Excellent, and thanks for notification as well!

Here is what I can report so far from version downloaded yesterday:

  1. Rendering is messed up, take a look:

HexMessedUp 2880x1800

This happened on Windows 11, on high DPI (2880x1800) display. Not sure if that matters, but the old version works fine on this system.

  1. If QWidget::show is called and QHexDocument is not set for the widget a crash occurs.

  2. qhexutils.h and qhexutils.cpp are not included in the .pri file.

  3. The lines separating columns are gone, could you bring them back please? They could be another optional feature. And there also could be an option to style them like it is done for QFrame's VLine shape (plain, raised, sunken styles), take a look at screenshot in QFrame's documentation - https://doc.qt.io/qt-5/qframe.html

  4. Highlighting is restored and appears to be functional, so you might want to remove it from the "to do" list above. Also I see that it has a higher level API now, with functions like QHexMetadata::setBackground it is much more convenient to use, well done!

  5. The API has become more terse, which is good of course. The only renamings I am not sure about are from QHexView::hexCursor and QHexView::hexDocument functions, I believe just the "cursor" and "document" should be enough because:

  • Anyone can see in the header file that they return QHexCursor and QHexDocument, so adding an extra "hex" in function names seems to be redundant;
  • It is obvious from the context what kind of cursor and document is being dealt with here. What else could that be? Are you concerned about confusion with similar functions of QTextEdit? I don't think that there is a problem here.

Porting to the new version was easy, so once that rendering issue is fixed I think I will be ready to use QHexView 5.0 full-time.

@Dax89
Copy link
Owner Author

Dax89 commented Mar 2, 2022

  1. I will investigate this issue: it doesn't happen on my Windows 10 VM
  2. Fixed
  3. Added qhexutils.h and qhexutils.cpp files in .pri file
  4. Restored as separator option (in QHexOptions), it's also possible to change the color: currently is drawn as a simple QPainter's line.
    It's also possible to use a QFrame's VLine as you asked, but we lose the ability to change its color, even the styles (Plain, Raised and Sunken) doesn't seems to be honored for VLine/HLine
  5. Done
  6. I can change that, but a cursor method already exists because QWidget implements that and QHexView hides that one.
    A possible solution is to remove hexCursor from QHexView and rename hexDocument as document,

Thanks for the feedback!

@T-640
Copy link

T-640 commented Mar 2, 2022

A possible solution is to remove hexCursor from QHexView

Pubic access to QHexCursor from QHexView is needed to at least being able to switch modes. Removing would make sense if you can completely hide QHexCursor within QHexView's implementation, and add functions like QHexView::setCursorMode to relay calls to the underlying cursor. But QHexCursor has more than a dozen public functions, so adding all these 'proxy' functions would likely clutter QHexView's own API.

So I'd say if you come up with a way to hide QHexCursor in QHexView go for it, if not let the "hexXXX" names stay for consistency then.

@T-640
Copy link

T-640 commented Mar 2, 2022

Update on the rendering issue:

Tested on an older notebook (Windows 10, 1366x768 display), messed up there as well. Old version works fine on that machine.

Try changing display resolution of your virtual machine, perhaps you have accidentally hardcoded some value that happens to work for you particular resolution?

@Dax89
Copy link
Owner Author

Dax89 commented Mar 3, 2022

I need two informations (so I can try to reproduce this issue):

  • The font selected by the widget
  • It might be useful to know the Qt version

I've tried to change the resolution and used different fonts, I can't reproduce it.

@T-640
Copy link

T-640 commented Mar 3, 2022

Qt version is 5.14.2.

The result of this (on Windows 11):

qDebug () << hex_view.font() << QFontDatabase::systemFont(QFontDatabase::FixedFont);

is

QFont(Courier New,9,-1,2,50,0,0,0,0,0) QFont(Courier New,9,-1,2,50,0,0,0,0,0)

Let me know if you need font results from the other Windows 10 machine.

@T-640
Copy link

T-640 commented Mar 3, 2022

Looks like I found the fix for this.

The issue is in QHexView::renderDocument, in this block:

QHexView/qhexview.cpp

Lines 206 to 219 in 34e1dcb

// Hex Part
for(auto column = 0u; column < this->options()->linelength; )
{
QTextCharFormat cf;
for(auto byteidx = 0u; byteidx < this->options()->grouplength; byteidx++, column++)
{
QString s = linebytes.isEmpty() || column >= static_cast<qint64>(linebytes.size()) ? " " : QHexUtils::toHex(linebytes.mid(column, 1)).toUpper();
quint8 b = static_cast<int>(column) < linebytes.size() ? linebytes.at(column) : 0x00;
cf = this->drawFormat(c, b, s, Area::Hex, line, column, static_cast<int>(column) < linebytes.size());
}
c.insertText(" ", cf);
}

Convert the result of this intoQString:

QHexUtils::toHex(linebytes.mid(column, 1)).toUpper();

Like this, for example:

QString(QHexUtils::toHex(linebytes.mid(column, 1)).toUpper())

This brought everything back to normal for me. I have no idea how exactly this fixes the issue, but I suppose using QStrings instead of QByteArrays when drawing could be more reasonable choice. So perhaps you should make sure that QByteArrays get converted to QStrings within functions that deal with rendering.

@Dax89
Copy link
Owner Author

Dax89 commented Mar 3, 2022

Great!
It must be a nasty implicit cast with some Qt/Compiler/OS specific combo

Pushed! Let me know if it's ok now.

@T-640
Copy link

T-640 commented Mar 3, 2022

Everything looks fine. Now, to the lesser issues:

  1. In Hex area background highlighting grabs an extra character (an empty character that separates representation of bytes). In ASCII area it works fine, highlighting from cursor selection is correct too.

Extra space

The green color demonstrates the issue, the black one is just cursor selection, working correctly.

  1. When copying bytes from hex area via Ctrl + C the last byte is never copied.

Last hex copy

The FF byte will not be copied.

  1. Regarding the Renderer does not work well with widget’s width #57

In the old version I used a small hack to adjust widget's size to what is actually being rendered (to remove that useless white space after ASCII column) by calling QHexRenderer::documentWidth, but it is gone now. I was giving user an option to change the size of QHexView by making its font larger, and then readjusted its width via a call to that function. But the best solution would probably be to implement some function like QHexView::adjustWidthToContent(bool) or make it default behaviour of QHexView altogether. I suppose the situation when widget becomes too large needs to be considered in that case, or when user wants to set fixed width for it.

The whole idea of what is the best way to resize QHexView may need more speculation though, because resizing such widget correctly seems to be more challenging than doing it with others. Changing its font and adjusting width accordingly is what I could come up with, it is a bit stiff but fairly simple. Maybe there is a better idea, but for now this will do.

  1. It still crashes unless I set QHexDocument within my subclass of QHexView first.

  2. Alternate row colour starts in the wrong place:

Rows

  1. Vertical scrollbar. Probably not a bug, but rather an aesthetical preference. It feels like it should be larger. Take a look:

Small scrollbar

This view has 17 rows, two of which are not seen. Scrollbar steps correctly, that is, twice, as it is supposed to. I am only pointing out its appearance. Shouldn't it be longer?

For comparison, this is a scrollbar from a QTreeView with 17 root items (not expanded), it is twice as large:

Large scrollbar

Other than that everything else that I use appears to work well so far. Editing works fine, both in Hex and ASCII areas, both modes, insert and overwrite work correctly. I mentioned changing font above, that works well too.

@Dax89
Copy link
Owner Author

Dax89 commented Mar 4, 2022

I've fixed 1, 2, 5 and 6 along with a bunch of other minor fixes/improvements.

About the other issues:

  • 3: Do you want that the widget calculates the line's length according to its width?
  • 4: I can't reproduce it, is it possible to have a sample code which causes this issue?

@T-640
Copy link

T-640 commented Mar 4, 2022

Do you want that the widget calculates the line's length according to its width?

The reverse of that, if I understood the question correctly. But this could work too I suppose. Either way I would like widget's width (just width, not height) to be "synchronised" with the actual content, with what is being drawn (hex , ASCII areas, etc.). Right now widget draws offset, hex and ASCII columns and anything after that is wasted space (if widget's width is larger than what these 3 columns occupy).

So if widget is too wide I think its width should be reduced. In the old version I did something like this:

hex_view->setMaximumWidth(m_renderer->documentWidth());.

It may have some implications since widget is programmatically resized, but the idea seems to be worth exploring.

4: I can't reproduce it, is it possible to have a sample code which causes this issue?

I figured it out. New QHexView does not create QHexDocument unless it is explicitly set by QHexView::setDocument. In the old version, an empty one was always created at the end of the constructor:

this->setDocument(QHexDocument::fromMemory<QMemoryBuffer>(QByteArray(), this));

My program crashed because of this:

  1. QHexView instance is created, QHexDocument is not set (it will be when something is actually loaded);
  2. QSettings instance is being read to retrieve "insert" or "overwrite" mode setting and calls hex_view->hexDocument()->cursor()->setMode();;
  3. But because QHexDocument has not been set yet, QHexView::hexDocument returns nullptr and thus crash occurs.

The simplest way to fix is to just copy that single line from the old version. Although perhaps cursor mode could be set directly from QHexView, with something like QHexView::setCursorMode, thus bypassing the need to call QHexView::hexDocument? Unless it is designed to handle multiple cursors, like QTextEdit though. And it could unnecessarily bloat QHexView's public API too I suppose, so I am not sure.

A few more bugs:

  1. Alternate colour is still drawn incorrectly, but in a different way:

Alternate still incorrecy

  1. At certain widget height scrollbar does not appear when it is supposed to:

F0 hidden

In this screenshot there is actually one more line at offset 0xF0. It is not visible, thus scrollbar is supposed to appear, but it doesn't. Reducing widget's height slightly makes scrollbar to appear.

@Dax89
Copy link
Owner Author

Dax89 commented Mar 4, 2022

I have added QHexView::setCursorMode and QHexView::setAutoWidth in order to rescale the viewport's width according to the rendered area, let me know if it's ok now.

And I've also fixed all issues you have reported, an empty QHexDocument is created by default too, like the previous version.

@T-640
Copy link

T-640 commented Mar 4, 2022

Issue 8:

Now scrollbar area appears, I can see up and down arrows, but the scrollbar itself is not there, so I am still unable to scroll down to the 0xF0 line.

QHexView::setAutoWidth:

When QByteArray supplied is large, that is, when vertical scrollbar needs to appear, it does readjust width successfully. However, there are two occasions when it does not do that: when QHexDocument is empty and when the size of QByteArray is small (when there is no need for scrollbar).

Also while the widget itself correctly adjusts its width, the QAbstractScrollArea::viewport does not seem to - horizontal scrollbar appeared and I could scroll a lot to the right.

@T-640
Copy link

T-640 commented Mar 4, 2022

And there is also one more thing that could be restored from the old version: in Hex area bytes 00 and FF were coloured differently from the rest, and dots in ASCII area were too. Now that there is QHexOptions it would make a good optional feature. Maybe even provide an option to change colour of these sort of special bytes.

Different color

@Dax89
Copy link
Owner Author

Dax89 commented Mar 5, 2022

I've improved the scrollbar's behavior, issue 8 should be fixed.

You can highlight any byte by using QHexOptions::bytecolors map, for example:

QHexOptions options;
options.bytecolors[0x00] = {Qt::lightGray, QColor()};  // {Foreground, Background}
options.bytecolors[0xFF] = {Qt::darkBlue, QColor()};   // {Foreground, Background}

hexview->setOptions(options);

It's also possible to highlight bytes from the widget itself with these functions:

  • void QHexView::setByteColor(quint8 b, QHexColor c)
  • void QHexView::setByteForeground(quint8 b, QColor c)
  • void QHexView::setByteBackground(quint8 b, QColor c)

By default the new version doesn't highlight anything.

Screenshot
Screenshot_20220305_105006

@T-640
Copy link

T-640 commented Mar 5, 2022

issue 8 should be fixed

It is, but one more emerged: it appears that there is some kind of invisible "scrollbar" when there is no actual scrollbar, mouse wheel can be used to trigger "it" and viewport will scroll one hex line down (so it essentially looks like the first line just disappeared). If mode is switched to insertion and you keep inserting till the new line is created it automatically does that invisible scroll too. And when you scroll with this invisible "scrollbar" it scrolls one empty line further than the last actual line.

void QHexView::setByteColor(quint8 b, QHexColor c)

When set in constructor it appears to be applied to the current default empty QHexDocument. But when it is replaced via the QHexDocument::setDocument, the new one does not receive these options. I guess there should be some optional way to preserve previous QHexOptions.

void QHexView::setByteForeground(quint8 b, QColor c)

If only the foreground is set for the byte, during painting the QHexOptions::linealternatebackground is ignored and QPalette::Base is applied instead:

cf.setBackground(it->background.isValid() ? it->background : this->palette().color(QPalette::Base));

Replacing it with this worked for me:

if (it->background.isValid()) cf.setBackground(it->background);

It takes advantage of the fact that QHexView::drawFormat is being called before the row is coloured with an alternate colour.

But I have not studied the rendering code thoroughly, so without having a bigger picture I might have missed something.

@Dax89
Copy link
Owner Author

Dax89 commented Mar 6, 2022

It is, but one more emerged: it appears that there is some kind of invisible "scrollbar" when there is no actual scrollbar, mouse wheel can be used to trigger "it" and viewport will scroll one hex line down (so it essentially looks like the first line just disappeared). If mode is switched to insertion and you keep inserting till the new line is created it automatically does that invisible scroll too. And when you scroll with this invisible "scrollbar" it scrolls one empty line further than the last actual line.

I have found an issue similar to this one, check if it's fixed now.

When set in constructor it appears to be applied to the current default empty QHexDocument. But when it is replaced via the QHexDocument::setDocument, the new one does not receive these options. I guess there should be some optional way to preserve previous QHexOptions.

I have added QHexView::resetDocument which copies QHexMetadata, QHexOptions and its base address to the new QHexDocument.

If only the foreground is set for the byte, during painting the QHexOptions::linealternatebackground is ignored and QPalette::Base is applied instead:

cf.setBackground(it->background.isValid() ? it->background : this->palette().color(QPalette::Base));

Replacing it with this worked for me:

if (it->background.isValid()) cf.setBackground(it->background);

Fixed

@T-640
Copy link

T-640 commented Mar 6, 2022

I have found an issue similar to this one, check if it's fixed now.

I'm afraid not. To reproduce, create an empty QHexView, and use mouse wheel to scroll down. Despite having no data and without a visible scrollbar it will still scroll down to offset 0x10.

I have added QHexView::resetDocument which copies QHexMetadata...

I am not sure that this is a good idea to copy anything other than QHexOptions. Suppose we are done dealing with a particular QByteArray and load a new one, creating a new document and calling QHexView::resetDocument. The new QByteArray may require completely different highlighting, comments, etc., but the old metadata being copied will possibly mess everything up. And I suppose same applies to base address being copied too, although I have never used it.

I think that the difference between QHexOptions and QHexMetadata is that the former is something that it is more generic, and thus stable and should be preserved, while the latter is more volatile because of being strongly coupled with particular binary data supplied to the widget.

So perhaps QHexOptions should be applied to the whole QHexView instead of just the underlying QHexDocument? Pretty much like QPalette: We've got QWidget::palette to get a copy of it, change whatever we need in it, then set it back via the QWidget::setPalette. Same could be be done with QHexView: you could add QHexView::hexOptions and QHexView::setHexOptions to operate in a similar fashion. QPalette and QHexOptions are even similar due to the fact that they are both "simple" copiable classes, while QHexMetadata is a QObject subclass.

The only other thing that I think needs to be copied is cursor's mode. Perhaps it could be part of QHexOptions too then?

@Dax89
Copy link
Owner Author

Dax89 commented Mar 6, 2022

Mmmmmh, it makes sense.

At this point even QHexCursor should be part of QHexView: so it's possible to have multiple views with the same QHexDocument but different QHexCursor and QHexOptions.

@T-640
Copy link

T-640 commented Mar 6, 2022

Maybe, although I can not say much here because my only interaction with QHexCursor was switching its mode. Though what is definitely needed is a clearer idea of what is supposed to belong where: what to QHexView itself and what to QHexDocument. The way I see it - something general affecting the whole widget's appearance and workflow - it is QHexView's domain, anything else that deals with particular binary data set is up to QHexDocument to work with.

Edit: I forgot to mention that I can confirm that QHexView::setAutoWidth is working correctly now.

@Dax89
Copy link
Owner Author

Dax89 commented Mar 6, 2022

I'm afraid not. To reproduce, create an empty QHexView, and use mouse wheel to scroll down. Despite having no data and without a visible scrollbar it will still scroll down to offset 0x10.

Got it, fixed.

Maybe, although I can not say much here because my only interaction with QHexCursor was switching its mode. Though what is definitely needed is a clearer idea of what is supposed to belong where: what to QHexView itself and what to QHexDocument. The way I see it - something general affecting the whole widget's appearance and workflow - it is QHexView's domain, anything else that deals with particular binary data set is up to QHexDocument to work with.

Ok, I will do this improvement (if there aren't any pending issue, so it's easier to catch regressions).

@T-640
Copy link

T-640 commented Mar 6, 2022

Got it, fixed.

Still acting same unfortunately.

Ok, I will do this improvement (if there aren't any pending issue...

That one with this invisible "scrollbar" should be the last bug reported here, I think all the rest are fixed now. I have got a few more secondary things to discuss, but indeed, I will postpone posting them not to rush things too much, let's deal with this last bug and that improvement we discussed earlier first.

@Dax89
Copy link
Owner Author

Dax89 commented Mar 7, 2022

Still acting same unfortunately.

I can't reproduce it anymore with the case you have described.
Btw, I have pushed another commit which disables the vertical scroll entirely if the scrollbar is not visible, check now!

EDIT:
I've reorganized API and class relations in this way:

  • QHexView: Owns QHexCursor, QHexMetadata and QHexOptions
  • QHexCursor: Keep che cursor position and insert/overwrite state, it's always owned by a single QHexView
  • QHexDocument: Has the internal buffer, editing and undo/redo information (now it can be attached to multiple QHexView)

The readme with the sample code is updated to reflect these changes.

(It's possible that I have created regressions with this change!)

@T-640
Copy link

T-640 commented Mar 7, 2022

That last bug is gone, everything works like a charm! At least in the subset of QHexView features that I am actively using no regressions were spotted.

The new API is more high level, doing a good job at hiding implementation details: I am now able to setup everything I need without having to access QHexMetadata, QHexCursor pointers, and my subclass of QHexView now uses less code and is more readable.

The new screenshot is very nice, gives an excellent first look impression, though it would probably look even better with setAutoWidth(true) called.

Now that everything appears to be stable, the old features restored, if I am not bothering too much with suggestions, I would like to discuss a few more secondary things, I will post my thoughts a bit later.

@Dax89
Copy link
Owner Author

Dax89 commented Mar 7, 2022

Good!

Yes, I'm open to listen for more suggestions/fixes/improvements

@T-640
Copy link

T-640 commented Mar 7, 2022

Here are a few more things:

  1. Resetting data of QHexDocument

Currently whenever I need to reset the widget with a new QByteArray I create a fresh QHexDocument with the new data and call QHexView::setDocument. Is this the right approach, or perhaps it is an overkill to create a whole new document just to reset data? Would having a slot QHexView::setData(QByteArray) be a better idea? In this case some other things might need to be taken care of though, like resetting metadata.

  1. Notifying when data is manually edited

This has been described in #58. Whenever I manually edit binary data I need to sent it back from QHexView to wherever it came from originally. Currently I have to filter out whether data was actually changed or whether it just were other unrelated things like metadata.

  1. Header

3.1 The first column, the Address one, is empty. Let's put it to some good use: let's display the number of byte that is currently being selected by cursor, e. g. 5th, 10th, 20th, etc, in decimal system.

Position and override

I think somewhere here you could query for the current byte at cursor position and and compose the final "Pos: xxx" string with it. And also maybe instead of rightJustified there should be an option to justify at centre or to the left (and for the ASCII label too).

QString addressheader = m_options.addresslabel.rightJustified(this->addressWidth()), hexheader;

3.2 I have just found a solution for #59 that does not require any code changes to work, but I need a small cosmetic change. When separators are enabled, could you make it so that the last line is not drawn? Optionally, although there may be no reason to have it in the first place, since there is nothing to separate ASCII area from the right.

Line crosses widget

The last vertical separator line crosses the QComboBox, so I would like it to be removed.

3.3 And about header style: QHexView in its appearance has some similarity with QTreeWidget and QTableWidget: they all have a header. Perhaps QHexView's header could borrow the default style of headers of these widgets (optionally, of course, there is nothing wrong with the current style). I am only asking because I have a QTreeWidget next to the QHexView in my program, and them having same header style would make appearance more consistent.

@Dax89
Copy link
Owner Author

Dax89 commented Mar 9, 2023

I can take a look this weekend.

Yes, I can push 5.0 to master branch after these fixes.

@T-640
Copy link

T-640 commented Mar 10, 2023

And one other little thing: suppose there is a need to manually copy some byte sequence from the hex view into, let's say, some place in a C++ source code file.

Currently the copied result is written like this:

15 6D 6F B5 0E 50 04 38 B0 82 51 17 C5 20 3C 25

This needs manual adjustment in order to be properly introduced within the C++ source code.

What about introducing a way for QHexView to do this automatically (via specific QActions I suppose, or some other option which controls what CTRL + C produces by default). That is, outputting this:

{0x15, 0x6D, 0x6F, 0xB5 ...} or this "\x15\x6D\x6F\xB5 ..."

And maybe some other notations if they are popular enough. For those who need some very specific output suitable for their particular needs there should be a way to customise this process by doing something like adding some custom QAction or function to convert these bytes into a custom string.

And finally the user needs to be given control over whether this string created via the CTRL + C combination will occupy a single line or multiple lines just like in the QHexView itself.

@Dax89
Copy link
Owner Author

Dax89 commented Mar 12, 2023

Ok so:

  • std::isprint() is replaced with QChar::isPrint()
  • I have added QHexView::copyAs(CopyMode mode) slot which copies data in various formats:
    • QHexView::CopyMode::HexArrayChar: data is copied as "\x15\x6D\x6F\xB5 ..."
    • QHexView::CopyMode::HexArraySquare: data is copied as [0x15, 0x6D, 0x6F, 0xB5 ...]
    • QHexView::CopyMode::HexArrayCurly: data is copied as {0x15, 0x6D, 0x6F, 0xB5 ...}

Also earlier we were talking about the possibility of introducibg QHexView::fullDataChanged signal as an optional alternative to the QHexView::dataChanged one, but now I am not that sure if it would be that useful, at least I myself in my current project find the granular approach of QHexView::dataChanged superior. Although perhaps someone else could find it useful, so if it does not mess up the design it could still be optionally included.

Ok, I prefer to keep only QHexView::dataChanged signal for now.
A "possible workaround" is to ignore the signal's arguments and extract the internal buffer with QHexDocument (it depends how the bytes are loaded though)

And finally the user needs to be given control over whether this string created via the CTRL + C combination will occupy a single line or multiple lines just like in the QHexView itself.

Mmmmh, for example?

@T-640
Copy link

T-640 commented Mar 13, 2023

For example, suppose we need to copy a large byte sequence into a C++ file, let's say a sequence of 128 bytes. With curly braces notation this would take 6 characters per byte, which would result in a very long string occupying a single line of code, looking very ugly in the editor.

I suggest inserting a new line character after every 16th byte (and perhaps even give an option to customise that, could be a parameter in QHexOptions, or a second optional parameter of QHexView::copyAs). With that, in the example above, instead of a single long line we have 8 short ones which makes it much more readable.

And I suppose this feature of inserting new line characters should itself be optional (a boolean value in QHexOptions I suppose). For example, if 17 bytes need to be copied instead of 128 we would not want to reserve a whole new line just for that last byte. Also we can never know where exactly the byte string will be pasted and what are the requirements of the software or file format where it gets pasted, so we can never assume which is the right choice there. However, I think for CopyMode::Visual the default result should be a single line string, and multi line string for others.

@Dax89
Copy link
Owner Author

Dax89 commented Mar 16, 2023

I have added QHexOptions::copybreak (default true) which breaks at QHexOptions::linelength.

CopyMode::Visual behavior is now changed: it copies data as they appears in the hex part.
For example, if QHexOptions::grouplength is 4, data is copied as:

0x61736461, 0x73646173, 0x64206173, 0x64613334, 
0x35617364, 0x730A

@T-640
Copy link

T-640 commented Mar 17, 2023

I checked it out, working smoothly. A few comments:

  • QHexView::copyAs has two unused i variables:

QHexView/qhexview.cpp

Lines 261 to 263 in 5105af5

case CopyMode::HexArrayChar: {
QString hexchar;
int i = 0;

QHexView/qhexview.cpp

Lines 272 to 274 in 5105af5

default: {
QString hexchar;
int i = 0;

  • Perhaps the CopyMode::HexArrayChar and the regular QHexView::copy function could also benefit from the QHexOptions::linelength option for line breaks?

  • There is one more hex notation that I am aware of, in Common Lisp byte vectors are created this way:
    #(#xFF #xFF #xFF #xFF). Perhaps QHexView::copyAs could be made more generic to allow a wider range of customisation. For instance, two extra options could be added in QHexOptions, like this:

    QString hex_prepend = "0x"
    bool add_comma = true or even better QString hex_separator = ", "

    And I suppose since we already have got the CopyMode::HexArrayCurly and CopyMode::HexArraySquare why not add the third bracket type, which would be the CopyMode::HexArrayParentheses. In fact these three enumerations could possibly themselves be moved to QHexOptions, and the QHexView::CopyMode would need to contain only thee values - Visual as default, WithBrackets to wrap the result of Visual in brackets, and HexArrayChar as special case. In that case, perhaps CopyMode::Visual could be renamed to CopyMode::Default?

    All that combined would make it possible to customise the copy output for pretty much any programming language.

    To sum up, this is what the code could possibly look like:

class QHexView : public QAbstractScrollArea
{
    enum class CopyMode { Default, WithBrackets, HexArrayChar };
};

struct QHexOptions
{
    struct CopyModeParameters
    {
        enum class BracketType { Curly, Square, Parentheses };
        BracketType bracket = BracketType::Curly;
        QString prepend = "0x";

        QString separator = ", ";

        // Or this could be even better instead:

        QChar separator = ',';
        bool add_space_after_separator = true;
    };
};
  • I have noticed that when QHexView contains no data, the cursors are still drawn along with the 00000000 position indicator. I think it would look nicer if it does not happen.

@T-640
Copy link

T-640 commented Mar 17, 2023

In addition to my recent comment above here is an update about the bug mentioned in #71 (comment)

It seems to be unrelated to scrollbars, at least to the horizontal one definitely. In the screenshot below I am unable to scroll down any further and the last line containing four FF bytes can not be fully seen. If I slightly adjust the widget's height I am able to fully see it again. If I keep changing height this issue comes and goes at various height values.

Last line obscured

@Dax89
Copy link
Owner Author

Dax89 commented Mar 20, 2023

The last commit seems to fix this issue (on linux).
Now visible lines are calculated with the viewport's height instead of the widget one

I see in the next days this one: #71 (comment)

@T-640
Copy link

T-640 commented Mar 21, 2023

This has improved the situation but still has not fully fixed it yet though.

The issue happens if high DPI mode is enabled, that is QApplication::setAttribute (Qt::AA_EnableHighDpiScaling) is called. The resolution of display I work with is 2560 × 1600. I work under Windows but this might be irrelevant in this case.

If QApplication::setAttribute (Qt::AA_EnableHighDpiScaling) is not called on this system the widget works fine. Unfortunately setting this attribute is mandatory because without it many widgets look incorrectly on displays with high resolution.

Here is what I can tell so far: I populate widget with enough data to trigger the vertical scrollbar appearance, choose a certain height, scroll all the way down and then carefully drag the height slider (not the scrollbar) upwards and observe what happens to the last line:

First, this is the intended look of it as far as I understand:

Normal

I keep extending height and then some extra space appears (is this intentional?) :

Extra space

At some point this extra space abruptly disappears and this is where the issue occurs. Sometimes it may be just a few pixels that are hidden, sometimes noticeably more. Not sure if there is any correlation but it seemed the more I increased the widget's height the more pixels of the last line were obscured.

Partially obscured

The cycle repeats as I keep increasing the height of widget.

One a side note, the issue that was fixed when std::isprint() was replaced with QChar::isPrint() was related to high DPI mode as well, but in this case it appears to be a bug in Qt itself (at least version 5.14.2) and only on Windows platform in particular. This CreateFontFaceFromHDC() failed error occurs every time Qt attempts to draw non-printable symbols on Windows system in High DPI mode. I have observed it when some non-printable characters ended up going into QPlainTextEdit and when simply selecting some fonts from the QFontDialog::getFont call. So this is the right decision to replace all non-printable characters with dots, make sure it stays that way.

@T-640
Copy link

T-640 commented Mar 29, 2023

It appears I have managed to find a solution to this issue. Although it has been awhile since I had done any math, let alone math related to GUI, so please make sure to carefully check this in case you incorporate it.

The vertical scrollbar is the cause of the problem: in some cases as shown above it does not have enough range, making it impossible to scroll down to fully see the last line.

The fix can be as simple as to always add another +1 to vscrollmax somewhere in QHexView::checkState, before calling the setRange of the scrollbar:

QHexView/qhexview.cpp

Lines 406 to 413 in 67dc02e

void QHexView::checkState()
{
if(!m_hexdocument) return;
this->checkOptions();
int doclines = static_cast<int>(this->lines()), vislines = this->visibleLines(true);
qint64 vscrollmax = doclines - vislines;
if(doclines >= vislines) vscrollmax++;

I have tried improving it by inserting this after the code above:

int char_height = QFontMetrics(font()).height();
div_t division = std::div (viewport()->height(), char_height);
if (division.rem > (char_height / 2))
    vscrollmax++;

With that the last line now is never obscured when the vertical scrollbar is active. It may still happen if vertical scrollbar is not active though - when widget is too small to show all the contents but the activation of scrollbar has not kicked in yet. So probably some fine-tuning is still needed to calculate more precicely when exactly the vertical scrollbar should become active (decimal fractions instead of integers maybe, but don't take my word for it).

One other minor cosmetic issue due to this is that this extra occasional white space after the last line I mentioned earlier sometimes becomes even larger.

Extra space

These issues are negligible though, so if everything proves to be fine in your environment I think this fix should be added. I tested it both with and without the High DPI mode enabled and it does not seem to break anything. Perhaps you could try fine-tuning it and see what comes out.

@Dax89
Copy link
Owner Author

Dax89 commented Mar 30, 2023

If a +1 improves the situation it can be a rounding issue where the line's height is calculated, maybe a qCeil() fixes that.

@T-640
Copy link

T-640 commented Apr 1, 2023

Probably, although I tried some floating point arithmetic which have not made much difference compared to the integer solution above, but I made no thorough research there. I suppose the extra challenge is the fact that viewport's height and the number of scrollbar steps are integer values.

One thing I can tell for sure is that the High DPI mode is very sensitive to precision. Recently I have discovered yet another issue with it - I have noticed some misalignment of text in QPlainTextEdit despite setting a monospace font. Further investigation showed that 4 spaces were not equal in width to 1 tab. As usual the widget worked fine when this mode was disabled.

As it turned out, the default tab width value was an integer value of 32 and had to be recalculated with the help of QFontMetricsF to become 31.xxxx.

I see that you are already using QFontMetricsF and qreal a lot, but perhaps there is still some place where integers are used instead, which is causing the issue?

@arabine
Copy link

arabine commented Apr 4, 2023

Ok wonderful branch, great job! The minimal changes to build against the Qt6 library is to remove QStringRef and .midRed().

I would suggest, if you plan to keep Qt5 compatibility, is to replace with QStringView maybe?

@Dax89
Copy link
Owner Author

Dax89 commented Apr 5, 2023

@arabine
I have added some shims in order to support both Qt5 and 6 with minimal changes, CMake is also updated: now it looks for Qt6 first, otherwise it fallbacks to Qt5.

Tested in a Linux system with:

  • Qt 6.4.3
  • Qt 5.15.8

@T-640

I see that you are already using QFontMetricsF and qreal a lot, but perhaps there is still some place where integers are used instead, which is causing the issue?

Maybe, I will check if there are some integer calculations in the code

@arabine
Copy link

arabine commented Apr 11, 2023

Ok seems good to me. QStringView is available since Qt 5.10 so maybe you can use it for all the versions (5.10 is old enough I believe).

I have a question: is there a way to refresh the view when the memory content has changed? My use case is an emulator, I want to view the RAM content (I can call a method to force the refreshing if needed).

@T-640
Copy link

T-640 commented Apr 12, 2023

5.10 is old enough I believe

The goal is to retain compatibility with the version 5.6.3 though, according to this discussion - #69

@NikolajSchlej
Copy link

Had to make some trivial changes to make the widget buildable with vc141_xp target in Qt 5.6.3:

  • replacing std::isxdigit with isxdigit from <cctype>
  • replacing QString.first() and QString.last() calls with at(0) and at(1), as that string is 2 characters long.

Switched to this widget because it natively supports UI dark mode and overall looks much nicer than QHexEdit2 I've used before in UEFITool: LongSoft/UEFITool@cba31d8

Huge thanks for making it, and for using MIT license.

Minor feature ideas (might implement some and send PRs later):

  • STATIC_READ_ONLY option that ifdefs all editing-related code away
  • similar static options to disable unused features, i.e. large file support, editable memory buffers, commands, etc.

@NikolajSchlej
Copy link

Dark mode in Linux with Qt 6.2.4, global scaling 200% on 5K display. Works really well!
Screenshot_20230423_031150

@NikolajSchlej
Copy link

A minor bug to report: macOS starting with 10.7 has scrollbars hidden by default and shown only when scrolling is performed. Had to make this modification to restore the expected behavior there:

void QHexView::wheelEvent(QWheelEvent* e)
 {
     e->ignore();
 #if defined Q_OS_OSX
     // In macOS scrollbar invisibility should not prevent scrolling from working
     if(!m_hexdocument) return;
 #else
     if(!m_hexdocument || !this->verticalScrollBar()->isVisible()) return;
 #endif

@NikolajSchlej
Copy link

Also, the default font in macOS is somehow not a monospace one, so I had to manually call setFont() to set it back to sanity. This is how it looks in macOS 13.3.1 on the same 5K screen.
Screenshot 2023-04-23 at 16 11 37
Screenshot 2023-04-23 at 16 12 03

@T-640
Copy link

T-640 commented Apr 24, 2023

@NikolajSchlej,

I see that you happen to be in possession of a high resolution monitor. There is a bug related to these. Could you confirm that it happens on your Linux and Macintosh systems as well?

The bug is described here - #71 (comment). Make sure to call QApplication::setAttribute (Qt::AA_EnableHighDpiScaling) in advance before the QApplication instantiation and pay close attention to the last line, as the glitch may be hard to spot. A somewhat crude workaround can be found here - #71 (comment).

The reason behind it is yet to be determined, so any assistance would be appreciated.

@Dax89
Copy link
Owner Author

Dax89 commented Apr 24, 2023

@NikolajSchlej

replacing std::isxdigit with isxdigit from

I have done an internal implementation (implemented as QHexUtils::isHex()) so we'll avoid compiler shenanigans entirely!

replacing QString.first() and QString.last() calls with at(0) and at(1), as that string is 2 characters long.

I have found a front() and back(): both replaced replaced with at() .

A minor bug to report: macOS starting with 10.7 has scrollbars hidden by default and shown only when scrolling is performed.

Fix done as you suggested

Also, the default font in macOS is somehow not a monospace one, so I had to manually call setFont() to set it back to sanity

Is there some way to set a monospace font for MacOS?

There is a conditional statement that may create some issues, this is the relative code:
https://github.com/Dax89/QHexView/blob/5.0/qhexview.cpp#L36-L40

@Dax89
Copy link
Owner Author

Dax89 commented Apr 24, 2023

@arabine

I have a question: is there a way to refresh the view when the memory content has changed? My use case is an emulator, I want to view the RAM content (I can call a method to force the refreshing if needed).

There is QHexDocument::documentChanged() signal which is connected automatically to the view and it redraws the data when notified.

If data is changed externally I think it's sufficient to trigger an update manually with Qt's update() method when needed

@NikolajSchlej
Copy link

@Dax89, not really a fan of platform-specific hacks, and it feels like a Qt-issue more than a QHexEdit-issue, but I'll investigate those font shenanigans further.

@T-640, I don't use the flag you mentioned because UEFITool is only using Qt 5 for a single legacy platform (x86 Windows XP and newer), with explicit refusal to support HiDPI on anything that is not built against Qt 6. As mentioned on https://doc.qt.io/qt-6/highdpi.html, "Qt 5 behavior assumes that AA_EnableHighDpiScaling has been set (this flag is not needed on Qt 6)".
Will still try to reproduce it on supported configurations later.

@NikolajSchlej
Copy link

NikolajSchlej commented Apr 24, 2023

@Dax89, @T-640, looks like the issue is reproducible even with Qt 6.5.0 and without the explicit HiDPI flag, just resizing the window with nothing but QHexView widget slowly, a moment can be caught where the next line is already drawn (so now about a half of line is visible), but the scroll area is not yet updated (so it can't be scrolled to become fully visible). The window can then be resized back a tiny bit, making the visible potion even smaller. Resizing further back or forward fixes it, but it can be repeated indefinitely.

Screenshot_20230424_112306

@arabine
Copy link

arabine commented Apr 26, 2023

@Dax89

There is QHexDocument::documentChanged() signal which is connected automatically to the view and it redraws the data when notified.

If data is changed externally I think it's sufficient to trigger an update manually with Qt's update() method when needed

Do you have an example? Because in this case, I think I have to create a document with a reference to the data (without copying it on each data update).
It would be worth to have it, it would complete the README examples provided (the examples show only the use case when the user can change the data from the view).

@Dax89
Copy link
Owner Author

Dax89 commented Apr 28, 2023

I suppose that the RAM is just a buffer of bytes updated by a virtual CPU, outside the control of the widget.

In this case I think QMemoryRefBuffer is what you need, because it takes an external buffer without copying it.
Note that QMemoryRefBuffer have deletion and insertion disabled, but you can edit its content.

For example:

#include <QHexView/model/buffer/qmemoryrefbuffer.h>
#include <array>

static std::array<char, 8> BUFFER = {
    'm', 'y', 'b', 'u', 'f', 'f', 'e', 'r'
};

// QByteArray is also accepted
QHexDocument* hexdocument = QHexDocument::fromMemory<QMemoryRefBuffer>(BUFFER.data(), BUFFER.size());
hexview->setDocument(hexdocument);

If you want something even more specific you can also roll your own model inheriting from QHexBuffer class.

@caffedrine
Copy link

Guys, is there any way to scroll to a highlighted area? I tried manually method QHexView::scroll(0, 20) and it does produce a really weird output. It seems the whole area is shifted down:
image

@Dax89
Copy link
Owner Author

Dax89 commented Sep 10, 2023

The widget scrolls automatically if the cursor (or selection) is outside of visibile range, otherwise it doesn't do anything.

The method scroll() is inherited from QAbstractScrollView and shouldn't be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants