Further refinement of image tooltips (issue #593) #629
Conversation
|
I decided not to squash these two commits together because having them separate helps set this last one apart from the other Linux-related Qt that was already in the patch. 😄 Image link tooltip test sheet.txt (updated to match new patch) This should be the final version of the image tooltip code. (Famous last words...) Edit: Yes, I totally jinxed it. Let me know your preference on how you'd like this mess to be squashed, I suppose..? 🐸 |
|
In my testing on Kubuntu 16.04 with Qt 5.5.1 this PR is a definite improvement over the crash that would previously occur I agree with your assessment to let Qt handle all the OS specific stuff so that we can keep the code simpler and more maintainable. Personally I would like if it could resolve a relative URL like "figures/image.jpg" as if it was a file path in the current working directory, but this sounds like it would add undue complexity. While reviewing the code I noticed the following sentence which can appear in the GUI:
In order to remove any sense of judgement from the message I suggest we use something like the following:
When I'm not sure about a sentence I try to follow the GNOME Human Interface Guidelines for writing style.
Please squash this PR into a single commit. Thanks, |
Last time I touched this code, I went in looking for a specific problem, and came out with a fix specific to that issue. That fix was not wrong, yet it hardly covered all the problems present in the code once one took into account issues like: - local vs remote resources, - relative vs absolute paths, - different operating systems behaving differently, and - Qt being uniquely buggy on different platforms. The major part of it was fixed by using QUrl.fromUserInput(), which does the exact kind of auto-detection for the nature of the resource that we were in need of. The rest of the issues were fixed by creating a number of test cases and fixing problems as they popped up. Testing was done in Windows & Ubunty against the above-mentioned test cases, which can be found in PR olivierkes#629. Regarding ImageTooltip.supportedSchemes When QUrl.fromUserInput() misidentifies the scheme on Linux, it causes all resemblance between the original request and the reply.request() in the finished() signal to be lost, which results in this item getting stuck in the ImageTooltip processing pipeline. Limiting the supported schemes to the ones most commonly encountered ('file', 'http', 'https' and the schema-less local paths) is the only reliable method I have found to work around this particular bug in Qt.
|
@gedakc Your suggestions have been implemented and the squash has been performed, and now this PR is ready for merging. 😄 Edit: I had the same realization regarding local paths when I was working on it, but the code was getting rather complicated as it was. In the end, I decided to let it be as-is due to the fact that whatever format a project is exported to likely determines how a particular link/path/URI is going to be interpreted, and as such we will probably always have a certain degree of incorrectness here in comparison to the result the user ends up with after exporting a text / compiling their project. |
|
The squashed and cleaned up PR looks good to me, and also runs correctly when I tested (no crashes). I will merge this PR with the develop branch for inclusion in the next release of Manuskript. |
Last time I touched this code, I went in looking for a specific problem,
and came out with a fix specific to that issue. That fix was not wrong,
yet it hardly covered all the problems present in the code once one took
into account issues like:
The major part of it was fixed by using
QUrl.fromUserInput(), which doesthe exact kind of auto-detection for the nature of the resource that we
were in need of.
The rest was fixed by creating a whole lot of test cases and fixing
problems as I came across them, switching between Windows & Ubuntu about
a dozen times till I got it right. These will be included in the PR.
An unfortunate wart left behind is that I have not found a satisfying
way to ensure the processing pipeline remains clean. Some code paths
unfortunately cause the original request QUrl() to be changed during
processing, and without that it is very difficult to know what original
url we were fetching to begin with. (The odd QNetworkRequest behaviours
that I have observed make me hesitant about rewriting the processing
code right now when all my test cases finally work on Ubuntu & Windows.)
The test cases: Image link tooltip test sheet.txt
I'll see if I can get rid of that final wart tomorrow when I am properly awake and not sick of this code anymore. I put up the PR so that those involved in the original issue (@scribas & @gedakc ) can verify whether this patch indeed fixes all the issues they have come across when dealing with image tooltips.