Prototype for customizable printing support.

Lubomir I. Ivanov neolit123 at gmail.com
Sun Apr 19 13:43:17 PDT 2015


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).

--------------------
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.

--------------------
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?

+#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.

+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).

--------------------
0003

diff --git a/display.h b/display.h
...
- ONEPERPAGE
+ ONEPERPAGE,
+ TEMPLATEBASED

not sure if this is the naming way to go. since all current layouts
will become template based, the one where the user has better control
of what is printed should be called either CUSTOM or USERDEFINED.

diff --git a/qt-ui/mainwindow.cpp b/qt-ui/mainwindow.cpp

- Printer *p = new Printer();
- p->print();
  PrintDialog dlg(this);

amends code added in the previous patch...basically, it should not be
added in the previous patch in the first place.
"git rebase -i "HEAD~N"" is quite handy.

diff --git a/qt-ui/printlayout.cpp b/qt-ui/printlayout.cpp
...

we need to remove the old printlayout code completely, eventually. i
suggest the following important aspect here...
keep the old printlayout and printdialog, add a new Subsurface macro
e.g. NEW_PRINTING and use it where needed (like we do with NO_MARBLE).

this way if Subsurface <insert_new_version_here> comes out we can
toggle OFF the macro and release the build without the WIP, new
printing stack.
what i mean by "remove the old printlayout code completely" is that we
can keep the class but it should contain only the Grantlee powered
code. there is a bit of decision making here to decide what class will
handle the rendering e.g. Printer vs PrintLayout vs DivePrinter
etc...i guess Printer will suffice.

--------------------
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.

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?

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?

--------------------
0006

diff --git a/printer.cpp b/printer.cpp
...
- int x = box.x() - viewPort.x();
- int y = box.y() - viewPort.y();
- int x1 = box.width() + x;
- int y1 = box.height() + y;
-
- QRect rec(x, y, box.width(), box.height());
- painter->fillRect(x, y, box.width(), box.height(), QColor("#CfC7C5"));
- painter->drawText(rec, Qt::AlignCenter, "Profile Area");
-}

again this patch removes code, which was previously added. ideally
this should be rebased.


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.

+//I think I am wasting time copying the object into memory.
+//May be there is a better way to genrate the objects.

i agree, the local dive class should be avoided completely. we already
have dives in both C (dive.h) and C++ (was it models.h?) form and
those should be used instead.

+ file = new QTemporaryFile(QDir::tempPath() + QDir::separator() +
"subsurface_XXXXXX.html");

again, i would say that we can avoid temporary files and write the
HTMLs in memory.

diff --git a/templatelayout.h b/templatelayout.h
...
+ TemplateLayout() {};
+ ~TemplateLayout(){
+ delete m_engine;
+ };

constructor and destructor definitions should not be in the headers.
declarations only.

--------------------
0007

diff --git a/printer.cpp b/printer.cpp
...
-QWebView *webView;
-

the patch does more that what is written in the description.
in general cleanup patches should be separate. if the series add then
remove the same hunks of code before they enter master the series
should be rebased, instead!

also see my temp. files related comments from above.

-------
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?

-------
0009

can be rebased.

-------

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.

lubomir
--


More information about the subsurface mailing list