[Differential] [Changed Subscribers] D2295: improve logging for kscreen

[davidedmundson (David Edmundson)]

davidedmundson added inline comments.

INLINE COMMENTS

> log.cpp:30
> +Log* Log::sInstance = nullptr;
> +QtMessageHandler sDefaultMessageHandler = nullptr;
> +

why have it separate instead of a member of log?

> log.cpp:35
> +    QByteArray localMsg = msg.toLocal8Bit();
> +    if (QString::fromLocal8Bit(context.category).startsWith(QStringLiteral("kscreen"))) {
> +        Log::log(localMsg.constData(), context.category);

QLatin1String.

> log.cpp:46
> +
> +Log* Log::instance()
> +{

Question:
What's the threading policy on libkscreen?

This isn't reentrant (which might be fine)

> log.cpp:71
> +
> +    if (qEnvironmentVariableIsSet(logging_env)) {
> +        const QString logging_env_value = qgetenv(logging_env).constData();

you probably want to manually force the logging category to enable itself if this env var is set.

or get rid of this env var, and just check if the logging category is enabled.

> log.cpp:136
> +    const QString timestamp = QString::number(QDateTime::currentDateTime().toMSecsSinceEpoch());
> +    QString logMessage = QString("\n%1 ; %2 ; %3 : %4").arg(timestamp, _cat, instance()->context(), msg);
> +    QFile file(instance()->logFile());

why call instance() from inside a member function?
it'll always return this.

> log.cpp:137
> +    QString logMessage = QString("\n%1 ; %2 ; %3 : %4").arg(timestamp, _cat, instance()->context(), msg);
> +    QFile file(instance()->logFile());
> +    if (!file.open(QIODevice::Append | QIODevice::Text)) {

You're going to need a file lock round this aren't you?
The entire point is that two processes will be writing in here at the same time.

REPOSITORY
  rLIBKSCREEN KScreen Library

BRANCH
  sebas/log

REVISION DETAIL
  https://phabricator.kde.org/D2295

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: sebas, #plasma, bshah
Cc: davidedmundson, bshah, plasma-devel, ali-mohamed, jensreuterberg, abetts, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160728/422c88c6/attachment.html>