Windows: Portable file check inside ctor not working as intended

[DoumanAsh]

Current usage of isWritable on Windows will result in directory being always read only (if user installs portable installation on NTFS volume)

Here info on how to enable NTFS permission checking actually https://doc.qt.io/qt-6/qfileinfo.html#ntfs-permissions But I would suggest just to disable it all together on windows and let it fail if user actually marks folder as read only

Code in question: https://github.com/kvirc/KVIrc/blob/master/src/kvirc/kernel/KviApplication_setup.cpp#L97C35-L97C45

[ctrlaltca]

Hi, thank you for this ticket. As far as i understand, the problem here is that code will only check if the directory is not readonly instead of also checking specific NTFS permissions for the user running the executable. I think checking if the directory is readonly is not perfect, but it's still useful. Why should we remove that check?

[DoumanAsh]

This directory, if created, will be most likely created my KVirc itself. So in most cases it will be pointless to check as otherwise it would mean user tampered with his installation.

I understand if you want to keep it I'm just pointing out that at the current moment it is not very reliable.

I think it would be maybe simpler to create temp file for test purpose instead though Folder permissions are flimsy on windows unfortunately 😩

[DoumanAsh]

I will test if QNtfsPermissionCheckGuard works on windows or otherwise make myself local build without this check to remove setup window

[DoumanAsh]

Ok so I tried QNtfsPermissionCheckGuard and it is not helping as expected I believe it has to do with the fact that permission model is broken for folders on Windows

So next I will try a more simpler approach of just creating temp file

    auto temp_file = QString("%1%2%3").arg(m_szLocalKvircDir).arg(KVI_PATH_SEPARATOR_CHAR).arg(".kvi-write-test");
    QFile temp_file_fd(temp_file);
    if (!temp_file_fd.open(QIODevice::ReadWrite)) {
        return false;
    }
    temp_file_fd.remove();

[DoumanAsh]

It seems I made wrong assumption and there is something fundamentally wrong with checking file metadata This check actually fails before it even tries to verify Settings folder (portable file exists of course) https://github.com/kvirc/KVIrc/blob/master/src/kvirc/kernel/KviApplication_setup.cpp#L301

[DoumanAsh]

@ctrlaltca So I found that in my case portable check was not working rather than setting check So after I made this patch, my KVirc behaved as expected https://github.com/DoumanAsh/KVIrc/commit/67adf7ca8d9f4483a433cce0735f118e4cf97e94

I'm not sure if it is bug in QT or some unexpected behavior, but for whatever reason current check in KviApplication ctor doesn't find file correctly which I assume means g_pApp->applicationDirPath() points to wrong path? At least that's what my experiments led me to believe

[DoumanAsh]

On side note, I'm not well versed in QT, but I suspect you cannot run this function in your app ctor as QCoreApplication might not be initialized yet? (due to compiler bug or what not) https://codebrowser.dev/qt6/qtbase/src/corelib/kernel/qcoreapplication.cpp.html#2391

[ctrlaltca]

@DoumanAsh thank you for debugging your issue. Your reasoning make sense. I remember portable mode working on old Qt5 builds, maybe something changed in Qt6, anyway we'd better implement your patch to avoid issues. Do you mind opening a PR with the change (excluding .github/workflows/build_windows.yml) so we can properly attribute credits to yourself? Thank you

[DoumanAsh]

Sure, I'll make it Just a note I heard that you're actually not supposed to use QApplication as base class in first place so most likely it is intended not to be possible to call its methods within child class ctor