QHexView 5.1 Development

[Dax89]

5.1 development topic

[T-640]

So let's begin with what we've previously discussed about QHexOptions, QHexcharFormat and QHexDelegate. When it's done I'll go through the old 5.0 discussion to see if anything from there is missing and through my notes I've made since then. I'll post suggestions piecemeal one at a time. I don't remember encountering any more bugs, so I think it'll all be about feature requests.

[Dax89]

The last commit introduces some little breaking changes:

• Dropped QHexColor in favour of QHexCharFormat
• Refactored QHexView::drawHeader
• Added more fields in QHexOptions:
  • headerformat which replaces headercolor
  • addressheaderformat which is related to the address part's style of the header
  • hexheaderformat same as above but for hex part
  • asciiheaderformat same as above but for ascii part
• As requested, backgrounds now uses QBrush instead of QColor
• There are some changes in QHexDelegate too:
  • Removed renderHeader
  • Removed renderHeaderPart
  • Renamed render as renderByte
• QHexMetadata now uses QHexCharFormat too. I have made some tests and it looks fine, but I need to do deeper tests for regressions

Here are more area of improvements:

• Look for deeper QHexCharFormat integration (when and if needed)
• I'm thinking to drop paintSeparator and paint functions from QHexDelegate, because:
  • Probably the separator can be styled from options, there is no need to be dynamic or heavily customized, it's just a separator
  • paint is too free and overkill, by removing it also solves the concerns about wild user code that does clipping in delegate

[T-640]

As requested, backgrounds now uses QBrush instead of QColor

As I mentioned earlier my intention for QBrush was to take advantage of gradients without using QHexDelegate::paint, but turns out I have forgotten that they require coordinates to be properly drawn, and thus simply putting a QGradient subclass into QBrush and calling it a day is not enough. Fortunately I found a solution, below is the explanation of the issue and how to handle it.

This is how I painted gradient before:

virtual void MyDelegate::paint (QPainter* painter, const QHexView* hex_view) const override
{
    QLinearGradient gradient (0, 0, 0, headerRect().height());
    gradient.setColorAt (0.1, hex_view->palette().color (QPalette::Light));
    gradient.setColorAt (1,   hex_view->palette().color (QPalette::Midlight));
    painter->fillRect (hex_view->headerRect(), gradient);
    QHexDelegate::paint (painter, hex_view);
}

This is how I setup gradient now:

QHexOptions options;
QLinearGradient gradient;
gradient.setColorAt (0.1, hex_view->palette().color (QPalette::Light));
gradient.setColorAt (1,   hex_view->palette().color (QPalette::Midlight));
options.headerformat.background = gradient;
hex_view->setOptions (options);

The issue is with this line:

QLinearGradient gradient (0, 0, 0, headerRect().height());

In order to paint the gradient we need to know the start and stop points. The default-constructed QLinearGradient is pretty much useless unless these points are set up later. The start point we know - for header it is always 0, 0. But the stop point is variable, since it relies on header height, which might change if the font is changed.

The solution I came up with is adding a few lines of code to QHexView::drawHeader, right after the m_options.headerformat copy is made here:

QHexView/src/qhexview.cpp

Lines 649 to 656 in 6ecffde

	void QHexView::drawHeader(PaintContext* ctx) const {
	if(m_options.hasFlag(QHexFlags::NoHeader))
	return;
	ctx->painter->setClipRect(this->headerRect());
	ctx->y = 0; // Move to top
	QHexCharFormat hcf = m_options.headerformat;

Add this after:

if (hcf.background.gradient())
{
    QGradient* gradient = const_cast <QGradient*> (hcf.background.gradient());
    if (gradient->type() == QGradient::LinearGradient)
    {
        QLinearGradient* linear_gradient = reinterpret_cast <QLinearGradient*> (gradient);
        linear_gradient->setStart (0, 0);
        linear_gradient->setFinalStop (0, headerRect().height());
    } // Should something be done with other gradient types as well?
}

This is a bit unpleasant as QHexView::drawHeader now has to take care of a particular case and possibly of other gradient cases, but on the other hand it's just a few lines of code so far and there is no need for the "heavy artillery" QHexDelegate::paint to just render a simple gradient, so from the user's point of view this is an improvement.

Added more fields in QHexOptions

As I have shown above I am using only headerformat at the moment, but I see how other options can be useful, so in general, the more QHexOptions the merrier. The naming convention could probably be adjusted though, these other XXXformat members consist of three words, I'd say a snake case could be used to separate them to make these members more readable.

QHexMetadata now uses QHexCharFormat too. I have made some tests and it looks fine, but I need to do deeper tests for regressions

I haven't used QHexMetadata yet, so I don't think I can be of much help there. Doesn't necessarily mean I will never use it in the future, just haven't explored the full set of QHexView's API yet, being focused on the things we have been discussing thus far. But if you need some samples to be run on my Windows high DPI system to check for bugs I'd be happy to help.

Look for deeper QHexCharFormat integration (when and if needed)

Can't say much about this, it fits great in QHexOptions, other API I have not explored that much to suggest anything sensible.

Now, on to what to do with QHexDelegate:

Probably the separator can be styled from options, there is no need to be dynamic or heavily customized, it's just a separator

Good point, a line is, well, a line, it doesn't seem much can be done with it. There is, nonetheless, one case that needs to be addressed should QHexDelegate::paintSeparator be removed. In particular, I am using it to call qDrawShadeLine. This is how it looks:

bool MyDelegate::paintSeparator (QPainter* painter, QLineF line, const QHexView* hex_view) const override
{
    qDrawShadeLine (painter, line.p1().toPoint(), line.p2().toPoint(), hex_view->palette());
    return true;
}

To call qDrawShadeLine is all what I am using QHexDelegate::paintSeparator for. So if you can add some members to QHexOptions that result in a call to this Qt function I think there would probably be no need for QHexDelegate::paintSeparator anymore. Beware that qDrawShadeLine takes 3 optional parameters, sunken, lineWidth and midLineWidth, so they would need to be added to QHexOptions as well. So I'd say if you can fit all this functionality elegantly into QHexOptions and painting API then QHexDelegate::paintSeparator can be removed, if not just let it be.

As for QHexDelegate::paint:

... by removing it also solves the concerns about wild user code that does clipping in delegate

I wouldn't worry too much about that, I mentioned that one extremely unlikely case when user can mess things up simply because I try to take everything into account, no matter how insignificant it is, but how much attention should be paid for it is a whole different matter. And I think that issue can be taken out entirely if you use QPainter::save and QPainter::restore instead of just erasing the clipping. I don't remember the last time I have used these two functions, if ever, but using them instead of calling QPainter::setClipRect to set an empty clipping rectangle visually makes no difference, it seems these two functions were made exactly for the case we are talking about. We save the painter's state with whatever possible clipping the user has made (I think it is supposed to do that), do our own clipping and then simply roll back to the previous state. This is at least how I understand it.

There is one more thing I am using QHexDelegate::paint for: to leverage the QStyle API in order to paint the header's background. That way QHexView's style is consistent with other widgets. QTreeView in particular, as I have it right next to QHexView, and seeing both their headers being painted in a uniform way is nice. To clear the possible confusion, I paint gradient on Windows 10 and later (because my goodness, have they butchered the style!), but use QStyle API to paint header on earlier versions of the OS. So this is how I use it (the code is a bit outdated but you get the idea):

virtual void paint (QPainter* painter, const QHexView* hex_view) const override
{
    static bool old_windows = QOperatingSystemVersion::current().majorVersion() <
                              QOperatingSystemVersion::Windows10.majorVersion();
    if (old_windows)
    {
        QStyleOptionHeader headerStyleOption;
        headerStyleOption.rect = QRect (0, 0, hex_view->width(), QFontMetrics(hex_view->font()).height());
        hex_view->style()->drawControl (QStyle::CE_HeaderSection, &headerStyleOption, painter, hex_view);
    }
            
    QHexDelegate::paint (painter, hex_view);
}

This, however, can be added as another of QHexFlags. Could be something like HeaderUsesQStyle. Could even be the default choice. Just make sure that QHexOptions::headerformat or QHexDelegate do not override it unexpectedly or vice versa.

If everything that I have mentioned above is implemented, I personally would have no need for neither QHexDelegate::paint nor QHexDelegate::paintSeparator. However, should they really be gone? QHexDelegate::paintSeparator maybe, but about QHexDelegate::paint I have doubts. It gives the user an easy access to rendering QHexView without the need to look into the source code. As you can see I have used it for variety of experiments with style, some of which now made it into the QHexOptions, and others may be if you find them useful. Without QHexDelegate::paint I wouldn't be able to play around with style and make these suggestions (well, technically I guess I could have subclassed and overriden the paint event, but it is a bit less convenient). Somebody else may do their experiments with QHexDelegate::paint and come up with suggestions we both couldn't even think of.

And if you are concerned that with too much access the user might mess up something else, don't be. That's the whole point of delegates, to boldly override where no man has overriden before, isn't it? If I screw up there with delegate it's on me, I know the risks as a user. I personally take advantage of Qt's own delegates and QProxyStyle significantly. An experiment involving a simple override of QToolButton's paint event later resulted in a whole QStyle plugin I made. So to sum up, I use the the default API of Qt widgets when getting started with something new, but once in a while have a creative inspiration and start testing various ideas, and the power and freedom that delegates and QStyle provide is very helpful there, But as I mentioned earlier, overriding the paint event would probably suffice for everything I've said, but if QHexDelegate::paint is not much of a burden to maintain I'd probably keep it. QHexDelegate::paintSeparator is the true candidate for removal, but even there, despite how unlikely it could be, what if someone comes up with some original styling choice there and wants to use this function to implement that? For instance I personally like thicker QSplitter separators, what if someone would want thicker separators for QHexView, and more intersting things could be done to them in that case since more space to draw is available.

I am probably wandering too much into the "what if" territory there, talking beyond the funcionality I already use the delegates for. When I am not sure whether to remove something or not, I usually leave everything as is. But I am talking from the user perspective here, for you probably the most important question would be "will it be much of a burden to maintain these two functions?"

And finally, if you decide to keep QHexDelegate::paint there is a small regression that needs to be fixed. Basically there is a case when QHexOptions overrides everything that QHexDelegate::paint does to the header. It comes down to this:

hex_view->setDelegate (some_delegate_that_paints_header);
hex_view->setOptions (any_options_there); // This is what messes things up

QHexView::setOptions calls QHexView::checkAndUpdate which calls QHexView::checkState which finally calls the culprit - QHexView::checkOptions

This is the part of QHexView::checkOptions that is causing the issue:

QHexView/src/qhexview.cpp

Lines 525 to 528 in 6ecffde

	if(m_options.headerformat.background == Qt::NoBrush) {
	m_options.headerformat.background =
	this->palette().brush(QPalette::Base);
	}

If we pass QHexOptions with default-constructed headerformat.background into QHexView::setOptions, it eventually calls QHexView::checkOptions which overrides background with QPalette::Base.

Now background is no longer Qt::NoBrush which unexpectedly affects QHexView::drawHeader here:

QHexView/src/qhexview.cpp

Lines 660 to 661 in 6ecffde

	if(hcf.background != Qt::NoBrush) // Draw background directly
	ctx->painter->fillRect(this->headerRect(), hcf.background);

[Dax89]

• I have added gradient support for header's background brush, usually I avoid const_cast because it can lead to UB, but ironically, it's even used in Qt codebase: https://codebrowser.dev/qt6/qtbase/src/widgets/widgets/qplaintextedit.cpp.html#1874 so looks good to me too.

• I have removed this part in QHexView::checkOptions, it's a refactoring leftover from QColor => QBrush transition, so a default initialized QBrush is a still valid setting:

 if(m_options.headerformat.background == Qt::NoBrush) { 
     m_options.headerformat.background = 
         this->palette().brush(QPalette::Base); 
 } 

• We can keep QHexDelegate::paintSeparator and QHexDelegate::paint for now, it was just a bold idea!

• I have also refactored QHexOptions with snake case as proposed, so they are easier to read them, also linealternate_ becomes linealt_:
  

  QHexView/include/QHexView/model/qhexoptions.h

  Lines 33 to 63 in 71f76cd

  	struct QHexOptions {
  	// Appearance
  	QChar unprintablechar{'.'};
  	QChar invalidchar{'?'};
  	QString addresslabel{""};
  	QString hexlabel;
  	QString asciilabel;
  	quint64 baseaddress{0};
  	unsigned int flags{QHexFlags::None};
  	unsigned int linelength{0x10};
  	unsigned int addresswidth{0};
  	unsigned int grouplength{1};
  	int scrollsteps{1};
  	// Colors & Styles
  	QHash<quint8, QHexCharFormat> bytecolors;
  	QColor linealt_background;
  	QColor line_background;
  	QHexCharFormat header_format;
  	QHexCharFormat address_format;
  	QHexCharFormat addressheader_format;
  	QHexCharFormat hexheader_format;
  	QHexCharFormat asciiheader_format;
  	QColor comment_color;
  	QColor separator_color;
  	// Misc
  	bool copybreak{true};
  	inline bool hasFlag(unsigned int flag) const { return flags & flag; }
  	};

[T-640]

... usually I avoid const_cast ...

Me too, every time this pops up in my code it likely means that something ain't right, I was hesitant myself but could not come up with any other solution. In this case, however, it looks like the issue is with Qt's own API, not much we can do about that. Normally I'd expect it to have two methods - QBrush::gradient and QBrush::constGradient, just like there is QByteArray::data and QByteArray::constData, but alas there is only a method that returns a const gradient. Technically there should be nothing preventing us from modyfing the underlying gradient, as it is a copyable object after all, but I guess they don't want it to be modified via pointer due to internally employing the "Copy-on-write" technique, or as they call it, "Implicit Sharing". Because as far as I understand, due to inherent issues with the "Copy-on-write", accessing internal data via pointers may result in unwanted changes in objects from which the copy is made. In our case this is unlikely to be an issue, we simply create QHexOptions with all the QBrushes on the stack, pass them to QHexView, and then the original QHexOptions is gone when the function exits, so no problem there, Well, technically somebody might use it in some other unusual way, but as long as it does not cause a crash (seems like it shouldn't in our case) it could be considered as acceptable.

As you have shown they tend to violate these principles even in their own code, and in the case of QByteArray even provide a non-constant pointer to data. I guess this is because they became somewhat trapped by that decision to implement COW made a long time ago. The C++ standard itself had it back in the 90's as far as I understand, at least for strings, and it took them more than a decade to remove this in C++11. But Qt didn't, and we've got what we've got.

We can keep QHexDelegate::paintSeparator and QHexDelegate::paint for now, it was just a bold idea!

Don't mind that large wall of text I have written, I just sometimes get carried away and go deep into details. I myself always have that nagging desire to remove various things, constantly questioning whether I should keep something or not.

I mentioned I was using QStyle::drawControl with QStyle::CE_HeaderSection for header painting and qDrawShadeLine for separator painting and how they could be added to QHexOptions, what do you think about that? They are more specific than simple QColors and QBrushes, but still seem to be generic enough to be there. On the other hand adding them might result in code bloat if they don't fit elegantly, and also priority between them, simple QBrushes and delegates will have to be considered. Can't say for sure, you have better judgement regarding whether more sophisticated things like that belong internally in QHexView's source code or whether it is up to the delegates to take care of this. And alternatively a third option between implementing or not implementing that could be me adding this small delegate I already use as an optional feature in some separate header to the project. This optional sucblass of QHexDelegate could be called something like QAdvancedHexDelegate and won't require any internal changes in QHexView.

[Dax89]

I mentioned I was using QStyle::drawControl with QStyle::CE_HeaderSection for header painting and qDrawShadeLine for separator painting and how they could be added to QHexOptions, what do you think about that?

Probably it's a better idea to leave this part as it is: if someone wants to use qDrawShadeLine it creates a custom delegate and overrides paintSeparator.

[T-640]

Alright, things are pretty good as they are now. We've got QHexView taking care of simple colors and brushes on its own and delegates for more complex cases. They are not getting in the way of each other, and there is a clear separation of labor between them, plus it mirrors the Qt's own approach to customization. If it ain't broke, don't fix it after all. And one more reason to keep the delegates: back in the "reverse painting" thread you mentioned considering splitting one of the QHexDelegate's functions into the "before" and "after" parts. With QHexDelegate::paint there is no need for that, as the user is in full control of when to call the base class'es version of this virtual function. So this whole "before" and "after" is entirely the user's concern and responsibility to take care of, you don't need to worry about that.

It seems like we've covered everything mentioned before, shall we move on to the next batch of suggestions? Let's talk about the QHexView::copyAs function.

First things first, it seems there is a bug:

Any byte before the value 0x80 is copied correctly. That is if we copy 7F we get 0x7F in the CopyMode::Visual, for example. But starting from 0x80, however, things look odd. An extra 0xFFFFFF is added. For instance, 80 turns into 0xFFFFFF80.

Now on to the extra features it could benefit from.

When QHexView::copyAs does its job (and the regular QHexView::copy too), the resulting string is a single line. I beleive it would be more convenient if it resembled what we see in the widget instead. For instance, let's say we want to copy the first 24 bytes. Naturally I'd expect the resulting string to have a line break, so when pasted into a text editor it would look like this, having two rows just like in the widget:

0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F,
0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17

QHexOptions::linelength could probably be taken advantage of to implement that, but that's just a wild guess.

Also I beleive QHexView::copyAs could be customised further. The prepended "0x" and the separator ", " are hardcoded within the function, but they could be customizable as well, maybe as new QHexOptions members. For example, Common Lisp has a different notation for byte vectors: #(#xFF #xFF #xFF #xFF), also note the lack of commas for separation, they are not required there at all.

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, Common Lisp using only these types of brackets being the example again. In fact these three enumerations representing the three types of brackets could possibly themselves be moved to QHexOptions, and the QHexView::CopyMode would need to contain only three values - Visual as default, WithBrackets to wrap the result of Visual in brackets, and HexArrayChar as the special case. And perhaps CopyMode::Visual could be renamed to CopyMode::Default then?

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

That's a rough outline of how these features could be implemented. I guess something could go into QHexOptions and some other things become the parameters of QHexView::copyAs.

[Dax89]

Any byte before the value 0x80 is copied correctly. That is if we copy 7F we get 0x7F in the CopyMode::Visual, for example. But starting from 0x80, however, things look odd. An extra 0xFFFFFF is added. For instance, 80 turns into 0xFFFFFF80.

It seems that QString::number handles negative integers even if I cast it to uint, so I have replaced it with QHexView's internal integer-to-hex conversion.

Also I beleive QHexView::copyAs could be customised further. The prepended "0x" and the separator ", " are hardcoded within the function, but they could be customizable as well, maybe as new QHexOptions members. For example, Common Lisp has a different notation for byte vectors: #(#xFF #xFF #xFF #xFF), also note the lack of commas for separation, they are not required there at all.
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, Common Lisp using only these types of brackets being the example again. In fact these three enumerations representing the three types of brackets could possibly themselves be moved to QHexOptions, and the QHexView::CopyMode would need to contain only three values - Visual as default, WithBrackets to wrap the result of Visual in brackets, and HexArrayChar as the special case. And perhaps CopyMode::Visual could be renamed to CopyMode::Default then?

I have extended it a bit further: instead of adding more hardcoded/opinionated copy flavours I have introduced this tiny struct:

QHexView/include/QHexView/qhexview.h

Lines 17 to 24 in b7831d0

	struct QHexCopyFormat {
	QString prefix;
	QString suffix;
	QString byte_prefix;
	QString byte_suffix;
	QString separator;
	bool linebreak{false};
	};

which allows to copy bytes in any format you want, eg:

// Copy as C hexadecimal string
QHexCopyFormat cf;
cf.prefix = "\"";
cf.suffix = "\"";
cf.byte_prefix = "\\x";
hexview->copyFormat(cf);  // copies as "\xAA\xBB\xCC"

// Copy as Common Lisp expression
QHexCopyFormat cf;
cf.prefix = "#(";
cf.suffix = ")";
cf.byte_prefix = "#x";
cf.separator = " ";
hexview->copyFormat(cf);  // copies as #(#xAA #xBB #xCC)

So:

• copyAs and its related enum has been removed
• copyFormat provides a non-opinionated copy on clipboard
• copy works as before

Also, there is a new method called copyVisual which copies what you see on viewport (or selected bytes), it uses a simplifified version of the rendering code.
Because of this method, there is a new public one called visibleBytes which returns a QByteArray of what you see on viewport (I don't know if it will ever be useful outside though).

[T-640]

Excellent, this is even better than what I suggested, works great.

A few minor notes:

• I am getting compiler errors in QHexView::copyVisual (Qt 5.14 MSVC 2019):

qhexview.cpp:379: error: C2666: 'QByteArray::operator []': 5 overloads have similar conversions
could be 'QByteRef QByteArray::operator [](uint)'
or       'QByteRef QByteArray::operator [](int)'
or       'char QByteArray::operator [](uint) const'
or       'char QByteArray::operator [](int) const'
or       'built-in C++ operator[(const char *, __int64)'
qhexview.cpp:379: while trying to match the argument list '(QByteArray, qint64)'

Changing this (and in a few other places):

QHexUtils::toHex(bytes[i + col]).toUpper()}

to this:

QHexUtils::toHex(bytes[(unsigned int)i + col]).toUpper()}

fixed it, but this was just a quick patch to get things working immediately, I didn't put much thought into it, so something may probably be incorrect.

• I think it would look nicer if QHexView::copyFormat takes into account the length of QHexCopyFormat::prefix in order to align subsequent rows with the first one. That is instead of this:

{ 0x00, 0x01, 0x02 ...
0x03, 0x04, 0x05 ...
0x06, 0x07, 0x08 ... }

the output could look like this:

{ 0x00, 0x01, 0x02 ...
  0x03, 0x04, 0x05 ...
  0x06, 0x07, 0x08 ... }

or maybe even like this:

{
    0x00, 0x01, 0x02 ...
    0x03, 0x04, 0x05 ...
    0x06, 0x07, 0x08 ...
}

• The regular QHexView::copy doesn't do the line breaks, making a single-line string. Fortunately I can get the same result this with the newly introduced QHexCopyFormat, so this is not a problem, and I guess it is better that way.

Also, there is a new method called copyVisual which copies what you see on viewport

Good idea, this is much simpler than having to take a screenshot, using graphical editor to crop the image, etc. when there is a need to share hex output. Actually I recently came across a conversation where something like that could have been used. A person was explaining a particular point in file which needed to be edited in a hex editor, and his messages consisted of something along the lines of "you've got to reach this piece of text... skip a few rows... then find that byte..." and so on, a quick copy like you've implemented would have simplified things a lot for him.

Now, moving on to the search dialogs.

So, say, we are looking for a hex pattern. Like this:

00 ?? ?? ?? FF

So far so good. But what if the gap represented by wildcard characters is larger. 8 bytes? 16 bytes? A whole lot more? Would be very inconvenient to repeatedly write lots of question marks. And I guarantee somewhere in the middle of copy-pasting the user forgets something and ends up with the incorrect number of bytes to skip. This could be addressed with a relatively small change to the syntax:

00 ?? skip 25 FF

We could write the skip keyword followed by a decimal number of bytes to skip and after that proceed as usual.

Honestly, I am a bit conflicted about it. If a more sophisticated search like that is required it probably means that the user is better off using an actual scripting language rather than a domain-specific one. On the other hand it feels like a nice thing to have, still looks simple enough and if it doesn't require much effort to implement, then why not? And one wouldn't want to link to the whole Python runtime just to skip a few dozens of bytes in a hex editor, would he? I wonder, since hex editors certainly have been there for a long time, shouldn't there be some standard similar to the regular expressions, or at least some common practices regarding the syntax of hexadecimal search?

[Dax89]

Changing this (and in a few other places):

QHexUtils::toHex(bytes[i + col]).toUpper()}

to this:

QHexUtils::toHex(bytes[(unsigned int)i + col]).toUpper()}

I have fixed the one in copyVisual as you reported but I cannot test in a Qt5 environment right now, if there are other ones let me know!

{
    0x00, 0x01, 0x02 ...
    0x03, 0x04, 0x05 ...
    0x06, 0x07, 0x08 ...
}

I have opted for this formatting style, it's available when QHexCopyFormat:linebreak is set, because sometimes a non-formatted output is needed (like strings).
It's available in the latest commit.

00 ?? skip 25 FF
We could write the skip keyword followed by a decimal number of bytes to skip and after that proceed as usual.

I haven't checked this one yet, I will do in the following days.
I'm thinking to use .. as 'skip' character: 00 ?? .. 25 FF just to stay with 2 characters and make parsing easier.

[T-640]

... I cannot test in a Qt5 environment right now ...

Not to worry, I'm sticking to Qt5 and don't have any plans to switch to the new version any time soon, so whatever compiler errors arise I can report instantly. Speaking of which, there are two more places to apply the fix at:

QHexView/src/qhexview.cpp

Lines 396 to 397 in b241464

	: (QChar::isPrint(bytes[i + col])
	? QChar{bytes[i + col]}

I had a quick glance at documentation, they've changed the subscript operator's argument from plain integer to qsizetype in Qt6, which is likely to be larger than that, hence the compiler error. It does not really matter (I am having a hard time imagining someone trying to stick an array larger than 4 gigabytes into QHexView) but you might want to apply the integer type depending on the Qt version in use for the sake of correctness.

Regarding the new formatting style you've opted to, a few small suggestions, not crucial, but if they won't complicate the code much:

• The indentation in the copied string is always two characters long. Perhaps add an option to QHexCopyFormat to control it? And I guess there needs to be an option to choose between tabs and spaces, otherwise it might be unusable in Python.

• There is an extra white space character at the end of each line, like this:

{
    0x00, 0x01, 0x02,[extra white space here]
    0x03, 0x04, 0x05,[extra white space here]
    0x06, 0x07, 0x08
}

This does not really bother me, just being a bit pedantic there.

I'm thinking to use .. as 'skip' character: 00 ?? .. 25 FF just to stay with 2 characters and make parsing easier

That'll do, it is slightly less obvious than the 'skip' keyword, but on the other hand being two characters long it is more aesthetically pleasing in this context. The number representing the byte span to skip might need to be more than two characters long though, however unlikely it may be. I suppose you may implement the feature with this limitation first and then figure out what to do to bypass it.

Is the two-character policy due to spaces between hex bytes within the pattern being optional? If separating the pattern's bytes with spaces was enforced adding any keywords or even new syntax constructs might not be much of a challenge. Frankly speaking, I am not really a proponent of enforcing something or removing features, but what is the hex pattern not separated by spaces good for? One case I can think of is quickly copying an integer value in hexadecimal form from a calculator. But other than that anything beyond eight bytes is likely to look as gobbledegook in that notation. Well, maybe one other case is copying a hash value I suppose. Albeit I am speaking from a personal narrow perspective, maybe there are much more use cases for it that I am simply not aware of. Maybe you can even go as far as having two notations - one for simpy copy-pasting values for the cases I outlined and the other for manually constructing the pattern with a more sophisticated syntax.

[Dax89]

Not to worry, I'm sticking to Qt5 and don't have any plans to switch to the new version any time soon, so whatever compiler errors arise I can report instantly.

Great, thanks!

The indentation in the copied string is always two characters long. Perhaps add an option to QHexCopyFormat to control it? And I guess there needs to be an option to choose between tabs and spaces, otherwise it might be unusable in Python.

I did some tests, in my case the trailing whitespace is added because it's part of the separator (it's just appended as-is), separators can be right-trimmed when it's added before line break, I can add it as option in QHexCopyFormat.

Is the two-character policy due to spaces between hex bytes within the pattern being optional? If separating the pattern's bytes with spaces was enforced adding any keywords or even new syntax constructs might not be much of a challenge. Frankly speaking, I am not really a proponent of enforcing something or removing features, but what is the hex pattern not separated by spaces good for? One case I can think of is quickly copying an integer value in hexadecimal form from a calculator. But other than that anything beyond eight bytes is likely to look as gobbledegook in that notation. Well, maybe one other case is copying a hash value I suppose. Albeit I am speaking from a personal narrow perspective, maybe there are much more use cases for it that I am simply not aware of. Maybe you can even go as far as having two notations - one for simpy copy-pasting values for the cases I outlined and the other for manually constructing the pattern with a more sophisticated syntax.

The last commit finally implements pattern search with a more flexible implementation, so as we planned before, it supports:

• ??: single byte wildcard
• ..: multi byte wildcard or "skip until..."

Also edge cases like ..AABBCC, ..AABBCC.., AABBCC.., AABB......CC and AABBCC....??DD are valid and they means:

• ..AABBCC: match at the end
• ..AABBCC.. match at the middle
• AABBCC..: match at the start
• AABB......CC is like AABB..CC
• AABBCC....??DD is like AABBCC..??DD

Probably there are more edge cases, I haven't tested all possible combinations.

AFAIK whitespaces was optional even before, now I'm 100% sure: whitespaces are simply ignored.
The current implementation needs some deeper tests and code cleanup, I will do that in the next days, however let me know if you find something that doesn't work!

[T-640]

... separators can be right-trimmed when it's added before line break, I can add it as option in QHexCopyFormat.

Yea, if it won't bloat the function too much that'd be a small nice thing to have, along with the ability to control indentation (tabs, spaces, their count, etc.)

The last commit finally implements pattern search with a more flexible implementation, so as we planned before ...

Gave it a try, haven't noticed any issues, and that's even better than what I suggested, that is it does not require the exact number of bytes to skip. Nonetheless sometimes writing exactly how much bytes to skip might improve the pattern. Here's a somewhat contrived example: let's say we're looking for a local host source address within a set of IPv4 packets. The first byte is typically 45, the next eleven bytes are to be skipped, and the 127.0.0.1 address would be 7F 00 00 01. Thus the pattern would be written this way:

45 .. 7F 00 00 01

That will work correctly in 99,9% of cases. But there will be that one 0,01%, however unlikely, where other IPv4 header fields are formed in such a way that the 7F 00 00 01 pattern might randomly occur in these eleven bytes that are supposed to be skipped. It won't be the 127.0.0.1 address that we are looking for but a random amalgamation of IP header fields that just happens to form 7F 00 00 01. With the current setup this will be an incorrect successful match, because it will hit the unlikely random 7F 00 00 01 before it gets the chance to look for the actual 127.0.0.1. But if there would be a way to tell to completely ignore these eleven bytes and without having to write eleven ?? wildcards that should be no problem.

Note that I don't really have any practical application for such feature in mind at this time, just thought that it would be something useful for the pattern syntax to have.

[Dax89]

Yea, if it won't bloat the function too much that'd be a small nice thing to have, along with the ability to control indentation (tabs, spaces, their count, etc.)

Done, QHexCopyFormat now allows to control indentation and to trim the last separator before breaking the line (indent = -1 means that indentation is calculated automatically by using separator's length):

QHexView/include/QHexView/qhexview.h

Lines 17 to 27 in 2cfdfaa

	struct QHexCopyFormat {
	QString prefix;
	QString suffix;
	QString byte_prefix;
	QString byte_suffix;
	QString separator;
	int indent{-1};
	bool trim_last_separator{false};
	bool line_break{false};
	bool use_tabs{false};
	};

...
But if there would be a way to tell to completely ignore these eleven bytes and without having to write eleven ?? wildcards that should be no problem.
Note that I don't really have any practical application for such feature in mind at this time, just thought that it would be something useful for the pattern syntax to have.

We can keep this implementation for now, it can be always improved later.

PS: there is also this new feature which highlights changes done by the user.

[T-640]

Done, QHexCopyFormat now allows to control indentation and to trim the last separator before breaking the line ...

Seems like QHexView::copyFormat is feature-complete at this point, thanks! It is instrumental for me to write automatic tests for network packet parsing, with this I can easily copy packets that interest me the most, straight into the test's cpp file without the need to store them separately in a pcap file.

We can keep this implementation for now, it can be always improved later.

Of course, no need to rush it. Could be a goal for the next 5.2 version. You actually might be the pioneer with this particular pattern-searching syntax, as I don't think I have seen anyone go beyond the simple ?? wildcards in their implementations, although I had not used hexadecimal search in various tools as much as I would've liked to until recently. Overall making a domain-specific language in C++ appears to be a fairly challenging task, yet some folks are crazy enough to do it even in bare C.

Just make sure to write it down in some "TO DO" comment, or make a "TO DO" txt file somewhere in the project, because chances are we'll forget about it as time passes, at least I am pretty sure I will.

And a few things to add to this potential "TO DO" list. As before I am not sure how they will be useful in practice, but we can never find out unless this is implemented, and this being worthy of being added depends on how much effort it will take to implement.

• Ability to match a range of values. A contrived example of this would be the IPv4 packet again, or any other protocol that assigns an ID to its packets. The IPv4 header has two bytes for identification field, and suppose we need to look for a local host packets with a specific range of IDs. The way I see it the pattern could look something like that in this case:

45 ?? ?? ?? [AAAA - FFFF] skip 6 7F 00 00 01

• Ability to take a multi-line string (and for the dialogs to optionally replace QLineEdit with QPlainTextEdit respectively). Suppose we copy a large chunk of bytes from the the widget in the simplest form, that is 00 01 02 03 04 05 etc. without all the prefixes and suffixes, but with the newline characters, so the copied result looks exactly as in the hex area of the widget. We edit it a little bit, replacing some exact values with wildcards, etc., and there we have our big pattern. For small patterns a single line is enough, but for this large one that we copied it would be easier to work with the multi-line way.

... new feature which highlights changes done by the user.

Good addition, I checked it out. Do not hesitate to point out here any other discussions popping up because I might miss them as my full attention is on this one. I have a suggestion regarding this new feature, will post it in that discussion.

Something that caught my attention in the last post in that thread is that you create a whole new QHexDocument, just to call the QHexDocument::setTrackChanges and replace the original back in QHexView with this one. Perhaps just cut out the middle man and let this be done through a simple QHexView::hexDocument call? A non-const version of this function will need to be made but I think it's more logical this way.

You've also mentioned that both QHexDocument and QHexView have the clearChanges() function. Maybe the one in QHexView is redundant as it merely calls the QHexDocument's counterpart? It also has a few other functions like that, I'd say they could be removed as well.

Usually I prefer to have all the API within the main widget with the rest of implementation being hidden and inaccessible to user, but in this case calling QHexDocument directly might be a better option. QHexView already has a lot of public API, and that's not a bad thing of course, quite the opposite in fact as it proves how flexible the widget has become. Removing these proxy functions and relying on the direct calls on QHexDocument might provide some degree of separation for the public API, making the code cleaner, and user not being overwhelmed with the sheer number of features when peeking at qhexview.h.

The null pointer check for the QHexDocument that these proxy functions perform appears to be redundant, after having a quick look at the code it seems to me that the document is never null, even if the user tries to stick a null pointer in there. So the direct access to the underlying QHexDocument should be safe, making the proxy functions even less useful. Finally, you may implement the null pointer check within the QHexView::hexDocument itself just to be completely sure and throw an exception upon the null condition.

[Dax89]

Good addition, I checked it out. Do not hesitate to point out here any other discussions popping up because I might miss them as my full attention is on this one. I have a suggestion regarding this new feature, will post it in that discussion.

It's implemented, now we have trackchange_format_ins and trackchange_format_ovr:

QHexView/include/QHexView/model/qhexoptions.h

Lines 51 to 52 in 1a76eda

	QHexCharFormat trackchange_format_ins;
	QHexCharFormat trackchange_format_ovr;

Also the tracking algorithm is changed a bit, because an insertion is implemented as insert + replace (press F => 0F is inserted, press A => 0F will be replaced to FA), I have done some tests and it looks fine now.

Maybe the one in QHexView is redundant as it merely calls the QHexDocument's counterpart?

These functions are there for convenience, but they can be removed or, at least, we can keep the most common ones.
However, I have checked the code and currently QHexView's APIs that belongs to other classes are:

QHexDocument

• setData() (both overloads)
• setTrackChanges()
• undo()
• redo()
• clearModified()
• clearChanges()

QHexCursor

• setCursorMode()
• switchMode()
• selectAll() (this calls 2 QHexCursor's API, it can be refactored)
• selectionStart()
• selectionEnd()
• selectionStartOffset()
• selectionEndOffset()

QHexMetadata

• setMetadata()
• setForeground()
• setBackground()
• setForegroundSize()
• setBackgroundSize()
• setComment()
• setCommentSize()
• removeMetadata()
• removeBackground()
• removeForeground()
• removeComments()
• unhighlight()

There is also a subtle visual change: the hex area byte's background is now x-padded by char_width / 2 so, if there is any following or preceding byte background highlighted, they will be displayed as a single strip, probably a screenshot is better than thousand words (ps: they gray square is the cursor):

These are single bytes highlighted invidividually, but the extra padding makes it looks like a single rectangle.

[T-640]

Tracking seems fine, just one thing needs to be fixed: upon calling QHexView::setData with a new set of data whatever colors the tracking API has painted before when the previous data was edited remain, they must be cleared.

This got me thinking, since I am using this to view network packets and frequently switch between them, the tracking colors data would need to be saved somewhere and restored upon switching. It being gone is not really an issue though, and it is probably not worth the effort to implement the save/restore functionality, so I am just pointing out the current behavior.

And I'd probably use the full "insert" and "overwrite" words for the trackchange_format member names. Yes, the trackchange_format members have a pretty long name, but not that long. It is not a big deal though, just a personal preference, a habit of always trying to make my variables and functions to be "as English as possible".

These functions are there for convenience... QHexDocument, QHexCursor, QHexMetadata ...

That is a very nice list of the major functions you've written there, great starting point giving a quick and clear overview of the most important features. Free self-documenting code right here. With how neatly the API is decoupled now, it seems to me that getting the grasp of it is better through examining these individual classes rather than looking at a blob of functions of the main QHexView class. Helps to form the bigger picture seeing how the functionality is separated into logical partitions. I mean, as a user I am starting to look for how to do X, then come across one of these classes, and now know where to look for the related Y, Z, etc. functionality, already conveniently grouped for me.

So the convenience functions might be more of a hindrance to learning the API than the actual convenience at this point. We don't have convenience functions for the QHexOptions struct because we don't need to, and while it is vastly different from the three aforementioned classes, the same principle probably applies. Plus about two dozen less functions for you to maintain.

All this is mostly speculative of course, despite me leaning more toward the convenience functions removal I am still not fully certain, but the points above seem to be fairly solid nonetheless. Ideally it should be a few other folks dropping by telling what they think, but chances are it'll be just two of us here discussing that.

There is also a subtle visual change ...

This is a fine change, but I'd say, just like back when we discussed the QHexFlags::PaddedAddress feature, both selection options seem to look good and it is really hard to tell which one is superior, so if this is not too much work I'd say a flag to go back to the old style might be worth it.

[Dax89]

Tracking seems fine, just one thing needs to be fixed: upon calling QHexView::setData with a new set of data whatever colors the tracking API has painted before when the previous data was edited remain, they must be cleared.

Fixed, I've forgot to clear the list of changes, thanks for spotting it!

This got me thinking, since I am using this to view network packets and frequently switch between them, the tracking colors data would need to be saved somewhere and restored upon switching. It being gone is not really an issue though, and it is probably not worth the effort to implement the save/restore functionality, so I am just pointing out the current behavior.

That's how the widget now restores previous changes: I put a copy of QHexChanges list in a QUndoCommand subclass and they are saved/restored in case of undo/redo.

However, changes are stored in QHexDocument not in QHexView, so it's also possible to have many QHexDocument instances to achieve this.

All this is mostly speculative of course, despite me leaning more toward the convenience functions removal I am still not fully certain, but the points above seem to be fairly solid nonetheless. Ideally it should be a few other folks dropping by telling what they think, but chances are it'll be just two of us here discussing that.

I also don't really know what to keep in QHexView and what to remove, we can keep this proposal more in the oven and a solution will comes out for a future release.
Yeah, more feedback will improve the widget's ergonomics, but I can't complain anyone for this :D

This is a fine change, but I'd say, just like back when we discussed the QHexFlags::PaddedAddress feature, both selection options seem to look good and it is really hard to tell which one is superior, so if this is not too much work I'd say a flag to go back to the old style might be worth it.

Implemented as QHexFlags::PaddedHighlight.
It's disabled by default (like other options) and when byte grouping is > 1, because it makes padding a bit tricky and error prone.

[T-640]

However, changes are stored in QHexDocument not in QHexView, so it's also possible to have many QHexDocument instances to achieve this.

I have just realized that I forgot that QHexView::dataChanged signal has all the necessary information about tracking the changes to save and restore. I am using a single instance of QHexDocument because it seems to be an overkill to have one for every single network packet that I capture, which could be thousands and more, so I simply clear and reset the one that comes with the QHexView each time I switch packet to examine. I have implemented a rough sketch of a system which stores a sequence of structures which contain QHexChangeReason, etc. alongside the packet, and once the current packet is switched this sequence is used to call the insert, replace and remove of QHexDocument to restore all the tracking, seems to work fine so far.

I think a non-const counterpart of QHexView::hexDocument function could be added to accommodate this so no const_cast is required, unless creating a new QHexDocument is a more preferable option.

... we can keep this proposal more in the open and a solution will comes out for a future release ...

Yea, it's a tough question, I guess it'll be clearer by the time the development will have been reaching the conclusion. Meanwhile better just keep these things we are not completely sure about (this and the syntax of hex search pattern) in some list so we won't have to dig through this discussion again when working on the next versions.

QHexFlags::PaddedHighlight

Doesn't seem to make much difference, if any at all to me, but adjusting the divisor here seems to do the trick:

QHexView/src/qhexview.cpp

Lines 80 to 84 in 628d248

	if(this->hexview->m_options.hasFlag(QHexFlags::PaddedHighlight) &&
	this->hexview->m_options.group_length == 1 && pad) {
	br.adjust(-this->hexview->cellWidth() / 2, 0,
	this->hexview->cellWidth() / 2, 0);
	}

Replacing the number 2 with not 4, not 8, but actually 16 (!) does bring back the old style. No idea why exactly this number, but it appears to work fine with this value, at least on my high DPI system.

---

Now, moving on to a few bugs regarding incorrect rectangles returned for some areas. Note that there is a very small chance these might be outdated as I have prepared this text in advance and decided to wait till we finished with copy formats and change tracking, so the discussion does not become cluttered. So here the bugs are:

The width of the address rectangle returned by the QHexView::addressRect is incorrect. It covers the hex and the ASCII areas as well, not just the address part. Calling QHexView::endColumnXthere does not seem right.

This is the calculation I came up with:

constexpr unsigned int address_padding = 2;
qreal width = (this->addressWidth() + address_padding) * this->cellWidth();
return QRectF (0, y, width, this->height() - y);

The width of QHexView::asciiRect needs to be adjusted as well, it does not take padding into account:

this->endColumnX() - this->asciiColumnX()

Add cellWidth() to it:

this->endColumnX() - this->asciiColumnX() + this->cellWidth()

---

Speaking of rectangles, since we have addressRect, hexRect and asciiRect, perhaps they should be complemented with similar functions but for the header? That is headerAddressRect, headerHexRect and headerAsciiRect? Or alternatively addressRectHeader, hexRectHeader and asciiRectHeader.

Having these three functions you can remove their counterparts in QHexDelegate that return strings. Now QHexDelegate::paint, having access to these rectangles, can take care of writing text there, becoming the one function "to rule them all".

This is how my delegate paints the position of the selected byte after I experimented a bit:

static const QString pos_string = QStringLiteral ("Pos:");
QHexPosition position = hex_view->hexCursor()->position();
QRectF rect = hex_view->headerAddressRect();

painter->save();
painter->setPen (hex_view->options().header_format.foreground);
painter->drawText (rect, pos_string);
rect.setLeft (hex_view->cellWidth() * pos_string.size() + hex_view->cellWidth() / 2);
painter->drawText (rect, QString::number (position.line * 16 + position.column).rightJustified (5, '0'));
painter->restore();

QHexDelegate::paint (painter, hex_view);

This is how it looks:

This is a bit more complex than simply using the QHexDelegate::addressHeader, but not much.

And the more freedom I have in QHexDelegate::paint came in handy: the address part does not contain much space, so I grab all the free space I can get my hands on:

• Writing on padding (QHexDelegate::addressHeader wouldn't let me do that);
• Instead of writing the full space character that takes too much valuable space I sort of "cut" it in half to fit more digits.

Speaking of the latter, I turned QHexView::cellWidth from private to public in my experiments to do the "cutting". Could have used QFontMetricsF, but since QHexView already has what I need I just used that. So perhaps this method should be made public? My particular case is pretty obscure but in general it seems logical for the QHexDelegate to have access to cell width (and I guess line height as well, but I am not using it, this is just an assumption) to do all kinds of shenanigans it gives the opportunity to perform.

And now here we arrive back at the dilemma of what to keep in the QHexView class and what not. I also recall you have mentioned earlier adding a new QHexView::visibleBytes public method and being unsure whether it will be useful outside. So it is challenging to predict what might be useful for the user and what not. So I suggest keeping as many methods as possible private, like QHexView::visibleBytes, but make them public if someone asks for it, like with QHexView::cellWidth.

By the time the development is getting close to finish and things become much clearer a final assessment regarding what part of the QHexView classe's API should be public, private or even removed could be made.