Merge branch 'master' into work/more-logging

Author: modSpike

.github/workflows/main.yml

@@ -39,16 +39,6 @@ jobs:
               os: macos-latest
               container:
               useSonarCloud:
-            - name: macos-clang-arm64-debug
-              target: macos-clang-arm64-debug
-              os: macos-latest
-              container:
-              useSonarCloud:
-            - name: linux-gcc-x86_64
-              target: linux-64-gcc
-              os: ubuntu-latest
-              container: owncloudci/appimage-build:sles15-amd64
-              useSonarCloud:
 
     name: ${{ matrix.name }}
 
@@ -141,12 +131,6 @@ jobs:
     - name: Build
       if: ${{ !matrix.useSonarCloud }}
       run: |
-        if ("${{ matrix.target }}" -eq "macos-64-clang-debug" ) {
-            # https://api.kde.org/ecm/module/ECMEnableSanitizers.html
-            # address;leak;undefined
-            # clang: error: unsupported option '-fsanitize=leak' for target 'x86_64-apple-darwin21.6.0'
-            & "${env:GITHUB_WORKSPACE}/.github/workflows/.craft.ps1" -c --set args="-DECM_ENABLE_SANITIZERS='address;undefined'" owncloud/owncloud-client
-        }
         if ("${{ matrix.target }}" -eq "linux-64-gcc" ) {
             # build with appimage updater and others
             & "${env:GITHUB_WORKSPACE}/.github/workflows/.craft.ps1" -c --set enableAppImageUpdater=true owncloud-client

src/gui/folder.cpp

@@ -155,7 +155,7 @@ Folder::Folder(const FolderDefinition &definition, const AccountStatePtr &accoun
         connect(_engine.data(), &SyncEngine::itemCompleted,
             _localDiscoveryTracker.data(), &LocalDiscoveryTracker::slotItemCompleted);
 
-        connect(_accountState->account()->spacesManager(), &GraphApi::SpacesManager::spaceChanged, [this](GraphApi::Space *changedSpace) {
+        connect(_accountState->account()->spacesManager(), &GraphApi::SpacesManager::spaceChanged, this, [this](GraphApi::Space *changedSpace) {
             if (_definition.spaceId() == changedSpace->id()) {
                 Q_EMIT spaceChanged();
             }

src/libsync/graphapi/space.cpp

@@ -32,11 +32,15 @@ const auto personalC = QLatin1String("personal");
 const auto sharesIdC = QLatin1String("a0ca6a90-a365-4782-871e-d44447bbc668$a0ca6a90-a365-4782-871e-d44447bbc668");
 }
 
-Space::Space(SpacesManager *spacesManager, const OpenAPI::OAIDrive &drive)
+Space::Space(SpacesManager *spacesManager, const OpenAPI::OAIDrive &drive, const bool hasManyPersonalSpaces)
     : QObject(spacesManager)
     , _spaceManager(spacesManager)
     , _image(new SpaceImage(this))
+    , _hasManyPersonalSpaces(hasManyPersonalSpaces)
 {
+    // todo future refactoring: get this setDrive out of the ctr since it potentially kicks off a job for the SpaceImage before the Space is fully constructed
+    // propose removing the drive arg from the ctr completely, and moving the call to setDrive to the spacesmanager such that any call to
+    // "new" space is immediately followed by setDrive.
     setDrive(drive);
     connect(_image, &SpaceImage::imageChanged, this, &Space::imageChanged);
 }
@@ -48,63 +52,34 @@ OpenAPI::OAIDrive Space::drive() const
 
 void Space::setDrive(const OpenAPI::OAIDrive &drive)
 {
-    _drive = drive;
-    _image->update();
-}
 
-SpaceImage::SpaceImage(Space *space)
-    : QObject(space)
-    , _space(space)
-{
-    update();
-}
+    // first config naturally has an empty drive - reality check that updated drives are always valid
+    Q_ASSERT(drive.isValid());
 
-QIcon SpaceImage::image() const
-{
-    if (_image.isNull()) {
-        return Resources::getCoreIcon(QStringLiteral("space"));
-    }
-    return _image;
-}
-
-QUrl SpaceImage::qmlImageUrl() const
-{
-    if (!_image.isNull()) {
-        return QUrl(QStringLiteral("image://space/%1/%2").arg(etag(), _space->id()));
-    } else {
-        // invalid space id to display the placeholder
-        return QUrl(QStringLiteral("image://space/placeholder"));
-    }
-}
+    QString curTag = _drive.getRoot().getETag();
+    QString newTag = drive.getRoot().getETag();
+    Q_ASSERT(!newTag.isEmpty());
+    // the tag should change when the space is edited on the server. I verified that it changes on space rename as well
+    // as on changing the space image so we may have further wrinkles if there is an error in logic server side, but for now
+    // we want to reduce updates to "only when something changed" else everything is auto-refreshed periodically (eg every 30s)
+    if (curTag == newTag)
+        return;
 
-void SpaceImage::update()
-{
-    const auto &special = _space->drive().getSpecial();
-    const auto img = std::find_if(special.cbegin(), special.cend(), [](const auto &it) { return it.getSpecialFolder().getName() == QLatin1String("image"); });
-    if (img != special.cend()) {
-        _url = QUrl(img->getWebDavUrl());
-        _etag = Utility::normalizeEtag(img->getETag());
-        auto job = _space->_spaceManager->account()->resourcesCache()->makeGetJob(_url, {}, _space);
-        QObject::connect(job, &SimpleNetworkJob::finishedSignal, _space, [job, this] {
-            if (job->httpStatusCode() == 200) {
-                _image = job->asIcon();
-                Q_EMIT imageChanged();
-            }
-        });
-        job->start();
-    }
+    _drive = drive;
+    _image->update();
 }
 
 QString Space::displayName() const
 {
-    auto hasManyPersonalSpaces = _spaceManager->account()->capabilities().spacesSupport().enabled;
-    if (hasManyPersonalSpaces) {
+    if (_hasManyPersonalSpaces) {
         return _drive.getName();
     }
 
+    // other systems like oCIS have one personal and one shared space and their names are hard coded
     if (_drive.getDriveType() == personalC) {
         return tr("Personal");
-    } else if (_drive.getId() == sharesIdC) {
+    }
+    if (_drive.getId() == sharesIdC) {
         // don't call it ShareJail
         return tr("Shares");
     }
@@ -141,3 +116,61 @@ QUrl Space::webdavUrl() const
 {
     return QUrl(_drive.getRoot().getWebDavUrl());
 }
+
+SpaceImage::SpaceImage(Space *space)
+    : QObject(space)
+    , _space(space)
+{
+}
+
+QIcon SpaceImage::image() const
+{
+    if (_image.isNull()) {
+        return Resources::getCoreIcon(QStringLiteral("space"));
+    }
+    return _image;
+}
+
+QUrl SpaceImage::qmlImageUrl() const
+{
+    if (!_image.isNull()) {
+        return QUrl(QStringLiteral("image://space/%1/%2").arg(etag(), _space->id()));
+    } else {
+        // invalid space id to display the placeholder
+        return QUrl(QStringLiteral("image://space/placeholder"));
+    }
+}
+
+void SpaceImage::update()
+{
+    const auto &special = _space->drive().getSpecial();
+    const auto img = std::find_if(special.cbegin(), special.cend(), [](const auto &it) { return it.getSpecialFolder().getName() == QLatin1String("image"); });
+    if (img != special.cend())
+    {
+        // ssue 12057: verified the image etag does change when the space's icon has been updated via the web interface
+        // check the etag before updating the members and creating the job. This should eliminate *many* pointless resource jobs which
+        // will exacerbate whatever is making the app crash when the space tries to clean up it's child jobs and one is already gone
+        QString newEtag = Utility::normalizeEtag(img->getETag());
+        if (_etag == newEtag)
+            return;
+
+        _etag = newEtag;
+        _url = QUrl(img->getWebDavUrl());
+        // issue 12057: I am leaving the space as the parent of the job just to avoid having confusion about "new" destructor crashes. At least
+        // we'll get any future crashes on the space still. I don't think that changing the parent is going to eliminate the associate crash - save
+        // this for a future refactoring
+        auto job = _space->_spaceManager->account()->resourcesCache()->makeGetJob(_url, {}, _space);
+
+        // TODO: next problem = this routine is correctly run when the icon has changed on the server, but the icon in the gui does not get refreshed!
+        // The icon IS in the app cache, so it seems to have been brought back from the server correctly, but it is not shown until restart
+        // I did have a quick look at Resources::getCoreIcon - that is used in the SpaceImage::image function. Also sus is that I can't find any slot
+        // for the imageChanged signal but I think this may be connected in qml via the associated space image property.
+        QObject::connect(job, &SimpleNetworkJob::finishedSignal, _space, [job, this] {
+            if (job->httpStatusCode() == 200) {
+                _image = job->asIcon();
+                Q_EMIT imageChanged();
+            }
+        });
+        job->start();
+    }
+}

src/libsync/graphapi/space.h

@@ -77,7 +77,7 @@ namespace GraphApi {
         OpenAPI::OAIDrive drive() const;
 
         /***
-         * Asign a priority to a drive, used for sorting
+         * Assign a priority to a drive, used for sorting
          */
         uint32_t priority() const;
 
@@ -94,14 +94,16 @@ namespace GraphApi {
         void imageChanged();
 
     private:
-        Space(SpacesManager *spaceManager, const OpenAPI::OAIDrive &drive);
+        Space(SpacesManager *spaceManager, const OpenAPI::OAIDrive &drive, bool hasManyPersonalSpaces);
         void setDrive(const OpenAPI::OAIDrive &drive);
 
         SpacesManager *_spaceManager;
         OpenAPI::OAIDrive _drive;
 
         SpaceImage *_image;
 
+        bool _hasManyPersonalSpaces;
+
         friend class SpacesManager;
         friend class SpaceImage;
     };

src/libsync/graphapi/spacesmanager.cpp

@@ -55,18 +55,22 @@ void SpacesManager::refresh()
     if (!_account->credentials()->ready()) {
         return;
     }
-    // TODO: leak the job until we fixed the onwership https://github.com/owncloud/client/issues/11203
+
+    // TODO: leak the job until we fixed the ownership https://github.com/owncloud/client/issues/11203
     auto drivesJob = new Drives(_account->sharedFromThis(), nullptr);
     drivesJob->setTimeout(refreshTimeoutC);
     connect(drivesJob, &Drives::finishedSignal, this, [drivesJob, this] {
+        // a system which provides multiple personal spaces the name of the drive is always used as display name
+        auto hasManyPersonalSpaces = this->account()->capabilities().spacesSupport().hasMultiplePersonalSpaces;
+
         drivesJob->deleteLater();
         if (drivesJob->httpStatusCode() == 200) {
             auto oldKeys = _spacesMap.keys();
             for (const auto &dr : drivesJob->drives()) {
                 auto *space = this->space(dr.getId());
                 oldKeys.removeAll(dr.getId());
                 if (!space) {
-                    space = new Space(this, dr);
+                    space = new Space(this, dr, hasManyPersonalSpaces);
                     _spacesMap.insert(dr.getId(), space);
                 } else {
                     space->setDrive(dr);

test/gui/shared/scripts/bdd_hooks.py

@@ -112,10 +112,7 @@ def hook(context):
         os.makedirs(tmp_dir)
 
     # sync connection folder display name
-    # For oCIS, it will be the user's display name
-    # and will be set while adding the account or creating resources
-    if not get_config("ocis"):
-        set_config("syncConnectionName", "ownCloud")
+    set_config("syncConnectionName", "Personal" if get_config("ocis") else "ownCloud")
 
 
 # determines if the test scenario failed or not

test/gui/shared/scripts/helpers/ConfigHelper.py

@@ -100,7 +100,6 @@ def get_default_home_dir():
     'ocis': False,
     'record_video_on_failure': False,
     'files_for_upload': os.path.join(CURRENT_DIR.parent.parent, 'files-for-upload'),
-    'syncConnectionName': '',
 }
 CONFIG.update(DEFAULT_PATH_CONFIG)
 

test/gui/shared/scripts/helpers/SetupClientHelper.py

@@ -74,18 +74,11 @@ def set_current_user_sync_path(sync_path):
 
 def get_resource_path(resource='', user='', space=''):
     sync_path = get_config('currentUserSyncPath')
-    if not sync_path.startswith(get_config('tempFolderPath')):
-        if user:
-            sync_path = user
-        else:
-            user = parse_username_from_sync_path(sync_path)
-        if get_config('ocis'):
-            space = (
-                space
-                or get_config('syncConnectionName')
-                or get_displayname_for_user(user)
-            )
-            sync_path = join(sync_path, space)
+    if user:
+        sync_path = user
+    if get_config('ocis'):
+        space = space or get_config('syncConnectionName')
+        sync_path = join(sync_path, space)
     sync_path = join(get_config('clientRootSyncPath'), sync_path)
     resource = resource.replace(sync_path, '').strip('/').strip('\\')
     if is_windows():
@@ -96,10 +89,6 @@ def get_resource_path(resource='', user='', space=''):
     )
 
 
-def parse_username_from_sync_path(sync_path):
-    return sync_path.split('/').pop()
-
-
 def get_temp_resource_path(resource_name):
     return join(get_config('tempFolderPath'), resource_name)
 
@@ -168,10 +157,12 @@ def generate_account_config(users, space='Personal'):
         server_url = get_config('localBackendUrl')
 
         if is_ocis := get_config('ocis'):
-            if space == 'Personal':
-                space = get_displayname_for_user(username)
+            set_config('syncConnectionName', space)
             sync_path = create_space_path(space)
-            dav_endpoint = url_join('dav/spaces', get_space_id(space, username))
+            space_name = space
+            if space == 'Personal':
+                space_name = get_displayname_for_user(username)
+            dav_endpoint = url_join('dav/spaces', get_space_id(space_name, username))
 
         args = {
             'url': url_join(server_url, dav_endpoint, ''),
@@ -199,10 +190,7 @@ def generate_account_config(users, space='Personal'):
     return sync_paths
 
 
-def setup_client(username, space=None):
-    if not space or space == 'Personal':
-        space = get_displayname_for_user(username)
-        set_config('syncConnectionName', space)
+def setup_client(username, space='Personal'):
     sync_paths = generate_account_config([username], space)
     start_client()
     for _, sync_path in sync_paths.items():

test/gui/shared/steps/account_context.py

@@ -17,13 +17,12 @@
     wait_for_initial_sync_to_complete,
     listen_sync_status_for_item,
 )
-from helpers.ConfigHelper import get_config, set_config, is_windows, is_linux
+from helpers.ConfigHelper import get_config, is_windows, is_linux
 
 
 @When('the user adds the following user credentials:')
 def step(context):
     account_details = get_client_details(context)
-    set_config('syncConnectionName', get_displayname_for_user(account_details['user']))
     AccountConnectionWizard.add_user_credentials(
         account_details['user'], account_details['password']
     )
@@ -106,7 +105,6 @@ def step(context):
 @When('the user adds the following account:')
 def step(context):
     account_details = get_client_details(context)
-    set_config('syncConnectionName', get_displayname_for_user(account_details['user']))
     AccountConnectionWizard.add_account(account_details)
     # wait for files to sync
     wait_for_initial_sync_to_complete(get_resource_path('/', account_details['user']))
@@ -115,10 +113,6 @@ def step(context):
 @Given('the user has entered the following account information:')
 def step(context):
     account_details = get_client_details(context)
-    if account_details['user']:
-        set_config(
-            'syncConnectionName', get_displayname_for_user(account_details['user'])
-        )
     AccountConnectionWizard.add_account_information(account_details)
 
 

test/gui/shared/steps/spaces_context.py

@@ -11,7 +11,7 @@
     get_file_content,
     resource_exists,
 )
-from helpers.ConfigHelper import get_config, set_config
+from helpers.ConfigHelper import get_config
 
 
 @Given('the administrator has created a space "|any|"')
@@ -43,7 +43,6 @@ def step(context, user, space_name):
     enter_password = EnterPassword()
     if get_config('ocis'):
         enter_password.accept_certificate()
-        set_config('syncConnectionName', space_name)
     enter_password.login_after_setup(user, password)
     # wait for files to sync
     wait_for_initial_sync_to_complete(get_resource_path('/', user, space_name))

test/gui/tst_addAccount/test.feature

@@ -76,7 +76,7 @@ Feature: adding accounts
             | user     | Alice          |
             | password | 1234           |
         When the user selects manual sync folder option in advanced section
-        And the user syncs the "Alice Hansen" space
+        And the user syncs the "Personal" space
         Then the folder "simple-folder" should exist on the file system
 
     @skipOnOCIS