Fix incorrect logger output for non-Latin strings. #2925

allexzander · 2021-02-11T21:25:03Z

Signed-off-by: allexzander [email protected]

allexzander · 2021-02-11T21:28:08Z

Logging of non-Latin named characters outputs garbage (confirmed with Cyrillic and Chinese at least), making it hard to study sync bug reports for such languages. This is an attempt to fix that.

Especially looking forward to suggestions on how to do it better from @er-vin.

er-vin

Not sure my comments are really on point, they're more questions to ask ourselves on such tasks. I think I still lack a bit of context on that one. Or my brain is underpowered by inactivity... can't be excluded. :-)

src/common/ownsql.h

er-vin · 2021-02-12T08:30:08Z

src/gui/main.cpp

@@ -73,6 +73,9 @@ int main(int argc, char **argv)
 #endif
    OCC::Application app(argc, argv);

+    QTextCodec *codec1 = QTextCodec::codecForName("UTF-8");


Does that really makes sense outside of Windows? AFAIK everyone else is using UTF-8 by default now.

@er-vin Might be not-needed outside of Windows, so conditional compilation could help. On Windows, though, adding this - helps to make logs written via qDebug, qCInfo, and others, readable in the .log files later.

I'm concerned about this though... because we don't change the locale but then we enforce a different codec than the "right" codec for that locale... If it's "just" about the log file because we want to make sure it's always encoded in UTF-8 (because indeed right now it will be encoded in whatever codec required by the user locale) then I'd only enforce UTF-8 on the codec used in the QTextStream of Logger. This would have way less side effects.

And actually now that I think of it, this might be the only change you need... if I'm not mistaken, all QByteArrays written to the log file at that point would be forcibly interpreted as UTF-8.

And actually now that I think of it, this might be the only change you need... if I'm not mistaken, all QByteArrays written to the log file at that point would be forcibly interpreted as UTF-8.

@er-vin Works in most scenarios but fails for the SQLite case, so changes to bindValue and qCInfo(lcDb) << "Updating file record for path:" << QString::fromUtf8(record._path) << "inode:" << record._inode are still needed

@er-vin Unless, I could go with changing QByteArray to QString for paths everywhere.

And actually now that I think of it, this might be the only change you need... if I'm not mistaken, all QByteArrays written to the log file at that point would be forcibly interpreted as UTF-8.

@er-vin Works in most scenarios but fails for the SQLite case, so changes to bindValue and qCInfo(lcDb) << "Updating file record for path:" << QString::fromUtf8(record._path) << "inode:" << record._inode are still needed

OK, then I was likely wrong.

@er-vin Unless, I could go with changing QByteArray to QString for paths everywhere.

Well, that'd be a much bigger and error prone endeavor. I'd keep that for much later, master doesn't need this right now. ;-)

er-vin · 2021-02-12T08:31:43Z

src/common/syncjournaldb.cpp

@@ -909,7 +909,7 @@ bool SyncJournalDb::setFileRecord(const SyncJournalFileRecord &_record)
        }
    }

-    qCInfo(lcDb) << "Updating file record for path:" << record._path << "inode:" << record._inode
+    qCInfo(lcDb) << "Updating file record for path:" << QString::fromUtf8(record._path) << "inode:" << record._inode


I think what you want is fromLocal8Bit? This is indeed what we should use for QByteArrays coming from the system before display.... that said I'm unsure, could be processed before and we already know it's utf8?

@er-vin fromLocal8Bit gives garbage. This has been processed before, and yes we know it's UTF-8. It's coming from QByteArray csync_file_stat_t::path which was set to QString::fromWCharArray(handle->ffd.cFileName).toUtf8 before.

OK. So we're good with utf8 indeed.

Note: I'm wondering if you might want to do the same with the _e2eMangleName below, this is also a path stored as QByteArray... you might want to check that too.

allexzander · 2021-02-12T08:48:54Z

Not sure my comments are really on point, they're more questions to ask ourselves on such tasks. I think I still lack a bit of context on that one. Or my brain is underpowered by inactivity... can't be excluded. :-)

@er-vin

The context can be seen when browsing log files of a Desktop Client. Here is one of such examples:

2021-02-10 17:19:48:408 [ info nextcloud.sync.csync.reconciler ]: INSTRUCTION_REMOVE client file: Ãðàôèêè ïî ó÷àñòêàì/!Ñëóæáà äîêóìåíòàöèè

As you can see, the line after client file: looks some kind of gibberish. So, the PR is about fixing such cases. In fact, it works right now, but I am not sure if that's the proper way of fixing it. And yes, this happens on Windows.

src/libsync/syncfileitem.cpp

allexzander · 2021-02-15T20:01:04Z

@er-vin If you got a spare minute - feel free to add some nitpicks :)

er-vin · 2021-02-15T20:28:15Z

src/common/syncjournalfilerecord.h

@@ -55,6 +55,8 @@ class OCSYNC_EXPORT SyncJournalFileRecord
    bool isDirectory() const { return _type == ItemTypeDirectory; }
    bool isFile() const { return _type == ItemTypeFile || _type == ItemTypeVirtualFileDehydration; }
    bool isVirtualFile() const { return _type == ItemTypeVirtualFile || _type == ItemTypeVirtualFileDownload; }
+    QString pathFromUtf8() const { return QString::fromUtf8(_path); }


For a "fromSomething" I'd expect the "Something" as parameter while this takes no parameter. What about "pathAsString()"? I'm even wondering if just "path()" would be enough... since we were toying with the idea of switching it all to QString instead, having such a QString path() const would be a nice API to slowly port against and when _path isn't used directly anymore it's a sign it could be switched to QString. Sounds like a potentially nice strategy for such a changer over a longer period of time.

er-vin · 2021-02-15T20:28:20Z

src/common/syncjournalfilerecord.h

@@ -55,6 +55,8 @@ class OCSYNC_EXPORT SyncJournalFileRecord
    bool isDirectory() const { return _type == ItemTypeDirectory; }
    bool isFile() const { return _type == ItemTypeFile || _type == ItemTypeVirtualFileDehydration; }
    bool isVirtualFile() const { return _type == ItemTypeVirtualFile || _type == ItemTypeVirtualFileDownload; }
+    QString pathFromUtf8() const { return QString::fromUtf8(_path); }
+    QString e2eMangledNameFromUtf8() const { return QString::fromUtf8(_e2eMangledName); }


Signed-off-by: allexzander <[email protected]>

nextcloud-desktop-bot · 2021-02-16T08:12:11Z

AppImage file: Nextcloud-PR-2925-4d66d209bc4d46e42a97ba20cda908be84702093-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

allexzander · 2021-02-17T12:52:48Z

/backport to stable-3.1

backportbot-nextcloud · 2021-02-17T12:53:25Z

The backport to stable-3.1 failed. Please do this backport manually.

allexzander requested review from er-vin, camilasan and FlexW February 11, 2021 21:25

er-vin reviewed Feb 12, 2021

View reviewed changes

allexzander marked this pull request as ready for review February 15, 2021 18:22

allexzander force-pushed the fix-incorrect-logger-output-for-non-latin-strings branch from 32d1f0e to 414e792 Compare February 15, 2021 18:23

allexzander requested a review from er-vin February 15, 2021 18:23

allexzander changed the title ~~Fix incorrect logger output for non-latin strings.~~ Fix incorrect logger output for non-Latin strings. Feb 15, 2021

camilasan approved these changes Feb 15, 2021

View reviewed changes

camilasan reviewed Feb 15, 2021

View reviewed changes

src/libsync/syncfileitem.cpp Outdated Show resolved Hide resolved

er-vin reviewed Feb 15, 2021

View reviewed changes

Fix incorrect logger output for non-latin strings.

4d66d20

Signed-off-by: allexzander <[email protected]>

allexzander force-pushed the fix-incorrect-logger-output-for-non-latin-strings branch from cb0ae42 to 4d66d20 Compare February 16, 2021 07:30

allexzander requested a review from er-vin February 16, 2021 07:38

er-vin approved these changes Feb 16, 2021

View reviewed changes

allexzander merged commit e6ec648 into master Feb 16, 2021

allexzander deleted the fix-incorrect-logger-output-for-non-latin-strings branch February 16, 2021 14:13

backportbot-nextcloud bot added the backport-request label Feb 17, 2021

allexzander removed the backport-request label Feb 17, 2021

allexzander restored the fix-incorrect-logger-output-for-non-latin-strings branch February 17, 2021 12:52

nextcloud deleted a comment from backportbot-nextcloud bot Feb 17, 2021

backportbot-nextcloud bot added the backport-request label Feb 17, 2021

allexzander deleted the fix-incorrect-logger-output-for-non-latin-strings branch February 17, 2021 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect logger output for non-Latin strings. #2925

Fix incorrect logger output for non-Latin strings. #2925

allexzander commented Feb 11, 2021

allexzander commented Feb 11, 2021

er-vin left a comment

er-vin Feb 12, 2021

allexzander Feb 12, 2021

er-vin Feb 12, 2021

er-vin Feb 12, 2021

allexzander Feb 12, 2021

allexzander Feb 12, 2021

er-vin Feb 13, 2021

er-vin Feb 13, 2021

er-vin Feb 12, 2021

allexzander Feb 12, 2021

er-vin Feb 12, 2021

er-vin Feb 12, 2021

allexzander commented Feb 12, 2021

allexzander commented Feb 15, 2021

er-vin Feb 15, 2021

er-vin Feb 15, 2021

nextcloud-desktop-bot commented Feb 16, 2021

allexzander commented Feb 17, 2021

backportbot-nextcloud bot commented Feb 17, 2021

Fix incorrect logger output for non-Latin strings. #2925

Fix incorrect logger output for non-Latin strings. #2925

Conversation

allexzander commented Feb 11, 2021

allexzander commented Feb 11, 2021

er-vin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allexzander commented Feb 12, 2021

allexzander commented Feb 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nextcloud-desktop-bot commented Feb 16, 2021

allexzander commented Feb 17, 2021

backportbot-nextcloud bot commented Feb 17, 2021