ubuntu-touch-coreapps-reviewers team mailing list archive

[Stefano Verzegnassi <stefano92.100@xxxxxxxxx>]

Review: Needs Information

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?

-- 
https://code.launchpad.net/~mrqtros/ubuntu-docviewer-app/ubuntu-docviewer-app-adv-logging/+merge/282846
Your team Ubuntu Document Viewer Developers is subscribed to branch lp:ubuntu-docviewer-app.