Refresh Windows download dialog progress when hydrating a placeholder #2981

allexzander · 2021-03-09T20:52:34Z

Documentation is here: https://docs.microsoft.com/en-us/windows/win32/shell/integrate-cloud-storage

Signed-off-by: allexzander blackslayer4@gmail.com

er-vin

Sorry... lacking time so only did a quick glance. There's clearly more things to deal with in there than what I had the time to point out.

er-vin · 2021-03-10T18:07:26Z

src/common/vfs.h

@@ -44,6 +44,12 @@ struct OCSYNC_EXPORT VfsSetupParams
     */
    QString filesystemPath;

+    // Folder display name in Windows Explorer
+    QString displayName;


I think you can get to that information (same thing for alias) from FolderMan by using the filesystemPath. It's better to get fresh information straight from the Folder object each time because they can change over time after that VfsSetupParams has been created.

@er-vin I've been trying to avoid having a dependency to a FolderMan. We already have a dependency from GUI to VFS, so I thought two-way dependency is not a very good idea?

@er-vin One other thing is - I actually don't need a refreshed information about the alias. I am just using an alias to be able to set multiple sync folders for the same user. So it is used as the last part of syncRootId so each folder can have the same user account and Windows Sid, but different alias at the end (!0, !1, etc.). From what I have noticed - the alias is never changed. Unless the folder is removed and then another folder is added. Then the alias is reused. But, I am removing the Registry key for that folder that's being removed anyway. When unregistering the Registry key - I STILL need the same alias that I've used when registering it. Otherwise, the Registry key I am trying to unregister is noexisting.

er-vin · 2021-03-10T18:16:06Z

src/gui/folderman.cpp

@@ -76,6 +76,9 @@ FolderMan::FolderMan(QObject *parent)
    connect(AccountManager::instance(), &AccountManager::removeAccountFolders,
        this, &FolderMan::slotRemoveFoldersForAccount);

+    connect(AccountManager::instance(), &AccountManager::accountRemoved,


Hm I think this is emitted at application exit too... if I'm correct you don't want to call vfs->unregisterFolder() there but only vfs->stop(). Otherwise IIRC that would kill all the stored placeholder information in the folder every time you exit.

I think what you're after is about unregistering folders for accounts the user deleted? If yes how is that related to what this PR states? Just trying to connect the dots here. :-)

Hm I think this is emitted at application exit too... if I'm correct you don't want to call vfs->unregisterFolder() there but only vfs->stop(). Otherwise IIRC that would kill all the stored placeholder information in the folder every time you exit.

I think what you're after is about unregistering folders for accounts the user deleted? If yes how is that related to what this PR states? Just trying to connect the dots here. :-)

@er-vin No. Absolutely no. I've been debugging it and it is not emitted. Only emitted when the account is removed.

Another question - when we are removing the account connection - why would we need to store placeholder information? My idea was - they are now invalid and we will instead register everything again when adding a new account connection (which actually happens now).

@er-vin No. Absolutely no. I've been debugging it and it is not emitted. Only emitted when the account is removed.

This is surprising, I wonder if something else is broken because this is clearly emitted from AccountManager::shutdown() which is called from the Application dtor.

Another question - when we are removing the account connection - why would we need to store placeholder information? My idea was - they are now invalid and we will instead register everything again when adding a new account connection (which actually happens now).

Sure, when the user removes an account there's no need to keep placeholder information. My comment was more from the prospect of my first comment that I expect the folder removal code to be triggered on application exit which we wouldn't want. Placeholder information needs to stay put even if the application is exited.

@er-vin Indeed, emit signal is there. But, the _folderMap is empty by the time it gets emitted, that's why I am not hitting it. Anyways, thx for this comment, I definitely need to change this part to make it more reliable. Now, it works, just because _folderMap is cleaned up in advance, but, it will break if something changes in the cleanup logic for _folderMap.

er-vin · 2021-03-10T18:18:09Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

@@ -35,6 +36,7 @@ Q_LOGGING_CATEGORY(lcCfApiWrapper, "nextcloud.sync.vfs.cfapi.wrapper", QtInfoMsg
      FIELD_SIZE( CF_OPERATION_PARAMETERS, field ) )

 namespace {
+QMap<QString, QString> registeredSyncRootKeys;


Please no non-POD static. This is always asking for trouble and especially here because the code in this file tends to be reached by different threads!

Besides (I still need to read the rest of this PR) but I'm so far kind of surprised you need this at all.

@er-vin Then, I need to convert this file into a class instead of just a namespace. I need to store registry paths that I've created so I can then remove them when I need to unregister a sync root. Another way - is to add parameters for "unregister" function, but, then, I will also need to change the interface for _vfs->unregister(), just didn't want it to affect other platforms.

No, no, you could go for Q_GLOBAL_STATIC (+ eventually a mutex to protect access if that's used on a code path from secondary thread, which in the context of that file would be anything called from one of the callbacks), it's having QMap straight as global static which will cause issues.

you could go for Q_GLOBAL_STATIC (+ eventually a mutex to protect access

@er-vin Done. Hopefully, this is what you had in mind?

er-vin · 2021-03-10T18:20:27Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

+OCC::Result<void, QString> OCC::CfApiWrapper::registerSyncRoot(const QString &path, const QString &providerName, const QString &providerVersion, const QString &folderAlias, const QString &displayName, const QString &accountDisplayName)
+{
+    // even if we fail to register our sync root with shell, we can still proceed with using the VFS
+    Q_ASSERT(createSyncRootRegistryKeys(providerName, folderAlias, displayName, accountDisplayName, path));


Dangerous! This won't get executed in release builds.

Dangerous! This won't get executed in release builds.

@er-vin In fact - I don't want it executed in Release builds. Why would I? I don't want the app to crash just because of non-working progress. But, now, I think, should I just keep the qcWarning() instead of assert in this case?

My point was that: createSyncRootRegistryKeys won't even get called in release build. It's not just about asserting or not, the whole expression is compiled out.

My point was that: createSyncRootRegistryKeys won't even get called in release build. It's not just about asserting or not, the whole expression is compiled out.

@er-vin Indeed. Fixed.

er-vin · 2021-03-10T18:22:03Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

 {
+    Q_ASSERT(deleteSyncRootRegistryKey(path));


Ditto, won't be executed in release builds

@er-vin In fact, I don't need it executed in a release. But, maybe, I should just log a warning here instead? This code is not intended to crash and the app is planned to continue working even if it fails.

Ditto, won't be executed in release builds

@er-vin Fixed.

er-vin · 2021-03-10T18:22:42Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

@@ -346,7 +467,7 @@ OCC::Result<OCC::CfApiWrapper::ConnectionKey, QString> OCC::CfApiWrapper::connec
    if (result != S_OK) {
        return QString::fromWCharArray(_com_error(result).ErrorMessage());
    } else {
-        return key;
+        return std::move(key);


Dont use std::move on returns, it has no effect (I oversimplify a bit, it's often worse than that).

Dont use std::move on returns, it has no effect (I oversimplify a bit, it's often worse than that).

@er-vin Well, it gives a warning here every time I build the project. So, I decided to fix it. Now - there is no warning anymore, and the code still works. So, I guess - it's a win? :) Or, no?

Hm, odd. Which warning is that? Sounds like a false positive. I seem to remember Visual Studio has kind of an old bug regarding this but I can't remember the specifics. Thing is that can prevent some return optimizations to use std::move on returns.

Hm, odd. Which warning is that? Sounds like a false positive. I seem to remember Visual Studio has kind of an old bug regarding this but I can't remember the specifics. Thing is that can prevent some return optimizations to use std::move on returns.

@er-vin This is coming from the Qt Creator:
cfapiwrapper.cpp:493:16: error: call to implicitly-deleted copy constructor of 'OCC::CfApiWrapper::ConnectionKey' cfapiwrapper.h:38:44: note: copy constructor of 'ConnectionKey' is implicitly deleted because field '_data' has a deleted copy constructor memory:2551:5: note: 'unique_ptr' has been explicitly marked deleted here result.h:34:14: note: passing argument to parameter 'value' here

It's because of std::unique_ptr<void, void(*)(void *)> _data; within the class OWNCLOUDSYNC_EXPORT ConnectionKey.

@er-vin

What I can also, alternatively is:

replace auto key = ConnectionKey(); with OCC::Result<OCC::CfApiWrapper::ConnectionKey, QString> key = ConnectionKey();

Then, std::move() is not needed on key.

Ah OK. Got it now. This is because of the type elision. I think the better fix is: return { std::move(key) };

This should result in the same generated code but would avoid the confusion on read that the move is not being for what's returned. This is clearly what tripped me up here. :-)

er-vin · 2021-03-10T18:23:26Z

src/libsync/vfs/cfapi/vfs_cfapi.cpp

@@ -89,7 +89,7 @@ void VfsCfApi::stop()
 void VfsCfApi::unregisterFolder()
 {
    const auto localPath = QDir::toNativeSeparators(params().filesystemPath);
-    const auto result = cfapi::unegisterSyncRoot(localPath);
+    const auto result = cfapi::unregisterSyncRoot(localPath);


too bad this typo fix wasn't in a separate commit, that clouds a bit the review

@er-vin Indeed? Well, I thought it is just so small so it doesn't deserve all the hassle with new PR + CI + merge? Anyways, can we let it slide just this time?

No no, I just meant separate commit in this PR. When there's more than one commit I review each one separately which helps quite a lot.

But yes don't bother changing it now.

allexzander · 2021-03-10T18:55:03Z

Sorry... lacking time so only did a quick glance. There's clearly more things to deal with in there than what I had the time to point out.
@er-vin
I am planning to finish it for 3.2.1 so, I guess we still have some time :)

allexzander · 2021-03-12T15:34:49Z

@er-vin One important question that blocks me from proceeding is - should I include Folderman into CfApiWrapper? The Folderman is in GUI, while the CfApiWrapper is in LibSync. The VFS is already included in the Folder, which is also GUI. Should I include them in both directions? Creating a double-connection? Or, maybe, it is better to keep passing the folder alias and folder display name via the VFS parameters when registering? And also, I am going to need to pass some parameters to virtual void unregisterFolder() = 0; hence changing the interface. Only this, allows me to get rid of static QMap.

Also, what about accountDisplayName? I need that value too for the Registry key. Should I also include AccountManager into the CfApiWrapper? I can as well pass the accountDisplayName into a register/unregister function as I do now (except for the unregister, I am going to need to add these parameters into the interface).

er-vin · 2021-03-12T16:51:39Z

@er-vin One important question that blocks me from proceeding is - should I include Folderman into CfApiWrapper? The Folderman is in GUI, while the CfApiWrapper is in LibSync. The VFS is already included in the Folder, which is also GUI. Should I include them in both directions? Creating a double-connection?

Yes indeed, this is a problem.

Or, maybe, it is better to keep passing the folder alias and folder display name via the VFS parameters when registering?

I guess we'll have to roll with that for now although I'm slightly surprised so much is required now. We'll see when I get a chance to give it a proper review... dunno when...

And also, I am going to need to pass some parameters to virtual void unregisterFolder() = 0; hence changing the interface. Only this, allows me to get rid of static QMap.

Well, see my comment. I think my initial comment wasn't clear enough. ;-)

er-vin · 2021-03-16T17:19:30Z

src/gui/folderman.cpp

@@ -76,6 +76,9 @@ FolderMan::FolderMan(QObject *parent)
    connect(AccountManager::instance(), &AccountManager::removeAccountFolders,
        this, &FolderMan::slotRemoveFoldersForAccount);

+    connect(AccountManager::instance(), &AccountManager::accountRemoved,


@er-vin No. Absolutely no. I've been debugging it and it is not emitted. Only emitted when the account is removed.

This is surprising, I wonder if something else is broken because this is clearly emitted from AccountManager::shutdown() which is called from the Application dtor.

Another question - when we are removing the account connection - why would we need to store placeholder information? My idea was - they are now invalid and we will instead register everything again when adding a new account connection (which actually happens now).

Sure, when the user removes an account there's no need to keep placeholder information. My comment was more from the prospect of my first comment that I expect the folder removal code to be triggered on application exit which we wouldn't want. Placeholder information needs to stay put even if the application is exited.

er-vin · 2021-03-16T17:20:21Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

@@ -35,6 +38,9 @@ Q_LOGGING_CATEGORY(lcCfApiWrapper, "nextcloud.sync.vfs.cfapi.wrapper", QtInfoMsg
      FIELD_SIZE( CF_OPERATION_PARAMETERS, field ) )

 namespace {
+using SyncRootKeys = QMap<QString, QString>;
+Q_GLOBAL_STATIC(SyncRootKeys, registeredSyncRootKeys)
+QReadWriteLock registeredSyncRootKeysLock;


Do we have so much contention on it that we need a read/write lock? It's much slower than a mutex in the general case.

@er-vin Fixed. Using a mutex now. But, I've got another idea. At the moment, we are always registering sync folders on application launch. Ideally, we shouldn't be doing this part if it is done already in the previous run, and we are not adding any new folders. So, I guess, I shouldn't really keep track of folders that I have added in the runtime, as, if we change this part in the future, I won't have information of registered folders if I restart the app. So, I guess, I need a way to traverse all the keys under *SyncRootManager* and check those that match by providername!WindowsSID!accountDisplayName and then just check if UserSyncRoots has a key that has a value of the sync root path we are removing. This will allow me to ditch this static map and also make the logic more reliable and less likely to becoe broken. Will just need to somehow pass the providerName and userDisplayName into unregister function, somehow.

@er-vin I've removed that global static completely.

er-vin · 2021-03-16T17:25:20Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

@@ -290,8 +301,134 @@ OCC::Optional<OCC::PinStateEnums::PinState> OCC::CfApiWrapper::PlaceHolderInfo::
    return cfPinStateToPinState(_data->PinState);
 }

-OCC::Result<void, QString> OCC::CfApiWrapper::registerSyncRoot(const QString &path, const QString &providerName, const QString &providerVersion)
+QString convertSidToStringSid(_In_ PSID sid)


This feels like code copied and pasted from an MSN example. :-)

Could we make that safer and more ergonomic using higher level constructs. I spent quite some time in the rest of that file to avoid construct like the stringSid raw pointer, the calls to LocalFree and so on. There's value in doing this, it pays off in safety and debug dividends. :-)

This apply to the whole block here.

This feels like code copied and pasted from an MSN example. :-)

Could we make that safer and more ergonomic using higher level constructs. I spent quite some time in the rest of that file to avoid construct like the stringSid raw pointer, the calls to LocalFree, and so on. There's value in doing this, it pays off in safety and debug dividends. :-)

This apply to the whole block here.

@er-vin I tried to get rid of ::LocalFree(stringSid); for like 1 hour. Tried different approaches. QScopedPointer with custom deleted. QVector<wchar_t>, using "delete[] stringSid" instead of ::LocalFree". Nothing worked. It either leads to a heap corruption assert and a crash, either empty SID string or, just doesn't compile. Kindly suggest your vision of how to replace this code with a Qt-like alternative.
Having said that, I don't think using WinAPI ::Local free with raw pointer is a problem inside the Windows-specific implementation source file. If we want to get rid of WinAPI, I guess, we shouldn't be using this ::ConvertSidToStringSid function at all. Not to mentioned, we shouldn't even be using CfAPI as Microsoft base all their constructs on WinAPI :(

Well, it's not really about getting rid of WinAPI, this would be pointless. It's more that their documentation and examples tend to push to use dangerous constructs because well... they don't have better tooling available when you use just that API. But we do and so we shall try to move away from the more dangerous constructs.

@er-vin Alright. Well, I agree with you, but, I couldn't find any alternative to get rid of this construct. The problem here is, they expect a pointer to a pointer. So, they allocate memory and put it into that pointer internally. What I could do, is to try, and implement my own function convertSidToStringSid by shifting required bits to obtain the string representation of WindowsSid. But, isn't it even worse? With their function, at least, we can expect the conversion to always work from one version of Windows to another. In the case of re-inventing the wheel, we will lose this reliability.

P.S. I can also move the function into "Utility" if this will improve the situation.

er-vin · 2021-03-16T17:32:14Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

@@ -346,7 +467,7 @@ OCC::Result<OCC::CfApiWrapper::ConnectionKey, QString> OCC::CfApiWrapper::connec
    if (result != S_OK) {
        return QString::fromWCharArray(_com_error(result).ErrorMessage());
    } else {
-        return key;
+        return std::move(key);


Ah OK. Got it now. This is because of the type elision. I think the better fix is: return { std::move(key) };

This should result in the same generated code but would avoid the confusion on read that the move is not being for what's returned. This is clearly what tripped me up here. :-)

er-vin

Alright, time's up, I'll get to the other functions another time. But at least that should give a direction for further adjustments.

er-vin · 2021-03-19T13:22:07Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

+        LARGE_INTEGER progressCompleted;
+        progressCompleted.QuadPart = offset;
+
+        CfReportProviderProgress(callbackInfo->ConnectionKey, callbackInfo->TransferKey, callbackInfo->FileSize, progressCompleted);


I'd say, please make a cfReportProviderProcess function akin to cfApiSendTransferInfo (or maybe move this into cfApiSendTransferInfo itself? but that'd force changing its interface a bit). Also I'd log CfReportProviderProgress failures just in case.

BTW, wouldn't you think they do that for you? I mean they know the total size (from the placeholder), the offset and the amount you just sent (provided in the CfExecute call)... but no, let's force people to mess with another piece of API just because. :-p

@er-vin Progress is not refreshed if not calling this function.

er-vin · 2021-03-19T13:30:43Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

@@ -291,8 +297,147 @@ OCC::Optional<OCC::PinStateEnums::PinState> OCC::CfApiWrapper::PlaceHolderInfo::
    return cfPinStateToPinState(_data->PinState);
 }

-OCC::Result<void, QString> OCC::CfApiWrapper::registerSyncRoot(const QString &path, const QString &providerName, const QString &providerVersion)
+QString convertSidToStringSid(void *sid)


This is used in retrieveWindowsSID only, so I'd either move it's content there or go the other way around completely and split a getTokenInformation out of retrieveWindowsSID. retriveWindowsSID would then become two simple calls.hink I think I prefer the latter actually. :-)

@er-vin Ok. It's the latter then.

er-vin · 2021-03-19T13:33:28Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

-OCC::Result<void, QString> OCC::CfApiWrapper::registerSyncRoot(const QString &path, const QString &providerName, const QString &providerVersion)
+QString convertSidToStringSid(void *sid)
+{
+    QString result;


Proposal mostly removing some of the extra noise for readability IMHO:

wchar_t *stringSid = nullptr; if (!ConvertSidToStringSid(sid, &stringSid)) { return {}; } const auto result = QString::fromWCharArray(stringSid); LocalFree(stringSid); return result;

er-vin · 2021-03-19T13:35:18Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

+QString retrieveWindowsSID()
+{
+    // FYI: code here is mostly identical to Windows-Classic-Examples/Samples/CloudMirror
+    QScopedPointer<TOKEN_USER> tokenInfo;


I'd favor std::unique_ptr these days. Less error prone.

@er-vin Ok. Bringing that back. I thought it makes no sense to create a method that returns a pointer of that token like it was done originally in that MS's sample. But, ok, let's bring back the old unique_ptr returning function then.

er-vin · 2021-03-19T13:41:06Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

+
+    const auto tokenHandle = GetCurrentThreadEffectiveToken();
+
+    DWORD tokenInfoSize = { 0 };


I'd drop either the = or the curly braces, or move to auto tokenInfoSize = DWORD{0}; whichever suits you best.

er-vin · 2021-03-19T13:47:28Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

+
+    DWORD tokenInfoSize = { 0 };
+    // first - get the required size
+    if (!::GetTokenInformation(tokenHandle, TokenUser, nullptr, 0, &tokenInfoSize))


In fact this should always fail, otherwise we're in big trouble. :-)

I think I'd go for (man... I hate this API):

const auto tokenSizeCallSucceeded = GetTokenInformation(tokenHandle, TokenUser, nullptr, 0, &tokenInfoSize); const auto lastError = GetLastError; Q_ASSERT(!tokenSizeCallSucceeded && lastError == ERROR_INSUFFICIENT_BUFFER); if (tokenSizeCallSucceeded || lastError != ERROR_INSUFFICIENT_BUFFER) { // Log it return {}; } tokenInfo.reset(reinterpret_cast<TOKEN_USER*>(new char[tokenInfoSize])); if (!GetTokenInformation(tokenHandle, TokenUser, tokenInfo.get(), tokenInfoSize, &tokenInfoSize)) { // log it return {}; } return convertSidToStringSid(tokenInfo->User.Sid);

Something along those lines...

@er-vin Ok. Done.

allexzander · 2021-03-19T16:09:11Z

@er-vin Pushed a commit with fixes.

er-vin · 2021-03-19T16:34:27Z

Oh I know, I was complaining at their API not at you. ;-) I find stupid that they force people an extra bit of API while the other ones have already all the information required...

er-vin · 2021-03-22T19:05:30Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

+    Q_ASSERT(!tokenSizeCallSucceeded && lastError == ERROR_INSUFFICIENT_BUFFER);
+    if (tokenSizeCallSucceeded || lastError != ERROR_INSUFFICIENT_BUFFER) {
+        qCCritical(lcCfApiWrapper) << "GetTokenInformation for token size has failed with error" << lastError;
+        return std::unique_ptr<TOKEN_USER>();


nitpick: return {};

er-vin · 2021-03-22T19:05:54Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

+    tokenInfo.reset(reinterpret_cast<TOKEN_USER*>(new char[tokenInfoSize]));
+    if (!::GetTokenInformation(tokenHandle, TokenUser, tokenInfo.get(), tokenInfoSize, &tokenInfoSize)) {
+        qCCritical(lcCfApiWrapper) << "GetTokenInformation failed with error" << lastError;
+        return std::unique_ptr<TOKEN_USER>();


er-vin · 2021-03-22T19:06:33Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

+        return convertSidToStringSid(tokenInfo->User.Sid);
+    }
+
+    return QString();


ditto: return {};

er-vin · 2021-03-22T19:07:12Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

+    return tokenInfo;
+}
+
+QString retrieveWindowsSID()


retrieveWindowsSid (instead of SID mainly for consistency)

er-vin · 2021-03-22T19:07:36Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

+{
+    // We must set specific Registry keys to make the progress bar refresh correctly and also add status icons into Windows Explorer
+    // More about this here: https://docs.microsoft.com/en-us/windows/win32/shell/integrate-cloud-storage
+    const auto windowsSID = retrieveWindowsSID();


s/SID/Sid/g

er-vin · 2021-03-22T19:13:59Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

+        const auto deleteKeyResult = OCC::Utility::registryDeleteKeyTree(HKEY_LOCAL_MACHINE, providerSyncRootIdRegistryKey);
+        Q_ASSERT(!deleteKeyResult);
+    } else {
+        qCInfo(lcCfApiWrapper) << "Successfully set Registry keys for shell integration at:" << providerSyncRootIdRegistryKey << ". Progress bar will work.";


This would be outside of the if, just after the loop and be followed by return true.

A couple less of states to track this way.

er-vin · 2021-03-22T19:15:40Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

+
+bool deleteSyncRootRegistryKey(const QString &syncRootPath, const QString &providerName, const QString &accountDisplayName)
+{
+    const auto &syncRootManagerRegistryKey = R"(SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\SyncRootManager\)";


I'd drop the ref, this is an unwarranted indirection. You could wrap the raw string in QStringLiteral as well. AFAICT it gets implicitly converted to QString several times in the OCC::Utility calls.

er-vin · 2021-03-22T19:15:54Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

+    const auto &syncRootManagerRegistryKey = R"(SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\SyncRootManager\)";
+
+    if (OCC::Utility::registryKeyExists(HKEY_LOCAL_MACHINE, syncRootManagerRegistryKey)) {
+        const auto &windowsSID = retrieveWindowsSID();


s/SID/Sid/g

er-vin · 2021-03-22T19:16:29Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

+        OCC::Utility::registryWalkSubKeys(HKEY_LOCAL_MACHINE, syncRootManagerRegistryKey, [&](HKEY, const QString &syncRootId) {
+            // make sure we have matching syncRootId(providerName!windowsSID!accountDisplayName)
+            if (syncRootId.startsWith(currentUserSyncRootIdPattern)) {
+                const QString syncRootIdUserSyncRootsRegistryKey = syncRootManagerRegistryKey % syncRootId % R"(\UserSyncRoots\)";


QStringLiteral and +

er-vin · 2021-03-22T19:16:41Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

+                const QString syncRootIdUserSyncRootsRegistryKey = syncRootManagerRegistryKey % syncRootId % R"(\UserSyncRoots\)";
+                // check if there is a 'windowsSID' Registry value under \UserSyncRoots and it matches the sync folder path we are removing
+                if (OCC::Utility::registryGetKeyValue(HKEY_LOCAL_MACHINE, syncRootIdUserSyncRootsRegistryKey, windowsSID).toString() == syncRootPath) {
+                    const QString syncRootIdToDelete = syncRootManagerRegistryKey % syncRootId;


but mainly for consistency

allexzander · 2021-03-24T07:16:43Z

@er-vin Are we good?

er-vin · 2021-03-24T08:15:21Z

@er-vin Are we good?

Dunno, I had time to look only at half of it so far I think. I have to look at it in chunk unfortunately. But we still have some time anyway, you aimed at 3.2.1 you said.

allexzander · 2021-03-24T08:55:26Z

@er-vin Are we good?
Dunno, I had time to look only at half of it so far I think. I have to look at it in chunk unfortunately. But we still have some time anyway, you aimed at 3.2.1 you said.
@er-vin
Well, I thought I've addressed your comments and could include this into RC. Still waiting for the Status feature from Camila, though...

er-vin · 2021-03-24T11:08:47Z

Well, I thought I've addressed your comments and could include this into RC.

As I said: dunno, didn't look at the last round yet + I commented only on what I reviewed so far which was only part of it. I'm doing this on my free time you know. :-)

Signed-off-by: allexzander <blackslayer4@gmail.com>

nextcloud-desktop-bot · 2021-03-24T12:58:16Z

AppImage file: Nextcloud-PR-2981-193e50311b94843e363d77350d28b60c3e4ae645-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-03-24T13:02:37Z

@er-vin OK. Merged for now after fixing a bug that I've introduced when fixing the review comments :) It is always good to double-check. Confirmed as working. But, will gladly fix any other nitpicks you've got in a separate PR :)

er-vin

OK, in fact I was almost through. :-)

Just an issue with one of the latest adjustments.

er-vin · 2021-03-24T18:02:11Z

src/libsync/vfs/cfapi/cfapiwrapper.cpp

+        qCCritical(lcCfApiWrapper) << "Couldn't send transfer info" << QString::number(transferKey.QuadPart, 16) << ":" << cfExecuteresult << QString::fromWCharArray(_com_error(cfExecuteresult).ErrorMessage());
+    }
+
+    // refresh Windows Copy Dialog progress


This needs to be conditional, if the status passed in parameter is not STATUS_SUCCESS there's no progress to report. This would happen on the server timing out during a download or such.

allexzander force-pushed the vfs_win_progress_bar_refresh branch 2 times, most recently from f34d8bd to 3406895 Compare March 10, 2021 13:39

allexzander marked this pull request as ready for review March 10, 2021 14:10

allexzander requested review from er-vin, FlexW and camilasan March 10, 2021 14:10

er-vin reviewed Mar 10, 2021

View reviewed changes

allexzander force-pushed the vfs_win_progress_bar_refresh branch 2 times, most recently from 341482d to 654b301 Compare March 12, 2021 15:23

allexzander requested a review from er-vin March 12, 2021 15:34

allexzander force-pushed the vfs_win_progress_bar_refresh branch 2 times, most recently from bc1ed00 to 29d1f0e Compare March 15, 2021 10:51

allexzander requested review from er-vin and removed request for er-vin March 15, 2021 11:08

er-vin reviewed Mar 16, 2021

View reviewed changes

allexzander force-pushed the vfs_win_progress_bar_refresh branch 2 times, most recently from e2c2ef7 to 28e978b Compare March 17, 2021 14:04

allexzander requested a review from er-vin March 17, 2021 14:06

allexzander force-pushed the vfs_win_progress_bar_refresh branch from 28e978b to d2c84bc Compare March 18, 2021 10:43

er-vin reviewed Mar 19, 2021

View reviewed changes

allexzander force-pushed the vfs_win_progress_bar_refresh branch from 4a95402 to 1ec93ed Compare March 19, 2021 16:05

allexzander requested a review from er-vin March 19, 2021 16:09

er-vin reviewed Mar 22, 2021

View reviewed changes

allexzander force-pushed the vfs_win_progress_bar_refresh branch 2 times, most recently from b3a1bb4 to 9ac54f1 Compare March 23, 2021 08:50

allexzander force-pushed the vfs_win_progress_bar_refresh branch from 562a71b to 1a87bae Compare March 23, 2021 08:58

allexzander requested a review from er-vin March 23, 2021 09:00

Refresh Windows download dialog progress when hydrating a placeholder

193e503

Signed-off-by: allexzander <blackslayer4@gmail.com>

allexzander force-pushed the vfs_win_progress_bar_refresh branch from a4b23e6 to 193e503 Compare March 24, 2021 12:09

allexzander merged commit e3a2647 into master Mar 24, 2021

allexzander deleted the vfs_win_progress_bar_refresh branch March 24, 2021 13:01

er-vin reviewed Mar 24, 2021

View reviewed changes


		const auto tokenHandle = GetCurrentThreadEffectiveToken();

		DWORD tokenInfoSize = { 0 };

Refresh Windows download dialog progress when hydrating a placeholder #2981

Refresh Windows download dialog progress when hydrating a placeholder #2981

Conversation

allexzander commented Mar 9, 2021 • edited Loading

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

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 Mar 15, 2021 • edited Loading

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 Mar 10, 2021

allexzander commented Mar 12, 2021

er-vin commented Mar 12, 2021

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

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

allexzander commented Mar 19, 2021

er-vin commented Mar 19, 2021 via email

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 Mar 24, 2021

er-vin commented Mar 24, 2021 via email

allexzander commented Mar 24, 2021 • edited Loading

er-vin commented Mar 24, 2021 via email

nextcloud-desktop-bot commented Mar 24, 2021

allexzander commented Mar 24, 2021

er-vin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allexzander commented Mar 9, 2021 •

edited

Loading

allexzander Mar 15, 2021 •

edited

Loading

allexzander commented Mar 24, 2021 •

edited

Loading