Images can be saved, undo & redo is working

[Onetchou]

Fixes #1100
Fixes #1098

[Onetchou]

Since this PR is quite big and adds a lot of things it would be great if someone else than me could also test it to check if there's no unexpected crashes of the software in random situations 😉.

[DSCaskey]

, m_isHovered(false)
, m_coloredHandles(false)

I get the desire to change the flag variable name, but it makes no sense. For one it doesn't match the condition that you're testing for... that is are you in a hover event? Secondly the handles are always colored... they're either darkgrey or red. Thirdly isHovered is used in all the other tools, so now we're breaking continutity by renaming this flag. 

I think what I would do, since that's the condition we're really checking, is name this flag something like m_handleIsHighlighted so that

if (m_handleIsHighlighted){} ... reads better than if (isHovered){}

Then to be consistent I would then also address the other isHovered's at some point like:

m_imageIsHighlighted, m_lineIsHighlighted, m_scenePointIsHighlighted, etc...

[DSCaskey]

There's several issues I found if loading a pattern or redo'ng and an image file is deleted while working - not likely to happen, but possible. First is the dialog that pops up tells the user the file can not be read and it may be corrupt. This is not the case, but rather the file is missing Big difference.

The big issue is that the after you click OK, the app crashes. When reading, or redo'ing, need to check if the file exists before continuing... if NOT we need a new Error message box stating the file is not found... along the lines like not finding a measurement file when loading a pattern. A bonus would be the option to "find" a / the file. Again, along the lines of finding a missing measurement file.

[DSCaskey]

const QString VPatternConverter::PatternMaxVerStr = QStringLiteral("0.7.0");
const QString VPatternConverter::CurrentSchema = QStringLiteral("://schema/pattern/v0.7.0.xsd");
const QString VPatternConverter::PatternMaxVerStr = QStringLiteral("0.8.0");
const QString VPatternConverter::CurrentSchema = QStringLiteral("://schema/pattern/v0.8.0.xsd");

Is there a reason we went from 0.7.0 to 0.8.0 and skipped all the schemas in between?

For consistency, the next locical bump in schema should be v0.7.1.xsd.

[Onetchou]

I get the desire to change the flag variable name, but it makes no sense. For one it doesn't match the condition that you're testing for... that is are you in a hover event? [...] Thirdly isHovered is used in all the other tools, so now we're breaking continutity by renaming this flag.

I changed the name of the flag because I added a condition that is not related to a hover event:

You can be over the handle but if you unclick it will unset the flag, even if it's still hovered...

Secondly the handles are always colored... they're either darkgrey or red.
I think what I would do, since that's the condition we're really checking, is name this flag something like m_handleIsHighlighted

😆 ok I'll change it if you want, no problem. But I don't think this is really a problem since it's quite understandable I think. Because if we want to use proper scientific definitions everywhere, one could argue that black is not a color and we'll have a lot of things to change in the software to correct this 😉

Then to be consistent I would then also address the other isHovered's at some point like:

m_imageIsHighlighted, m_lineIsHighlighted, m_scenePointIsHighlighted, etc...

I think that isHovered is perfectly fine everywhere else. I changed it here only because I changed the behavior of the flag: it doesn't reflect anymore the fact that the handle is hovered or not.

[Onetchou]

There's several issues I found if loading a pattern or redo'ng and an image file is deleted while working - not likely to happen, but possible. First is the dialog that pops up tells the user the file can not be read and it may be corrupt. This is not the case, but rather the file is missing Big difference.

The big issue is that the after you click OK, the app crashes. When reading, or redo'ing, need to check if the file exists before continuing... if NOT we need a new Error message box stating the file is not found... along the lines like not finding a measurement file when loading a pattern.

I was so focused to make the undo and redo work that I completely forgot to address the cases where the file is missing: I'll take a look at it, thanks for noticing it!

A bonus would be the option to "find" a / the file. Again, along the lines of finding a missing measurement file.

Yes, great idea, I'll implement it.

[Onetchou]

Is there a reason we went from 0.7.0 to 0.8.0 and skipped all the schemas in between?

Because I thought it was a major change since new items are added, this is not only new attributes of existing items:

[DSCaskey]

You can be over the handle but if you unclick it will unset the flag, even if it's still hovered...

But the handles are always colored like I said. m_coloredHandles = false implies the handles are no longer colored. BTW... using "m_coloredHandles" implies you're coloring the handles NOT just this handle.

I think that isHovered is perfectly fine everywhere else. I changed it here only because I changed the behavior of the flag: it doesn't reflect anymore the fact that the handle is hovered or not.

Again, the condition that you're checking is whether to highlight the handle Red or not... which was my point of m_handleIsHighlighted. If it makes more sense... you could use m_highlightedHandle. Either one in English indicates the handle is highlighted... semantically different than coloredHandles or handlesAreColored. Yes, a highlighted handle is colored, but not every colored handle is highlighted. If you wanted to use m_redColoredHandle... then you'd be more correct, but still miss the WHY it's red.

painter->setBrush(m_highlightedHandle ? QColor(Qt::red) : QColor(Qt::darkGray));

Because I thought it was a major change since new items are added, this is not only new attributes of existing items:

Fair enough. I don't know why that's even there. Seems like it applies more to a build version, not a schema. The schema has nothing directly to do with a release. We could build a 1000 releases and patches without ever changing the schema. I suspect that RT just cut and pasted code from somewhere else not understanding the comments are not applicable in the case of the schemas. In fact the schema could have just as well been coded as a single digit, and it would have been easier. Like v001.xsd, v002.xsd... v100.xsd. If there is harder way to do things, that's what RT does.

That being said if it's not that big a hassle I'd like to see this schema as v0.7.1.xsd just so down the line someone else does wonder what happened to v0.7.1.xsd to v0.7.9.xsd... or worse try to use one of them.

[Onetchou]

Another error to solve (memo for myself):
New pattern > Add image > Undo > Redo > Add a point (endline tool) >

[Onetchou]

When reading, or redo'ing, need to check if the file exists before continuing... if NOT we need a new Error message box stating the file is not found... along the lines like not finding a measurement file when loading a pattern. A bonus would be the option to "find" a / the file. Again, along the lines of finding a missing measurement file.

Done 👌

I think what I would do, since that's the condition we're really checking, is name this flag something like m_handleIsHighlighted

Done 👌

That being said if it's not that big a hassle I'd like to see this schema as v0.7.1.xsd just so down the line someone else does wonder what happened to v0.7.1.xsd to v0.7.9.xsd... or worse try to use one of them.

Done 👌

😊

[DSCaskey]

Looks good to me.

[Onetchou]

@DSCaskey I've set this PR as a "draft PR", because it was not ready to be merged, there's still this important issue to be solved :

Another error to solve (memo for myself):
New pattern > Add image > Undo > Redo > Add a point (endline tool) >