Prototype for customizable printing support.

Tue Apr 21 11:48:40 PDT 2015

On 04/19/2015 10:43 PM, Lubomir I. Ivanov wrote:
> On 16 April 2015 at 01:14, Gehad Elrobey <gehadelrobey at gmail.com> wrote:
>> Hello all,
>>
>> As we are approaching gsoc community bonding period so I thought that a
>> small prototype may be the best way to discuss my thoughts with the
>> community.
>> I sent a group of patches with a very small but full working prototype.
>>
>> - I added Grantlee library to subsurface code base (thanks for the cmake
>> build environment it made my life much easier as I don't know qmake at all)
>> - I create a simple Grantlee template that prints two dives per page.
>> - the TemplateLayout class iterates the selected dives and interface with
>> Grantlee backend.
>> - QWebview and QPainter are used to render the html file living in a
>> QTemporaryFile object.
>> - finally I added the "print using templates" option in the print dialog.
>>
>> Note: I tested this on Linux only, so there may be problems on other
>> platforms.
>> also many exceptions still need to be handled this is only a fast prototype
>> for discussion.
>>
>> So you may test this and tell me what do you think?
> 
> 
> hello Gehad,
> 
> here are some generic comments on the patches (+ some more important
> structural ones!).
> i won't be able to provide more until i'm able to build; for some
> reason that UIC error still pops out (the one i explained in the the
> previous email).
> 

Lubomir,

I have attached a new series of cleaner patches, and I have considered
your remarks.

> --------------------
> 0001
> 
> it won't be a bad idea to use the same/similar coding style across all
> formats, being HTML, XML, CSS, JS.
> this includes whitespace indentation of realtabs, spaces after ":" in
> class properties, etc.
> 

Fixed in the attached patches.

> --------------------
> 0002
> this patch should be split into more...
> 
> diff --git a/printer.cpp b/printer.cpp
> 
> for the Printer class, have you considered our plans to use the same
> backend for the facebook share?
> 

I plan to add a new constructor to the Printer class with other paint
devices, also different options can be set for each rendering device.

> +#define A4_300DPI_WIDTH 2480
> +#define A4_300DPI_HIGHT 3508
> 
> i don't think we need to hardcode A4 or any other page sizes in the
> render related class files. the class files should be able to render
> to any size.
> what the printer setup (user setup) returns is of importance and what
> we are interested in.
> 

I am going to add TemplateEdit class that will handle the user setup and
extra printing settings which I didn't implement yet, so these values
exist in this prototype only.

> +QWebView *webView;
> 
> global variables are not really desired, that should be a member of some class.
> 
> + int Pages = ceil( (float)
> webView->page()->mainFrame()->contentsSize().rheight() /
> A4_300DPI_HIGHT);
> + for (int i =0; i < Pages;i++) {
> 
> coding style in the for() loop.
> 
> +void Printer::print()
> +{
> + QUrl qurl("file:///tmp/dives.html");
> 
> there is no such path on Windows. we should avoid temporary files and
> load the HTML from memory.
> 
> diff --git a/printer.h b/printer.h
> ....
> +public:
> + Printer() {
> +
> + }
> 
> we should not have definitions in the header even if the constructor
> is empty. this is code for the .cpp file.
> 
> +private:
> + void putFrame(QRect box, QRect viewPort, QPainter *painter);
> +private
> +slots:
> + void render(bool ok);
> 
> coding style!  accessors groups should be separated by new lines,
> ideally. the extra "private before "slots:" is redundant.
> 
> diff --git a/qt-ui/mainwindow.cpp b/qt-ui/mainwindow.c
> ...
> + Printer *p = new Printer();
> + p->print();
>   PrintDialog dlg(this);
> 
> i don't like the fact that MainWindow is aware of the existence of the
> Printer class. it should only know of PrintDialog, where an instance
> of Printer should be maintained (if that is the structure we keep on
> using. please see bellow).
> 

Fixed in the attached patches

> --------------------
> 0004
> 
> diff --git a/printer.cpp b/printer.cpp
> 
> - QPrinter printer;
> - printer.setOutputFormat(QPrinter::PdfFormat);
> - printer.setOutputFileName("/home/dive.pdf");
> - printer.setFullPage(true);
> - printer.setOrientation(QPrinter::Portrait);
> - printer.setPaperSize(QPrinter::A4);
> - printer.setPrintRange(QPrinter::AllPages);
> - printer.setResolution(300);
> -
> 
> using an existing instance should have been the case in the first
> place. this patch removes code which was previously added in the
> series. it would be best to rebase the series.
> 
> in the train of though for using existing render targets (e.g.
> QPrinter) i'm sure we can use QPaintDevice pointers as the rendering
> class should not care if said target is a QPrinter or a QPixmap (for
> the facebook) share. my point here is that the Printer class (or
> DivePrinter, or PrintLayout) should be a class which renders dives on
> surface / render target and only cares about it's dimensions. ideally,
> Printer should not be aware if the render target is a QPrinter.

as I mentioned above I think social network sharing and prints will be
handled slightly different so we can add different constructors for the
printer class with special flags so a social network printer is passed a
QPixmap while paper printer is passed a QPrinter.

> i'm pretty sure the diagram in your proposal followed logic which was similar.
> 
> --------------------
> 0005
> 
> the cmake setup for Granlee is a bit of an issue. i need to be able to
> test this on my end if i want to mentor this GSOC project in the first
> place. :-(
> i may have to add the option GRANTLEE_FROM_PKGCONFIG. please have a
> look at some of my recent patches that are now in master, so that you
> can understand what i mean.
> 
> +GET_FILENAME_COMPONENT(Grantlee_PLUGIN_DIR ${Grantlee_PLUGIN_DIR} PATH)
> +GET_FILENAME_COMPONENT(Grantlee_PLUGIN_DIR ${Grantlee_PLUGIN_DIR} PATH)
> +GET_FILENAME_COMPONENT(Grantlee_PLUGIN_DIR ${Grantlee_PLUGIN_DIR} PATH)
> 
> why add the same line multiple times? is this something weird that
> cmake requires?

I removed these lines as apparently we don't need any Grantlee Plug-ins now.

> SET(QT_LIBRARIES Qt5::Core Qt5::Concurrent Qt5::Widgets Qt5::Network
> Qt5::WebKitWidgets Qt5::PrintSupport Qt5::Svg Grantlee5::Templates)
> 
> we don't have Marble (another Qt lib) in this list. why add Grantlee here?
> wouldn't it be better to keep this list for Qt built-in libraries only?
> 

Fixed in the attached patches

> diff --git a/templatelayout.cpp b/templatelayout.cpp
> ...
> 
> ah, i see. so this is the class that will provide the Grantlee related work.
> i can't comment much on that ATM, but i do understand the need for a
> Grantlee specific class. i think of it the following way - ideally
> only a *single* class in the Subsurface code base should be aware that
> we are using Grantlee.
> 

templatLayout is the only class that will need to call Grantlee.

> -------
> 0008
> 
> diff --git a/grantlee_paths.h.cmake b/grantlee_paths.h.cmake
> +#define GRANTLEE_PLUGIN_PATH "@Grantlee_PLUGIN_DIR@"
> 
> i don't understand how this work, Grantlee wise. can you please
> explain in detail?

This passes Grantlee plugin dir path dynamically to the Grantlee engine,
I removed it from the attached patches, as we don't need grantlee
plugins in the meantime.

> 
> that's all for now.
> please ask any questions that you may have or make sure to point any
> of my comments that you don't agree with.
> 

Thanks for your detailed review.

-- 
regards,
Gehad elrobey

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Create-two-dives-per-page-grantlee-template.patch
Type: text/x-patch
Size: 3226 bytes
Desc: not available
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20150421/857ec8b6/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-CMAKE-Require-Grantlee-library.patch
Type: text/x-patch
Size: 2716 bytes
Desc: not available
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20150421/857ec8b6/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Add-TemplateLayout-class.patch
Type: text/x-patch
Size: 5526 bytes
Desc: not available
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20150421/857ec8b6/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Add-Printer-class-that-holds-the-rendering-logic.patch
Type: text/x-patch
Size: 3332 bytes
Desc: not available
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20150421/857ec8b6/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Integration-with-subsurface-printing-dialog.patch
Type: text/x-patch
Size: 7113 bytes
Desc: not available
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20150421/857ec8b6/attachment-0009.bin>