On 2018-12-25 23:06-0000 António Rodrigues Tomé wrote:
Hi Alain sorry I haven carefully read up to the end all the README.developers file here the patch
Hi António: Thanks for those commits. My apologies in advance for the number of questions I have for you in this reply, but I would appreciate you answering all of them to help improve my overall knowledge concerning the Qt-related components of PLplot and also to improve the commit messages associated with your two commits. One issue I immediately noticed with your commits is both have only a one-line commit message, and those should be expanded with a following paragraph with details and two further Tested by: paragraphs. I can do the necessary editing of your commit messages here to make those changes, but in order to do that I will need additional information from you as detailed below for your two different commits. I. 0001-correction-in-QtRasterDevice-QtRasterDevice-to-fix-a.patch Your current summary line, "correction in QtRasterDevice::QtRasterDevice to fix a alpha problem in the raster qt Drivers" is too repetitive and also too vague. Here are two alternative suggestions: "Fix background transparency in the raster qt Drivers" or "Fix transparency in the raster qt Drivers" Let me know which of these you prefer. (Note I do plan to mention QtRasterDevice::QtRasterDevice in the explanatory paragraph which is why I dropped that phrase from the summary line.) The reason I used the "background" qualifier for the first alternative is I can find no problem with the non-background transparency in these devices. For example, without your fix I attach test_qt.png.1 (the first page of the example) generated with examples/c/x30c -dev pngqt -o test_qt.png -fam where that result clearly shows the effects of transparency for the various semi-transparent blocks displayed by that example and also agrees with the first page displayed at <http://plplot.org/examples.php?demo=30> which was generated with -dev pngcairo. However, if your result there (without your fix) on openSUSE is not the same, then this may be another case of openSUSE exposing bugs in the PLplot qt device driver better than Debian (i.e., Debian Qt fixups and workarounds may be more extensive than those from openSUSE). The reason I am asking about this in detail is one of your changes involved moving from QImage::Format_RGB32 ==> QImage::Format_ARGB32.
From the description at
<http://doc.qt.io/qt-5/qimage.html#Format-enum> it appears that is absolutely the right thing to do (unless you decide later to move to QImage::Format_ARGB32_Premultiplied for efficiency reasons, but that is obviously a separate issue). But from that description it seems our unfixed RGB32 format should not be able to produce the semitransparent results we see in the attached plot. But maybe Debian (and openSUSE?) works around that by automatically switching from RGB32 to ARGB32 if alpha information is provided? Your response to these questions and comments will help determine whether I drop "background" from the summary line and greatly improve the explanatory paragraph I need to write. Also, can you explain why you had to add fill(Qt::transparent); ? Does that mean in the unfixed version there was no background fill at all for these devices so Qt was falling back to some opaque background? I haven't tested this commit yet, but once I do that I plan to add the following "Tested by" paragraph. Tested by: Alan W. Irwin <air...@users.sourceforge.net> on Linux (Debian Testing) <followed by details of the tests I ran>. Could you give me those test details for yourself that I could add to a paragraph started by Tested by: António Rodrigues Tomé <antoniort...@users.sourceforge.net> on Linux (openSUSE version number?) <followed by the test details you supply> ? II. 0002-correction-in-initQtApp-to-allow-qt-drivers-to-be-ca.patch Do you agree with shortening your summary line from "correction in initQtApp to allow qt drivers to be called from a qt program" ==> Allow qt devices to be called from a qt program with initQtApp mentioned in the explanatory paragraph? That paragraph does not have to be too long, but can you explain to me why you need to increment appCounter one additional time? Is the problem that the devices are decrementing that somehow when your application is finished with them? If so, wouldn't a better solution be to specifically increment appCounter in each of the qt devices? The reason I am concerned about these appCounter details is I am pretty sure your current fix will add a memory leak for non-device Qt applications such as examples/c++/qt_example.cpp. That example already has a memory leak due to PlotWindow * win = new PlotWindow( Argc, Argv ); with no corresponding xwin delete, but when I attempted to fix that recently by deleting win that did not work because some other destructor currently destroys part of it (as far as I can tell from my limited Qt/C++ knowledge). Anyhow, I do not want to make the already bad situation for that example (which makes it sometimes segfault on exit) worse due to your appCounter fix. If as a result of my questions about appCounter above you decide to rework this commit, then when you do that please add an explanatory and "Tested by" paragraph to your commit message for your revised commit. But if you think your appCounter change in its present form does not introduce memory leaks for the no-device case (i.e., the present code changes for this commit are fine), then I will need information from you on how you tested your change to be added to a "Tested by:" paragraph for you. In either case I will also add one of those paragraphs for my own tests of this commit before pushing this commit or its revised version from you. Alan __________________________ Alan W. Irwin Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net); the Time Ephemerides project (timeephem.sf.net); PLplot scientific plotting software package (plplot.sf.net); the libLASi project (unifont.org/lasi); the Loads of Linux Links project (loll.sf.net); and the Linux Brochure Project (lbproject.sf.net). __________________________ Linux-powered Science __________________________
test_qt.png.1
Description: example showing good transparency for unfixed pngqt device
_______________________________________________ Plplot-devel mailing list Plplot-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/plplot-devel
