Working Pandoc import (fixes #611) & small dialog UI update.

[worstje]

Once I realized I didn't need to waste a full day trying to compile Qt & PyQt from scratch, fixing issue #611 was a breeze.

Because a single line fix is a bit disappointing for a PR, I figured I'd also make sure to have the FileDialog used by the Import screen match the look of the native operating system like everywhere else in Manuskript does. It is but a small change, but at least I can now pretend to have something to show for a full days worth of headache. 😉

Using these changes, I have tested importing a Markdown file using Pandoc using both formatTo=OPML and formatTo=markdown, and both seemed to result in a successful import for me.

There is no doubt plenty of work to be done, but at least we can have the functionality back in the upcoming 0.10.0 once more.

IMHO, the following is work that needs doing in the future where the Import UI is concerned:

• Any warnings / errors thrown by pandoc pop into the console; we definitely want to user-facing at some point

• We also need to polish the UI. perhaps to better match the exporter... assuming the exporter UI is what we want, which I am not sure it is. Currently they look similar, but aren't quite similar at all. This might throw users off. Right now the visual layout is unsatisfactory (albeit functional)... but fixing that is a bit too much for me to tackle at present.

[gedakc]

I'm just beginning my review of this PR and was curious about the commented out line mentioned in this issue 611 comment.

It turns out it was me, gedakc, who made this change on 2018-11-11 with commit b0774b4.

I'll have to look deeper to see if this is simply a mistake I made, or if there something more regarding Qt versions.

[worstje]

I wonder if you ran into the same crashing bug that sent me down the rabbithole of trying to compile everything by hand. Anyway, from what I noticed, the crashing bug that sent me in circles did not apply to older Qt versions, nor to the most recent one, so I suspect it was aonly somewhere around the 5.11 releases (give or take a release perhaps) that it was crashing.

And from what I could tell, there was no real way to diagnose or work around the bug from our side. The crash happened in completely ordinary UI setup code as a side-effect of some other call somewhere. IMHO, the best option seems to be to just document it properly.

[gedakc]

The problem appears to be an issue with the Qt 5.11 releases. ;-(

When I run on Ubuntu 18.10 with this PR applied and then try an import, it crashes with the following console log:

user@ubuntu1810:~/manuskript.gedakc$ bin/manuskript 
Debug: Web rendering engine used: QWebView
Running manuskript version 0.9.0.
Note: No translator found or loaded from system locale for locale en_CA.
Loading: /home/user/deleteme-2.msk
Detected file format version: 1. Zip: True.
Project /home/user/deleteme-2.msk loaded.
Fatal Python error: Segmentation fault

Current thread 0x00007f9b0dfe7740 (most recent call first):
  File "/home/user/manuskript.gedakc/bin/../manuskript/ui/importers/importer.py", line 220 in setGroupWidget
  File "/home/user/manuskript.gedakc/bin/../manuskript/ui/importers/importer.py", line 202 in updateSettings
  File "/home/user/manuskript.gedakc/bin/../manuskript/ui/importers/importer.py", line 60 in __init__
  File "/home/user/manuskript.gedakc/bin/../manuskript/mainWindow.py", line 1583 in doImport
  File "/home/user/manuskript.gedakc/bin/../manuskript/main.py", line 145 in launch
  File "/home/user/manuskript.gedakc/bin/../manuskript/main.py", line 158 in run
  File "bin/manuskript", line 13 in <module>
Segmentation fault (core dumped)

On Ubuntu 18.10 the following software versions are in Use:

• Python 3.6.7
• PyQt 5.11.2
• Qt 5.11.1

I am searching for a backward compatible fix so that imports work both before and with Qt 5.11. Otherwise we might consider disabling import if Qt 5.11 is being used.

[gedakc]

Anyway, from what I noticed, the crashing bug that sent me in circles did not apply to older Qt versions, nor to the most recent one, so I suspect it was aonly somewhere around the 5.11 releases (give or take a release perhaps) that it was crashing.

@worstje, what Qt version was the "most recent one" that you found did not crash?

With this PR applied I still get a crash with pandoc import with Qt 5.12.2 which I tested with Ubuntu 19.04.

Software Versions in Use with Ubuntu 19.04:

• Python 3.7.3
• PyQt 5.12.1
• Qt 5.12.2

[worstje]

C:\Users\Me>pip show pyqt5
Name: PyQt5
Version: 5.13.0
Summary: Python bindings for the Qt cross platform UI and application toolkit
...

It looks like upgrading to PyQt 5.13 is what ended up fixing it for me. Dang, I wish I had taken note of the version I had installed prior to that, but I think it must have been a 5.12 version since I do not recall any surprise over being more than one 'major' version behind.

Judging by some of the backups I have of my 0.9.0 binary installation of Manuskript and the modification dates on the folders there, I think I started messing with the Manuskript source back in early February. So based on the PyQt5 release history, I was most likely using either 5.11.3 or 5.12 initial release.

[gedakc]

Following are the results of testing this PR with pandoc import of a markdown file.

Import works?	Qt	PyQt5	Python	OS
Yes	5.5.1	5.5.1	3.5.2	Kubuntu 16.04
Yes	5.7.1	5.7	3.5.3	Debian 9
Yes	5.9.5	5.10.1	3.6.7	Ubuntu 18.04
Yes	5.10.1	5.10.1	3.5.3	Debian 9
No - Crash	5.11.1	5.11.2	3.6.7	Ubuntu 18.10
No - Crash	5.12.2	5.12.1	3.7.3	Ubuntu 19.04
Yes	5.13.0	5.13.0	3.7.3	Debian 10

Based on these tests, it would appear that import is broken for the following Qt versions:

• Qt 5.11.x
• Qt 5.12.x

So far I have not discovered a fix that works for all versions, or even the problem versions.

Are you able to come up with a fix?

[worstje]

I doubt it. Despite spending some time on their bugtracker, I wasn't able to identify which bug is the cause; at most I could find issues that might be the same bug, but they don't have to be the one... which honestly isn't very helpful given the amount of bugs I see listed on that tracker.

For example, I think QT-BUG-69204 could be the one responsible. That one was apparently resolved in Qt 5.12.4, and since the code that exhibited the bug was joyfully adding and removing UI elements for the export dialog, I think it definitely could be the culprit. Perhaps we can figure out which bug is the culprit if we bisect in more detail which version it was fixed in... but do we really want to?

From my time trying to get to the bottom of it, the bug was not something for which the cause was apparent. It crashed while executing a really common call (I forgot which, something like setParent() or insertWidget(), I honestly forgot at this point) and to me the crash appeared to just be the delayed consequence of memory corruption elsewhere. Since a newer version of Qt fixed the problem,, I considered my time to be better spent not chasing that particular ghost anymore.

Which operating systems / distributions are unable to upgrade to a version of Qt that fixes this bug?

[gedakc]

Perhaps we can figure out which bug is the culprit if we bisect in more detail which version it was fixed in... but do we really want to?

Because the problem goes away with Qt 5.13 I am reluctant to spend the time needed to develop a fix for the Qt 5.11.x and Qt 5.12.x releases.

Which operating systems / distributions are unable to upgrade to a version of Qt that fixes this bug?

I think that all of the OSes are able to either upgrade or downgrade to a version that works with Manuskript and Import.

Since a segmentation fault resulting in the crash of Manuskript is not desirable, what do you think of disabling Import if Qt is version 5.11.x or 5.12.x?

EDIT: An example of code using the Qt version is available in manuskript/ui/about.py

[worstje]

Because the problem goes away with Qt 5.13 I am reluctant to spend the time needed to develop a fix for the Qt 5.11.x and Qt 5.12.x releases.

Same here. I thought you kept testing versions because you wanted to make sure it kept working in a particular OS. (Like we do for Windows XP with some antiquated Qt version for that.)

Since a segmentation fault resulting in the crash of Manuskript is not desirable, what do you think of disabling Import if Qt is version 5.11.x or 5.12.x?

On one hand, it might be a 'workaround', no matter how crappy. But on the other hand, I feel it just hides the problem from the user. How do we know the bug cannot be triggered in the export dialog? Or the settings dialog, or even the main window if following a specific set of steps? We do not know what causes it, except that the crashes happen in really basic Qt API calls... so the problem might still be lurking elsewhere in our code.

I'd prefer popping up a warning dialog when starting the program. 'You are running a version of Qt that is known to cause crashes when using Manuskript. Please upgrade Qt, or continue at your own risk.'

It is still not a neat solution (it does not technically fix the crash we know of), but I think it might be neater.

Maybe the two approaches should be combined for the best user experience? Let me know your preference to handling this and I'll prepare the patch for it.

[gedakc]

I like your suggestion of a pop-up when starting manuskript that would only be displayed if a user was running with Qt 5.11.x or Qt 5.12.x.

Your suggestion leaves open the possibility of a user running Manuskript with a potential future bug-fix release of Qt in the 5.11.x or 5.12.x range.

A patch to add the pop-up would be welcome. If you don't have time to do this, then I could add a warning to the "Run from Source Code" wiki pages.

Would you also consider editing the commit messages to include a reference to the Issue and PR numbers?

The reason I ask is because it makes it easier to trouble shoot later.

For example when reviewing this PR I used git blame to discover the commit that commented out the line in question. From there I looked up the commit which included a link to the issue. The issue then contained other useful links. This linking of one thing to another makes it quicker for me to re-learn why code changes were implemented.

It turns out I made a bad fix for the original problem which was a segmentation fault on import.

[worstje]

A patch to add the pop-up would be welcome. If you don't have time to do this, then I could add a warning to the "Run from Source Code" wiki pages.

Will add it when I dig into it. (Hopefully today, but I keep procrastinating with other things!)

Would you also consider editing the commit messages to include a reference to the Issue and PR numbers?

No problem. I think I still intended to perform a squash before merge anyways. (Or maybe it was that other PR?) There's always a few things that need adjusting after all.

[gedakc]

If you have a chance to just cleanup the existing patch commits (don't worry about implementing the warning message) then I can include this in an upcoming 0.10.0 release which I would like to start next week.

[worstje]

Sorry it took a while. 😄 A busy life and Windows Update very stubbornly breaking my computer with the same patch several times in a row was enough to put me off.

I implemented the warning, reordered the patches and included PR / Issue numbers where relevant. All good to go now.

[gedakc]

Thanks for the updates.

This next request may sound picky, but makes is possible to simply click on a reference because a link will be automatically built for it on github.

Can you make the PR reference in the commit comments prefix a number sign ("#") before the issue / PR number?

For example:

• PR-612 does not create an link.
• PR Working Pandoc import (fixes #611) & small dialog UI update. #612 does create a link.

In the meantime I'll continue testing and reviewing this PR.

[worstje]

Done. 😄

[gedakc]

My review and testing has gone well. The only times the warning was displayed was for Qt 5.11 and 5.12 which are the known problem versions which cause Manuskript to crash when trying to import a file.

Warning Displayed?	Qt	PyQt5	Python	OS
No	5.5.1	5.5.1	3.5.2	Kubuntu 16.04
No	5.10.1	5.10.1	3.5.3	Debian 9
Yes	5.11.1	5.11.2	3.6.7	Ubuntu 18.10
Yes	5.12.2	5.12.1	3.7.3	Ubuntu 19.04
No	5.13.0	5.13.0	3.7.3	Debian 10

I will rebase and merge this PR for inclusion in the next release of Manuskript.