Sharing feature for Nextmcloud #4858

TSI-pranalideshpande · 2022-08-18T12:39:12Z

In this ,we have harmonized the permission settings for sharing. Currently we have different options for internal, external and link shares as well as different option for files and folders.

We have added a radio button group containing the options for user(External) & sharelink for files and folder:
"Read only" (Nur Lesen)
"Can edit" (Kann bearbeiten)
we removed "can create", "can delete",

2.for link share on folder we have added one more option "Filedrop only"

3.Added dropdown containing the options of the radio button group for files and folder on user and share link
"Read only" (Nur Lesen)
"Can edit" (Kann bearbeiten)

Sample screenshot of added dropdown is herewith
Its only UI related changes , we have covered actual changes on ticket Adjust theme to nextcloud #3&NextCloud client unable to connect to server #4 for sharing , we will raised PR for same very soon.

claucambra

Hi, the contributions are appreciated and this is something we might be interested in merging at some point, Unfortunately, this PR needs significant changes before that can be considered.

Chiefly, there are 3 major problems:

Style -- the style of the changes is inconsistent and often makes the code difficult to read. Please adapt your code to follow the style of the rest of the desktop client
Unrelated and unneeded changes -- I am not sure if this was caused by something such as a bad rebase of the branch, but this PR contains a significant amount of additions and changes that do not seem relevant to the changes mentioned in the description. Some of the changes that do seem related to the PR are also unclear (for instance, minute changes to window sizing in UI files). Please address this as at the moment this code is unmergeable and the CI pipelines for this PR are failing
Screenshots -- just looking at the code it is very difficult to tell how the UI has changed. Please provide media such as screenshots and screen recordings to demonstrate the changes so that reviewers on both the Desktop Client team and the Design Team can review the visual changes, per the PR template

claucambra · 2022-08-19T00:33:55Z

admin/win/msi/RegistryCleanup.vbs

+On Error goto 0
+
+Const HKEY_LOCAL_MACHINE = &H80000002
+
+Const strObjRegistry = "winmgmts:\\.\root\default:StdRegProv"
+
+Function RegistryDeleteKeyRecursive(regRoot, strKeyPath)
+  Set objRegistry = GetObject(strObjRegistry)
+  objRegistry.EnumKey regRoot, strKeyPath, arrSubkeys
+  If IsArray(arrSubkeys) Then
+    For Each strSubkey In arrSubkeys
+      RegistryDeleteKeyRecursive regRoot, strKeyPath & "\" & strSubkey
+    Next
+  End If
+  objRegistry.DeleteKey regRoot, strKeyPath
+End Function
+
+Function RegistryListSubkeys(regRoot, strKeyPath)
+  Set objRegistry = GetObject(strObjRegistry)
+  objRegistry.EnumKey regRoot, strKeyPath, arrSubkeys
+  RegistryListSubkeys = arrSubkeys
+End Function
+
+Function GetUserSID()
+  Dim objWshNetwork, objUserAccount
+
+  Set objWshNetwork = CreateObject("WScript.Network")
+
+  Set objUserAccount = GetObject("winmgmts://" & objWshNetwork.UserDomain & "/root/cimv2").Get("Win32_UserAccount.Domain='" & objWshNetwork.ComputerName & "',Name='" & objWshNetwork.UserName & "'")
+  GetUserSID = objUserAccount.SID
+End Function
+
+Function RegistryCleanupSyncRootManager()
+  strSyncRootManagerKeyPath = "SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\SyncRootManager"
+
+  arrSubKeys = RegistryListSubkeys(HKEY_LOCAL_MACHINE, strSyncRootManagerKeyPath)
+
+  If IsArray(arrSubkeys) Then
+    arrSubkeys=Filter(arrSubkeys, Session.Property("APPNAME"))
+  End If
+  If IsArray(arrSubkeys) Then
+    arrSubkeys=Filter(arrSubkeys, GetUserSID())
+  End If
+
+  If IsArray(arrSubkeys) Then
+    For Each strSubkey In arrSubkeys
+      RegistryDeleteKeyRecursive HKEY_LOCAL_MACHINE, strSyncRootManagerKeyPath & "\" & strSubkey
+    Next
+  End If
+End Function
+
+Function RegistryCleanup()
+  RegistryCleanupSyncRootManager()
+End Function


These changes seem unrelated

claucambra · 2022-08-19T00:35:06Z

appveyor.ini

+[General]
+Branch = master
+ShallowClone = True
+
+# Variables defined here override the default value
+# The variable names are casesensitive
+[Variables]
+#Values need to be overwritten to create a chache
+UseCache = True
+CreateCache = False
+
+# Settings applicable for all Crafts matrices
+# Settings are Category/key=value
+# Category is case sensitive
+[GeneralSettings]
+
+## This is the location of your python installation.
+## This value must be set.
+Paths/Python = C:\Python39-x64
+Paths/Python27 = C:\Python27-x64
+
+Compile/BuildType = RelWithDebInfo
+
+Compile/UseNinja = True
+
+Paths/downloaddir = ${Variables:Root}\downloads
+ShortPath/Enabled = False
+ShortPath/EnableJunctions = True
+ShortPath/JunctionDir = C:\CM-SP\
+
+; Packager/RepositoryUrl = https://files.kde.org/craft/
+Packager/PackageType = NullsoftInstallerPackager
+Packager/RepositoryUrl = http://ftp.acc.umu.se/mirror/kde.org/files/craft/master/
+
+ContinuousIntegration/Enabled = True
+
+## This option can be used to override the default make program
+## change the value to the path of the executable you want to use instead.
+Compile/MakeProgram = jom
+
+Packager/UseCache = ${Variables:UseCache}
+Packager/CreateCache = ${Variables:CreateCache}
+Packager/CacheDir = ${Variables:Root}\cache
+
+[BlueprintSettings]
+# don't try to pip install on the ci
+python-modules.ignored = True
+nextcloud-client.buildTests = True
+binary/mysql.useMariaDB = False
+
+[windows-msvc2019_64-cl]
+QtSDK/Compiler = msvc2019_64
+General/ABI = windows-msvc2019_64-cl


These changes also seem unrelated, it looks like there has been a mistake somewhere, maybe rebasing the branch?

claucambra · 2022-08-19T00:35:18Z

appveyor.yml

+version: '{build}-{branch}'
+
+image: Visual Studio 2019
+
+branches:
+  only:
+    - master
+
+clone_depth: 1
+
+init:
+- ps: |
+    function craft() {
+        cmd /C "echo %PATH%"
+        & "C:\Python39-x64\python.exe" "C:\CraftMaster\CraftMaster\CraftMaster.py" --config "$env:APPVEYOR_BUILD_FOLDER\appveyor.ini" --variables "APPVEYOR_BUILD_FOLDER=$env:APPVEYOR_BUILD_FOLDER" --target $env:TARGET -c $args
+        if($LASTEXITCODE -ne 0) {exit $LASTEXITCODE}
+    }
+    function crafttests() {
+        cmd /C "echo %PATH%"
+        & "C:\Python39-x64\python.exe" "C:\CraftMaster\CraftMaster\CraftMaster.py" --config "$env:APPVEYOR_BUILD_FOLDER\appveyor.ini" --variables "APPVEYOR_BUILD_FOLDER=$env:APPVEYOR_BUILD_FOLDER" --target $env:TARGET -c $args
+    }
+
+install:
+- ps: |
+    #use cmd to silence powershell behaviour for stderr
+    & cmd /C "git clone -q --depth=1 https://invent.kde.org/packaging/craftmaster.git C:\CraftMaster\CraftMaster 2>&1"
+    craft --add-blueprint-repository [git]https://github.com/nextcloud/desktop-client-blueprints.git
+    craft craft
+    craft --install-deps nextcloud-client
+    craft nsis
+
+build_script:
+- ps: |
+    craft --src-dir $env:APPVEYOR_BUILD_FOLDER nextcloud-client
+
+test_script:
+- ps: |
+    crafttests --test --src-dir $env:APPVEYOR_BUILD_FOLDER nextcloud-client
+
+environment:
+    matrix:
+    - TARGET: windows-msvc2019_64-cl


Once more these seem unrelated

claucambra · 2022-08-19T00:35:50Z

src/gui/sharedialog.ui

@@ -7,7 +7,7 @@
    <x>0</x>
    <y>0</y>
    <width>385</width>
-    <height>400</height>
+    <height>404</height>


I am not sure why the four extra pixels are needed here

claucambra · 2022-08-19T00:36:20Z

src/gui/sharedialog.ui

-          <width>359</width>
-          <height>320</height>
+          <width>361</width>
+          <height>314</height>


Again, not sure why these tiny adjustments to the dialog size are needed?

claucambra · 2022-08-19T00:45:19Z

src/gui/sharelinkwidget.cpp


    } else if (action == _allowUploadLinkAction && state) {
        perm = SharePermissionCreate;
        _linkShare->setPermissions(perm);
+         _ui->currentPermission_3->setText(action->text());


Suggested change

_ui->currentPermission_3->setText(action->text());

_ui->currentPermission_3->setText(action->text());

claucambra · 2022-08-19T00:46:32Z

src/gui/sharelinkwidget.cpp

-        _allowUploadLinkAction = permissionsGroup->addAction(tr("File drop (upload only)"));
+        _allowUploadLinkAction = permissionsGroup->addAction(tr("File drop"));


I think the previous string was better, more descriptive

claucambra · 2022-08-19T00:50:03Z

src/gui/shareusergroupwidget.cpp

+    //    QString  menuStyle("QMenu::item:checked{color: #e20074;}");
+    //    permissionMenu->setStyleSheet(menuStyle);


If this is not needed please remove

claucambra · 2022-08-19T00:50:14Z

src/gui/shareusergroupwidget.cpp

+    //    QString  menuStyle("QMenu::item:checked{color: #e20074;}");
+    //    permissionMenu->setStyleSheet(menuStyle);
+
+
    _permissionReshare= new QAction(tr("Can reshare"), this);


Suggested change

_permissionReshare= new QAction(tr("Can reshare"), this);

_permissionReshare = new QAction(tr("Can reshare"), this);

claucambra · 2022-08-19T00:51:25Z

src/gui/shareusergroupwidget.cpp

@@ -589,7 +607,7 @@ ShareUserLine::ShareUserLine(AccountPtr account, QSharedPointer<UserGroupShare>
     * https://github.com/owncloud/client/issues/4996
     */
    if (share->getShareType() == Share::TypeRemote
-        && share->account()->serverVersionInt() < Account::makeServerVersion(9, 1, 0)) {
+            && share->account()->serverVersionInt() < Account::makeServerVersion(9, 1, 0)) {


indentation

codecov · 2022-08-19T01:02:59Z

Codecov Report

Merging #4858 (81f6ddb) into master (a8b296c) will not change coverage.
The diff coverage is n/a.

❗ Current head 81f6ddb differs from pull request most recent head 3095d5f. Consider uploading reports for the commit 3095d5f to get more accurate results

@@           Coverage Diff           @@
##           master    #4858   +/-   ##
=======================================
  Coverage   57.21%   57.21%           
=======================================
  Files         138      138           
  Lines       17144    17144           
=======================================
  Hits         9809     9809           
  Misses       7335     7335

Impacted Files	Coverage Δ
src/libsync/propagatedownload.cpp	`65.18% <0.00%> (ø)`

nextcloud-desktop-bot · 2022-08-22T08:33:18Z

AppImage file: Nextcloud-PR-4858-3095d5f8cc4f54ef6a5d2694c53548676af4f645-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.

TSI-pranalideshpande · 2022-08-22T12:27:36Z

Hi ,Thanks for the review , I observed some issues in rebasing and while creating pull request for this branch some files are unnecesarily raised for PR , I am planing to create new branch and then will raise pr for that.

claucambra · 2022-11-09T15:56:34Z

Closing now that #4929 has been merged and this is not compatible

TSI-pranalideshpande and others added 4 commits August 10, 2022 18:08

Sharing 1&2 related changes

af1a0b0

Sharing #1 & #2 features added

51d9caa

Sharing #1 ,#2 features added

79a6465

permission changes for file

81f6ddb

claucambra requested changes Aug 19, 2022

View reviewed changes

TSI-pranalideshpande and others added 2 commits August 22, 2022 12:59

Merge branch 'nextcloud:master' into SharingFeature

6d293d5

Retained size related changes

3095d5f

claucambra closed this Nov 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sharing feature for Nextmcloud #4858

Sharing feature for Nextmcloud #4858

TSI-pranalideshpande commented Aug 18, 2022

claucambra left a comment •

edited

claucambra Aug 19, 2022

claucambra Aug 19, 2022

claucambra Aug 19, 2022

claucambra Aug 19, 2022

claucambra Aug 19, 2022

claucambra Aug 19, 2022

claucambra Aug 19, 2022

claucambra Aug 19, 2022

claucambra Aug 19, 2022

claucambra Aug 19, 2022

codecov bot commented Aug 19, 2022 •

edited

nextcloud-desktop-bot commented Aug 22, 2022

TSI-pranalideshpande commented Aug 22, 2022

claucambra commented Nov 9, 2022

	_ui->currentPermission_3->setText(action->text());
	_ui->currentPermission_3->setText(action->text());

		_allowUploadLinkAction = permissionsGroup->addAction(tr("File drop (upload only)"));
		_allowUploadLinkAction = permissionsGroup->addAction(tr("File drop"));

		// QString menuStyle("QMenu::item:checked{color: #e20074;}");
		// permissionMenu->setStyleSheet(menuStyle);

	_permissionReshare= new QAction(tr("Can reshare"), this);
	_permissionReshare = new QAction(tr("Can reshare"), this);

Sharing feature for Nextmcloud #4858

Sharing feature for Nextmcloud #4858

Conversation

TSI-pranalideshpande commented Aug 18, 2022

claucambra left a comment • edited

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

codecov bot commented Aug 19, 2022 • edited

Codecov Report

nextcloud-desktop-bot commented Aug 22, 2022

TSI-pranalideshpande commented Aug 22, 2022

claucambra commented Nov 9, 2022

claucambra left a comment •

edited

codecov bot commented Aug 19, 2022 •

edited