Merge lp:~mrqtros/ubuntu-docviewer-app/ubuntu-docviewer-app-adv-logging into lp:ubuntu-docviewer-app

Proposed by Roman Shchekin on 2016-01-16
Status: Needs review
Proposed branch: lp:~mrqtros/ubuntu-docviewer-app/ubuntu-docviewer-app-adv-logging
Merge into: lp:ubuntu-docviewer-app
Diff against target: 243 lines (+34/-19)
6 files modified
src/app/renderengine.cpp (+3/-1)
src/app/renderengine.h (+3/-0)
src/plugin/libreofficetoolkit-qml-plugin/lodocument.cpp (+11/-9)
src/plugin/libreofficetoolkit-qml-plugin/lodocument.h (+3/-0)
src/plugin/libreofficetoolkit-qml-plugin/loview.cpp (+11/-9)
src/plugin/libreofficetoolkit-qml-plugin/loview.h (+3/-0)
To merge this branch: bzr merge lp:~mrqtros/ubuntu-docviewer-app/ubuntu-docviewer-app-adv-logging
Reviewer Review Type Date Requested Status
Stefano Verzegnassi 2016-01-16 Needs Information on 2016-01-16
Jenkins Bot continuous-integration Approve on 2016-01-16
Review via email: mp+282846@code.launchpad.net

Commit message

New style of logging.

Description of the change

New style of logging.

To post a comment you must log in.
review: Approve (continuous-integration)

Hey Roman!
In order to go on with the review, I need to ask you some question.

1) I see you've kept the #ifdef's. Shouldn't they be removed?

   Having a look at the KDE wiki at:
       https://techbase.kde.org/Development/Tutorials/Debugging/Using_Error_Messages#Improving_Logging_Output

   It looks like we'd need two steps in order to enable debugging output:
       1) Uncomment a #define entry in config.h
       2) Enable logging for a specific category (by default they're all disabled)

    I'd suggest to remove all the #define's and #ifdef's, and eventually add e.g.:
        QLoggingCategory::setFilterRules(QStringLiteral("RenderEngine.debug = true"));

    somewhere inside the plugin.

2) How do we like to filter the debug log? By class as you propose (e.g. "RenderEngine", "LODocument", etc.) or by scope (e.g. "TileBenchmark", "TileManagement", "Zoom", etc.)

3) Some of the debug in LODocument
       e.g. qCDebug(LoDoc) << "Loading document...";

   is information output rather than debug (I badly used qDebug() instead of qInfo()).

   Do we still want to make it optional? Could we use qInfo() or qCInfo()?

   I'm thinking at a scenario where something unexpected happens, and a user opens a bug report on Launchpad and upload the application log.
   Those basic informations wouldn't be available, and we would have to ask her to enable logging[1] for being able to say which area could generate the issue, and then (eventually) asking to enable some further category again.

   I consider this output useful for orienting my focus at first. I might as well be wrong!

[1] How can that be easily done on Ubuntu Touch, since everything is so confined?

review: Needs Information
Roman Shchekin (mrqtros) wrote :

Let's discuss it then :)

1) I should explore possibilities... To be honest, I was satisfied with text in output during debug, I thought that it was enough for us...
2) Seems that we should filter our output with groups. RenderEngine itself is a good category, LoDocument and LoView can be rethinked... The using of class name can be very handy since we have not so much classes and all of them are "core".
3) There is a thin difference between info and debug, I think, so every case is discussable, but not critical

1) Yes! If we could remove those #ifdef's from the code, it would look more linear/less bulky(TM) and it would be a great "double win".
However, I'd like to have something similar to the current config.h, so that we can toggle a more verbose output by uncommenting a single line. Which could be a suitable place for that?

2) Agree! "RenderEngine" is a good category. Others could be:
   - "TileBenchmark" in LODocument paint functions
   - "TileManagement" in LOView, for providing output about creation/removal/changes about the tile map and the visible/buffer areas
   - "ZoomEvents" for the automatic zoom management and events triggered by a change in the zoom value[1]
   - "VerboseOutput" for general information (e.g. error warnings) and something else we could add in future

3) "Loading document ..." and "Document loaded successfully !" may stay always visible.
   LODocument::loadDocument() is the place where the magical things happen (all the LibreOffice stuff are loaded there, and many 'events' in the plugin depend on a successful/failure of its execution).
   Having those two lines at the beginning/end of the function is more confortable for me. :)

[1] See for example "Zoom value of tiles is different than the current zoom value. Erasing cache..."
    This string could also be filtered by a "TileManagement" category, but it's not possible to assign a string to multiple categories, for what I see.

Unmerged revisions

261. By Roman Shchekin on 2016-01-16

New style of logging.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/app/renderengine.cpp'
2--- src/app/renderengine.cpp 2015-12-12 10:06:55 +0000
3+++ src/app/renderengine.cpp 2016-01-16 13:18:10 +0000
4@@ -2,6 +2,8 @@
5 #include <QtConcurrent/QtConcurrent>
6 #include <QThread>
7
8+Q_LOGGING_CATEGORY(RndEng, "RenderEngine")
9+
10 RenderEngine* RenderEngine::s_instance = nullptr;
11
12 RenderEngine::RenderEngine():
13@@ -65,7 +67,7 @@
14 void RenderEngine::doNextTask()
15 {
16 #ifdef DEBUG_VERBOSE
17- qDebug() << " ---- doNextTask" << m_activeTaskCount << m_queue.count();
18+ qCDebug(RndEng) << " ---- doNextTask" << m_activeTaskCount << m_queue.count();
19 #endif
20
21 // Check for too much threads or empty queue.
22
23=== modified file 'src/app/renderengine.h'
24--- src/app/renderengine.h 2015-12-12 10:06:55 +0000
25+++ src/app/renderengine.h 2016-01-16 13:18:10 +0000
26@@ -8,10 +8,13 @@
27 #include <QQueue>
28 #include <QAtomicInt>
29 #include <QList>
30+#include <QLoggingCategory>
31
32 //#include "lodocument.h"
33 #include "rendertask.h"
34
35+Q_DECLARE_LOGGING_CATEGORY(RndEng)
36+
37 class RenderEngine : public QObject
38 {
39 Q_OBJECT
40
41=== modified file 'src/plugin/libreofficetoolkit-qml-plugin/lodocument.cpp'
42--- src/plugin/libreofficetoolkit-qml-plugin/lodocument.cpp 2015-12-14 00:40:55 +0000
43+++ src/plugin/libreofficetoolkit-qml-plugin/lodocument.cpp 2016-01-16 13:18:10 +0000
44@@ -30,6 +30,8 @@
45 #include <QElapsedTimer>
46 #endif
47
48+Q_LOGGING_CATEGORY(LoDoc, "LODocument")
49+
50 lok::Office *LODocument::s_office = nullptr;
51
52 LODocument::LODocument()
53@@ -62,11 +64,11 @@
54 // Load the document
55 void LODocument::loadDocument(const QString &pathName)
56 {
57- qDebug() << "Loading document...";
58+ qCDebug(LoDoc) << "Loading document...";
59 setError(LibreOfficeError::NoError);
60
61 if (pathName.isEmpty()) {
62- qDebug() << "Can't load the document, path is empty.";
63+ qCWarning(LoDoc) << "Can't load the document, path is empty.";
64 return;
65 }
66
67@@ -86,7 +88,7 @@
68
69 if (!s_office) {
70 setError(LibreOfficeError::LibreOfficeNotInitialized);
71- qDebug() << "[lok-qml]: LibreOffice not initialized.";
72+ qCWarning(LoDoc) << "[lok-qml]: LibreOffice not initialized.";
73 return;
74 }
75
76@@ -96,7 +98,7 @@
77
78 if (!m_lokDocument) {
79 setError(LibreOfficeError::DocumentNotLoaded);
80- qDebug() << "[lok-qml]: Document not loaded.";
81+ qCWarning(LoDoc) << "[lok-qml]: Document not loaded.";
82 return;
83 }
84
85@@ -106,7 +108,7 @@
86 Q_EMIT documentTypeChanged();
87
88 m_lokDocument->initializeForRendering();
89- qDebug() << "Document loaded successfully !";
90+ qCDebug(LoDoc) << "Document loaded successfully !";
91
92 return;
93 }
94@@ -173,7 +175,7 @@
95 Twips::convertPixelsToTwips(tileSize.height(), zoom));
96
97 #ifdef DEBUG_TILE_BENCHMARK
98- qDebug() << "Time to render the tile:" << renderTimer.elapsed() << "ms";
99+ qCDebug(LoDoc) << "Time to render the tile:" << renderTimer.elapsed() << "ms";
100 #endif
101
102 return result.rgbSwapped();
103@@ -209,7 +211,7 @@
104 0, 0, tWidth, tHeight);
105
106 #ifdef DEBUG_TILE_BENCHMARK
107- qDebug() << "Time to render the thumbnail:" << renderTimer.elapsed() << "ms";
108+ qCDebug(LoDoc) << "Time to render the thumbnail:" << renderTimer.elapsed() << "ms";
109 #endif
110
111 return result.rgbSwapped();
112@@ -241,7 +243,7 @@
113 bool LODocument::saveAs(QString url, QString format = QString(), QString filterOptions = QString())
114 {
115 if (!m_lokDocument) {
116- qDebug() << "No loaded document. It's not possible to save this file.";
117+ qCWarning(LoDoc) << "No loaded document. It's not possible to save this file.";
118 return false;
119 }
120
121@@ -254,6 +256,6 @@
122 {
123 delete m_lokDocument;
124 #ifdef DEBUG_VERBOSE
125- qDebug() << " ---- ~LODocument";
126+ qCDebug(LoDoc) << " ---- ~LODocument";
127 #endif
128 }
129
130=== modified file 'src/plugin/libreofficetoolkit-qml-plugin/lodocument.h'
131--- src/plugin/libreofficetoolkit-qml-plugin/lodocument.h 2015-12-14 00:40:55 +0000
132+++ src/plugin/libreofficetoolkit-qml-plugin/lodocument.h 2016-01-16 13:18:10 +0000
133@@ -19,6 +19,9 @@
134 #define LODOCUMENT_H
135
136 #include <QObject>
137+#include <QLoggingCategory>
138+
139+Q_DECLARE_LOGGING_CATEGORY(LoDoc)
140
141 #include "loerror.h"
142
143
144=== modified file 'src/plugin/libreofficetoolkit-qml-plugin/loview.cpp'
145--- src/plugin/libreofficetoolkit-qml-plugin/loview.cpp 2015-12-14 00:40:55 +0000
146+++ src/plugin/libreofficetoolkit-qml-plugin/loview.cpp 2016-01-16 13:18:10 +0000
147@@ -24,6 +24,8 @@
148 #include <QTimer>
149 #include <QtCore/qmath.h>
150
151+Q_LOGGING_CATEGORY(LoVi, "LOView")
152+
153 static qreal zoomValueToFitWidth;
154
155 static qreal getZoomToFitWidth(const qreal &width, int documentWidth)
156@@ -210,7 +212,7 @@
157 m_document->documentSize().width());
158
159 setZoomFactor(zoomValueToFitWidth);
160- qDebug() << "Adjust zoom to width - value:" << zoomValueToFitWidth;
161+ qCDebug(LoVi) << "Adjust zoom to width - value:" << zoomValueToFitWidth;
162 }
163
164 void LOView::updateViewSize()
165@@ -247,7 +249,7 @@
166 if (m_zoomFactor != zoomValueToFitWidth) {
167 setZoomFactor(zoomValueToFitWidth);
168
169- qDebug() << "Adjust automatic zoom to width - value:" << zoomValueToFitWidth;
170+ qCDebug(LoVi) << "Adjust automatic zoom to width - value:" << zoomValueToFitWidth;
171 return;
172 }
173 }
174@@ -260,7 +262,7 @@
175 clearView();
176
177 #ifdef DEBUG_VERBOSE
178- qDebug() << "Zoom value of tiles is different than the current zoom value. Erasing cache...";
179+ qCDebug(LoVi) << "Zoom value of tiles is different than the current zoom value. Erasing cache...";
180 #endif
181 }
182 }
183@@ -298,7 +300,7 @@
184
185 // Out of buffer - we should delete this item.
186 #ifdef DEBUG_VERBOSE
187- qDebug() << "Removing tile indexed as" << i.key();
188+ qCDebug(LoVi) << "Removing tile indexed as" << i.key();
189 #endif
190
191 RenderEngine::instance()->dequeueTask(sgtile->id());
192@@ -324,8 +326,8 @@
193 int bufferToHeight = qCeil(qreal(m_bufferArea.bottom()) / TILE_SIZE);
194
195 #ifdef DEBUG_VERBOSE
196- qDebug() << "Visible area - Left:" << visiblesFromWidth << "Right:" << visiblesToWidth << "Top:" << visiblesFromHeight << "Bottom:" << visiblesToHeight;
197- qDebug() << "Buffer area - Left:" << bufferFromWidth << "Right:" << bufferToWidth << "Top:" << bufferFromHeight << "Bottom:" << bufferToHeight;
198+ qCDebug(LoVi) << "Visible area - Left:" << visiblesFromWidth << "Right:" << visiblesToWidth << "Top:" << visiblesFromHeight << "Bottom:" << visiblesToHeight;
199+ qCDebug(LoVi) << "Buffer area - Left:" << bufferFromWidth << "Right:" << bufferToWidth << "Top:" << bufferFromHeight << "Bottom:" << bufferToHeight;
200 #endif
201
202 this->generateTiles(visiblesFromWidth, visiblesFromHeight, visiblesToWidth, visiblesToHeight, tilesPerWidth, tilesPerHeight);
203@@ -350,7 +352,7 @@
204 createTile(index, tileRect);
205
206 #ifdef DEBUG_VERBOSE
207- qDebug() << "Generating tile - Index:" << index << "X:" << x << "Y:" << y;
208+ qCDebug(LoVi) << "Generating tile - Index:" << index << "X:" << x << "Y:" << y;
209 #endif
210 }
211 }
212@@ -375,7 +377,7 @@
213 {
214 if (!m_tiles.contains(index)) {
215 #ifdef DEBUG_VERBOSE
216- qDebug() << "Creating tile indexed as" << index << "- Rect:" << rect;
217+ qCDebug(LoVi) << "Creating tile indexed as" << index << "- Rect:" << rect;
218 #endif
219
220 auto tile = new SGTileItem(rect, m_zoomFactor, RenderEngine::getNextId(), this);
221@@ -384,7 +386,7 @@
222 }
223 #ifdef DEBUG_VERBOSE
224 else {
225- qDebug() << "tile" << index << "already exists";
226+ qCDebug(LoVi) << "tile" << index << "already exists";
227 }
228 #endif
229 }
230
231=== modified file 'src/plugin/libreofficetoolkit-qml-plugin/loview.h'
232--- src/plugin/libreofficetoolkit-qml-plugin/loview.h 2016-01-07 11:23:50 +0000
233+++ src/plugin/libreofficetoolkit-qml-plugin/loview.h 2016-01-16 13:18:10 +0000
234@@ -22,6 +22,9 @@
235 #include <QSharedPointer>
236 #include <QQmlContext>
237 #include <QQmlEngine>
238+#include <QLoggingCategory>
239+
240+Q_DECLARE_LOGGING_CATEGORY(LoVi)
241
242 #include "loerror.h"
243 #include "lorendertask.h"

Subscribers

People subscribed via source and target branches