Move Qt 5.11 / 5.12 version warning to Import invocation

[gedakc]

See issue #611

The warning previously added in PR #612 for Manuskript users with Qt 5.11 / 5.12 has shown itself, in my opinion, to be overly annoying. This is because the warning always displays on systems with the affected Qt versions even though the crash is verified to happen with the import feature only.

Additionally the process to upgrade to a newer version of PyQt / Qt is not trivial for users who rely on pre-built packages and do not run from source code.

At time of writing some currently supported OSes with a most recent Qt package version in the Qt 5.11 / 5.12 range are:

OS	Qt Version
Debian 10 Buster	5.11.3
Debian 11 Bullseye	5.11.3
Fedora 29	5.11.1
Fedora 30	5.12.4
Ubuntu 18.10 Cosmic	5.11.1
Ubuntu 19.04 Disco	5.12.2

Because the crash has been verified with the Import feature only, limit the scope of the warning to the Import feature.

Testing

Test platform: Kubuntu 19.04 with Qt 5.12.2, PyQt 5.12.1, and Python 3.7.3

In order to see if other areas of Manuskript would crash with Qt 5.11 / 5.12 I performed the following testing:

Feature	Works?
Navigation tabs - data entry	Yes
File -> Import	No - crash
File -> Compile - preview built-in HTML, Pandoc PDF	Yes
Edit -> Settings - Views/Text editor/Font size	Yes
Organize - Move up/down	Yes
View -> Mode - simple/fiction	Yes
Tools -> Spellcheck	Yes
Tolls -> Frequency analyzer	Yes
Help -> Show help texts	Yes
Cheatsheet	Yes
Search	Yes
Story line	Yes

Based on the above testing only the Import feature appears to crash with Qt 5.11/5.12. As such this PR moves the Qt version warning to the Import feature invocation.

This PR is ready for review.

[worstje]

That force-push just stopped my review midway. :) I was still writing some more, so I'm afraid you may not be done appeasing the review gods yet. ;)

[gedakc]

Thanks for reviewing.

One strange thing is that my recent force push triggered Travis CI on both my gedakc and the olivierkes repos at once. I haven't seen that before....

In the most recent push, both paragraph and bold HTML tags are started in the line above the translated question "Proceed with import?". After the translated line, the bold and paragraph HTML tags are closed.

I agree that Manuskript crashes when the import dialog is displayed.

Instead of "Proceed with import?" would something like "Proceed to display Import dialog?" be better?

[gedakc]

I think most users are concerned about whether import will work or not. I think the nuance about what causes the crash will be of minor importance to them.

[gedakc]

Aha! Just saw the solitary closing bold you mentioned. I'll fix that.

the previous dialog passed review in terms of message and layout, so why is there suddenly so much change to the popup dialog?

The reason is two fold.

• Originally I agreed that users should be able to upgrade to a newer version of Qt. However upon later reflection I don't think this is easy for the majority of users. Many can install a pre-built package such as the default packages for their GNU/Linux distor, but asking them to run other "source code" packages is often a step too far.
• I've been testing various Manuskript PR's since the original warning was added. I found it quite annoying to have the warning message displayed on Manuskript start-up for each and every test I ran on a computer or VM with Qt 5.11 and Qt 5.12 installed.

Regarding bolding in a message box I'll do some digging to see if I can find a Qt reference. If not I'll take a look at the GNOME HIG again.

[worstje]

Well, I turned this PR into a mess with that review and now these comments. 🤣 Sorry about that.

Regarding bold... all these most recent comments are about the most recent push. You still have a stray bold-closer in there for warning2 that you probably don't intend to have there. I checked several times to make sure I wasn't missing anything. 😄

I think the nuance about what causes the crash will be of minor importance to them.

Users only care that it crashes, yes. But they also expect a dialog to be accurate in the information it offers. If after reading the dialog they are under the impression that it is safe to look at the dialog, they'll just continue on... and might still lose their work.

If they lose work after we put in a message that misrepresents the nature of the issue, that data loss is on us. Many users might not read the dialogs shown to them, but betraying the ones that do read is a complete no-no by my book. 😄

Instead of "Proceed with import?" would something like "Proceed to display Import dialog?" be better?

That is good. But I'd still insist on taking out the bit about 'imports' in warning1. The fact we show the message at that point in the program reveals that 'something about what is about to happen might cause a crash', and being needlessly specific is no good. I'd suggest going with:

warning1 = self.tr("Qt 5.11 and 5.12 are known to cause a crash which might result in a loss of data.")

Your thoughts?

[gedakc]

Thank you for the feedback. I'm reworking the dialog with your suggestions and will post a screen shot shortly.

[worstje]

I don't disagree; it was rather annoying to have it on the startup there. But that's not the part I was questioning. 😄

My point is that every sentence was reworded, the emphasis was moved and the buttons were changed. The whole warning dialog underwent a makeover. Rather than 'moving the warning' it feels like 'redesigning the whole warning' to me.

It may well be necessary, and I certainly hope I don't sound contrary just because I designed the old after trying to do my due diligence. I just want to understand why there is so much change on every aspect of the original dialog when that is so far out of my expectations for this PR. 😄

[gedakc]

I was trying to make the warning message more specific to the crash when displaying the Manuskript import dialog. As you correctly point out I was not totally on the mark with my attempt.

While rewording I discovered that the original substitution was stating "PyQt", but was placing the Qt version (not PyQt) in the message. I had missed this in my earlier review.

By moving the warning, I found that the original message text would benefit from some wording changes.

Original message which was displayed at Manuskript startup:

With the above I think the choices of Abort or Ignore made sense.

New reworded (just now) message displayed when user chooses File -> Import:

With this new text, I think Yes or No are appropriate.

I think bolding the question "Proceed to display Import dialog?" would draw the user to this question as being relevant to the dialog buttons.

I'm still need to search for Qt guidelines for bolding in message boxes.

[gedakc]

Found a reference: User Interface Text Guidelines

From a quick read I realize the text I proposed still needs improvement.

I'm off for lunch and will continue working on this after.

[worstje]

I hadn't realized the switcheroo between Qt and PyQt, good catch. The reason I went with PyQt is because on Windows PyQt is built against and ships with a particular version of Qt already. The user might not even have installed Qt! I am not sure what the situation is on Linux, but since it is in the end all about the runtime, I figured testing against qVersion() (it is a Qt bug that affects runtime) would work out while talking about PyQt; after all, for most users that is going to be the package that needs replacing. This might need proper research and/or practical testing, but I think PyQt and Qt versions matching is a safe enough assumption in most cases.

One slight problem I see with your screenshots is that the 'yes' icon implies a 'yes, this is the good option that you want' deal. While in reality, that one might lead to a crash. Those icons are treacherous, but I am not sure what better option exists. (Though, to be fair: I have no clue what the icons would look like on Windows, for this or for the original patch...)

[gedakc]

One slight problem I see with your screenshots is that the 'yes' icon implies a 'yes, this is the good option that you want' deal.

Good point. I agree that the green check mark looks like a good thing to choose. I'll try re-wording closer to your original suggestions so that button names like "Abort" are used.

Regarding PyQt and Qt versions, these often follow closely together, but not exactly.

Following is a list of versions I have installed in some of my VMs:

Qt5	PyQt5	OS
5.5.1	5.5.1	Kubuntu 16.04
5.6.0	5.6.1	Fedora 24
5.7.0	5.7	Fedora 25
5.7.1	5.8.1	Fedora 26
5.9.1	5.9	Fedora 27
5.9.5	5.10.1	Ubuntu 18.04
5.10.1	5.10.1	Debian 9 (pip3)
5.11.1	5.11.2	Ubuntu 18.10
5.11.3	5.11.3	Debian 10 (default)
5.12.2	5.12.1	Ubuntu 19.04
5.13.0	5.13.0	Debian 10 (pip3)

Note the discrepancy with Fedora 26. I double-checked this anomaly just to confirm I didn't make a typo.

[gedakc]

Following is the wording contained in screen shots for the code change I just force pushed.

I've tried to take into account the many issues with wording and icons to make this dialog easier to understand.

Windows 10:

Ubuntu 18.10:

Kubuntu 19.04 (with Breeze Dark theme):

Please review.

If possible I would like to create a new Manuskript release this week and include this PR.

[worstje]

That dialog looks OK to me, as does the patch that produces it. 👍

Edit: Grammatically, wouldn't '... in a loss of data.' be preferable over '... in loss of data.'?

[gedakc]

Edit: Grammatically, wouldn't '... in a loss of data.' be preferable over '... in loss of data.'?

I will make this suggested change and plan to merge with develop.

[gedakc]

Merging this PR with develop branch for inclusion in the next release of Manuskript.