Add tag support in sync process #6400

Runtemund · 2024-01-30T23:43:33Z

I added basic support for file tags to the desktop client. Actually it is a draft, as long as also some of my todos also are already open. But I am welcome for early feedback about the integration.

So finally this shall fix #206

Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Nextcloud bot <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Matthieu Gallien <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: rakekniven <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Reported at Transifex Signed-off-by: rakekniven <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: John Molakvoæ <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 3 to 4. - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@v3...v4) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

is needed to avoid failing builds due to warnings unclear when we will tackle the work of removing the use of deprectaed APIs Signed-off-by: Matthieu Gallien <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Claudio Cambra <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Nextcloud bot <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Camila Ayres <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

…ingBody. - Add const auto. - Remove extra error message information to make it more user friendly. Signed-off-by: Camila Ayres <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Camila Ayres <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Nextcloud bot <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Camila Ayres <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Nextcloud bot <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Josh Richards <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Claudio Cambra <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Claudio Cambra <[email protected]> f Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Claudio Cambra <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Matthieu Gallien <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

we are defining a component, by definition there will be no parent setting the size if the responsibility of the stack view, not of the component (or even its definition) Signed-off-by: Matthieu Gallien <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Matthieu Gallien <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Nextcloud bot <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

… Windows. Prevents signature verification failure. Signed-off-by: alex-z <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Signed-off-by: Uwe Runtemund <[email protected]>

Runtemund · 2024-02-14T11:57:54Z

On client side, everything should be done for Mac and Linux. Tags will be synced, as good as possible with current server side implementation. For the following points a change on server side is also necessary:

Full down sync of server side tag changes. Currently the client is not informed, if a tag to file is changed on the server side. Probably this could be done, by finally implementing the metadata_etag property to inform the client about that. Actually the synchronisation is done by coincidence or if the file was changed.
Full up sync is not possible, because system tags cannot updated via PROPPATCH. We actually safe client side tags via the oc:tags property. Later, if also system tags can be updated via PROPPATCH it should automatically handle tags as system tags, if present and as oc:tags, if not as system tag present. That means actually in practice: system-tags cannot be changed on client side. They will be overridden with the server side data frequently. Client side assigned tags which are no system tags are fully synchronised.

Following client side improvements cloud be done in the future:

implementation in windows client. Maybe the IPropertySet is the right way, but I have to investigate a bit more.. So that this could be a new pull request and finish this one as soon as possible.

jancborchardt · 2024-03-08T11:02:36Z

@tobiasKaminsky @mgallien are you aware of this nice pull request? :)

joecomo67 · 2024-03-17T11:13:11Z

this sounds great - is there a way to use this already?

Runtemund · 2024-03-17T13:30:08Z

@joecomo67

this sounds great - is there a way to use this already?

Actually I am awaiting review from the nextcloud staff. I don't know their priorities, so I have to wait.

Even if I am confident, that the pull request will solve the issue, I recommend to wait for their review, because I am not so deep into the nexcloud logic then they will be.

claucambra

Again, huge thanks for this PR and apologies for the late review. From me, some comments -- two small ones, and one bigger request before we can merge:

Your code style varies quite a lot from what we use in the desktop client, it would be great if you could run something like clang-format to format your changes in the same style as the rest of the app's code
Use of const for variables that are not changed after definition would be great
Most importantly, since this is a big PR that directly affects sync behaviour, it would be a requirement to see some unit tests for the additions here so that we can feel safer in including this in the next release

Thanks a ton for your time and effort in writing this PR

claucambra · 2024-04-10T06:54:43Z

src/csync/vio/csync_vio_local_unix.cpp

+
+#ifdef __APPLE__
+
+  // Create necessary system related objects
+  CFStringRef cfstr = CFStringCreateWithCString(kCFAllocatorDefault,
+                                                fullPath.constData(),
+                                                kCFStringEncodingUTF8);
+  CFURLRef urlref = CFURLCreateWithFileSystemPath(kCFAllocatorDefault,
+                                                  cfstr,
+                                                  kCFURLPOSIXPathStyle,
+                                                  dirent->d_type == DT_DIR);
+
+  // Query tags
+  CFArrayRef labels=NULL;
+  Boolean result = CFURLCopyResourcePropertyForKey(urlref,
+                                                   kCFURLTagNamesKey,
+                                                   &labels,
+                                                   NULL);
+
+  if(result==true && labels != NULL){
+    // Extract the labels to our array
+    int count = (int) CFArrayGetCount(labels);
+
+    if(count>0){
+      QStringList tagarray;
+
+      for(int index=0;index<count;index++){
+        CFStringRef str = (CFStringRef)CFArrayGetValueAtIndex(labels,index);
+        if(CFStringGetLength(str)>0)tagarray << QString::fromCFString(str);
+      }
+      tagarray.sort(Qt::CaseInsensitive);
+      QString tagList = tagarray.join(QChar(0x000A));
+      file_stat->tagList=tagList.toUtf8();
+    }
+  }
+
+  // Clean up
+  CFRelease(cfstr);
+  CFRelease(urlref);
+  if(labels!=NULL)CFRelease(labels);
+
+#endif
+
+#ifdef __linux__
+	const int sz=4096;
+	char buffer[sz];
+	int result = getxattr(fullPath.constData(),"user.xdg.tags",buffer,sz);
+
+	if(result>0)
+	{
+		// Data is store usually as comma(,) separated list.
+		// So we just need to replace ',' by '\n'.
+		QByteArray tagList = QByteArray(buffer,result);
+		tagList.replace(',','\n');
+		printf("%s\n",buffer);
+		file_stat->tagList= tagList;
+	}
+#endif
+


nitpick: the formatting of the code here does not match the rest of the code, please use 4-space indentation and leave spaces in between operators, as well as if statements and their parenthesis :)

claucambra · 2024-04-10T06:55:03Z

src/gui/folder.cpp

@@ -610,7 +609,7 @@ void Folder::slotWatchedPathChanged(const QString &path, ChangeReason reason)
        // an attribute change (pin state) that caused the notification
        bool spurious = false;
        if (record.isValid()
-            && !FileSystem::fileChanged(path, record._fileSize, record._modtime)) {
+            && !FileSystem::fileChanged(path, record._fileSize, record._modtime,record._tagList)) {


Suggested change

&& !FileSystem::fileChanged(path, record._fileSize, record._modtime,record._tagList)) {

&& !FileSystem::fileChanged(path, record._fileSize, record._modtime, record._tagList)) {

claucambra · 2024-04-10T06:55:27Z

src/libsync/discovery.cpp

+    if(localEntry.isValid()
+        && dbEntry.isValid()) {


Suggested change

if(localEntry.isValid()

&& dbEntry.isValid()) {

if(localEntry.isValid() && dbEntry.isValid()) {

claucambra · 2024-04-10T06:55:43Z

src/libsync/discoveryphase.cpp

@@ -344,6 +345,7 @@ void DiscoverySingleLocalDirectoryJob::run() {
        i.isVirtualFile = dirent->type == ItemTypeVirtualFile || dirent->type == ItemTypeVirtualFileDownload;
        i.isMetadataMissing = dirent->is_metadata_missing;
        i.type = dirent->type;
+        i.tagList=dirent->tagList;


Suggested change

i.tagList=dirent->tagList;

i.tagList = dirent->tagList;

claucambra · 2024-04-10T06:56:13Z

src/libsync/discoveryphase.cpp

+        if(property == "system-tags" || property == "tags"){
+            FileTagManager::fromPropertiesToTagList(result.tagList,value);
+        }
+        if(property == "metadata_etag"){
+            //TODO: RMD Handle metadata etag for tag synchronization
+        }


Suggested change

if(property == "system-tags" || property == "tags"){

FileTagManager::fromPropertiesToTagList(result.tagList,value);

}

if(property == "metadata_etag"){

//TODO: RMD Handle metadata etag for tag synchronization

}

if (property == "system-tags" || property == "tags") {

FileTagManager::fromPropertiesToTagList(result.tagList,value);

}

if (property == "metadata_etag") {

//TODO: RMD Handle metadata etag for tag synchronization

}

claucambra · 2024-04-10T06:57:29Z

src/libsync/filetags.cpp

+    if(gFileTagManger==NULL)gFileTagManger=new FileTagManager();
+    return gFileTagManger;


nitpick: again, some small complaints about formatting in this file vs. the rest of the application code

claucambra · 2024-04-10T06:58:28Z

src/libsync/filetags.cpp

+
+    if(result==true && labels != NULL){
+        // Extract the labels to our array
+        int count = (int) CFArrayGetCount(labels);


I would use c++-style static casts here rather than c-style casts

claucambra · 2024-04-10T06:58:58Z

src/libsync/filetags.cpp

+
+    while (!reader.atEnd()){
+        QXmlStreamReader::TokenType type = reader.readNext();
+        QString name = reader.name().toString();


Suggested change

QString name = reader.name().toString();

const QString name = reader.name().toString();

Could also use const auto

claucambra · 2024-04-10T07:00:05Z

src/libsync/filetags.cpp

+
+        if(count>0){
+            for(int index=0;index<count;index++){
+                CFStringRef str = (CFStringRef)CFArrayGetValueAtIndex(labels,index);


Suggested change

CFStringRef str = (CFStringRef)CFArrayGetValueAtIndex(labels,index);

const CFStringRef str = (CFStringRef)CFArrayGetValueAtIndex(labels, index);

Also would be good to use c++ casts

Runtemund · 2024-05-06T12:23:48Z

H, thanks for replay. I will work on that soon. Unfortunately I have much to do at work, so I will need some time.

claucambra · 2024-11-17T14:43:31Z

H, thanks for replay. I will work on that soon. Unfortunately I have much to do at work, so I will need some time.

Hi @Runtemund have you had a chance to take a look at this? We are quite interested in this PR 🙂

Other than the changes requested above, we'd also need test coverage of the changed you've made in order to be able to merge this safely -- do you have the capacity to work on this?

Thanks again!

Runtemund · 2024-12-02T11:43:27Z

Hi @claucambra
actually not, i guess i will have time over the holidays soon. However, I don't know exactly whether I'll be able to manage everything I want. Because getting involved in the unit tests and ensuring sufficient coverage is something you can hardly do on a voluntary basis alone. So if anyone here would like to join in and cover this part, I would be happy to continue.

Runtemund marked this pull request as draft January 30, 2024 23:45

claucambra requested review from mgallien, claucambra, allexzander and camilasan and removed request for mgallien and claucambra February 5, 2024 10:19

Uwe Runtemund and others added 21 commits February 14, 2024 12:38

Initial implementation for tac support on macOS

db2f260

Signed-off-by: Uwe Runtemund <[email protected]>

Added xattr support in linux

7ba3fcb

Signed-off-by: Uwe Runtemund <[email protected]>

Fix(l10n): Update translations from Transifex

e4befd7

Signed-off-by: Nextcloud bot <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Fix(l10n): Update translations from Transifex

991c9c5

Signed-off-by: Nextcloud bot <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

wait longer to get the contextual menu entries: may be necessary

b213514

Signed-off-by: Matthieu Gallien <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

fix(i18n): uppercase ID

df11c0f

Signed-off-by: rakekniven <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

fix(i18n): Changed spelling of "public key"

886d97b

Reported at Transifex Signed-off-by: rakekniven <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

chore: update workflows from templates

dcac579

Signed-off-by: John Molakvoæ <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

suppress deprecated warnings from openssl

2720921

is needed to avoid failing builds due to warnings unclear when we will tackle the work of removing the use of deprectaed APIs Signed-off-by: Matthieu Gallien <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Run swift-format on swift code

25a7bf1

Signed-off-by: Claudio Cambra <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Add .swift-format.json

88d83e3

Signed-off-by: Claudio Cambra <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Clean up file provider extension property declarations and init

096f560

Signed-off-by: Claudio Cambra <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Cllean up function and method calls in File Provider Extension

f451958

Signed-off-by: Claudio Cambra <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Fix(l10n): Update translations from Transifex

c115c39

Signed-off-by: Nextcloud bot <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Modernize networkReplyErrorString by using const auto.

a81e818

Signed-off-by: Camila Ayres <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Modernize extractErrorMessage and AbstractNetworkJob::errorStringPars…

b947b8a

…ingBody. - Add const auto. - Remove extra error message information to make it more user friendly. Signed-off-by: Camila Ayres <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Update 'no connection' error text to a more user friendly message.

f09cef7

Signed-off-by: Camila Ayres <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Show systray error message when there is a network error.

05a5c2b

Signed-off-by: Camila Ayres <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Fix(l10n): Update translations from Transifex

d9cd1a4

Signed-off-by: Nextcloud bot <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Do not create systray notification if there are no errors.

aecc92c

Signed-off-by: Camila Ayres <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

nextcloud-bot and others added 14 commits February 14, 2024 12:38

Fix(l10n): Update translations from Transifex

a370f4b

Signed-off-by: Nextcloud bot <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Fix(l10n): Update translations from Transifex

f3090e6

Signed-off-by: Nextcloud bot <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

docs(conffile) Update chunk sizes to match v2 chunking PR

8ae927a

Signed-off-by: Josh Richards <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Properly report Undefined sync status when multiple folders syncing

430bb58

Signed-off-by: Claudio Cambra <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Use auto and const auto where possible in trayOverallStatus

d3d1ad7

Signed-off-by: Claudio Cambra <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Use bools rather than unnecessary ints in folderman trayoverallstatus

001d773

Signed-off-by: Claudio Cambra <[email protected]> f Signed-off-by: Uwe Runtemund <[email protected]>

Do an early break of folder state check loop when errors seen

c7998a8

Signed-off-by: Claudio Cambra <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

fix binding loop with palette assignments

c0cc3b2

Signed-off-by: Matthieu Gallien <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

ensure role hideDownload from ShareModel has a boolean default value

1ea0fc8

Signed-off-by: Matthieu Gallien <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

fix more issues from qml in share dialog

638d32e

Signed-off-by: Matthieu Gallien <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Fix(l10n): Update translations from Transifex

666a751

Signed-off-by: Nextcloud bot <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Do not upload modified PDF file while it is open in Kofax PowerPDF on…

8abe081

… Windows. Prevents signature verification failure. Signed-off-by: alex-z <[email protected]> Signed-off-by: Uwe Runtemund <[email protected]>

Improved cliend side sync process for tags. Some code clean-up.

32021e0

Signed-off-by: Uwe Runtemund <[email protected]>

Runtemund force-pushed the addtagsupportmac branch from d73b218 to 32021e0 Compare February 14, 2024 11:38

Merge branch 'master' into addtagsupportmac

f883c34

Runtemund marked this pull request as ready for review February 14, 2024 11:58

claucambra requested changes Apr 10, 2024

View reviewed changes

joshtrichards added 3. to review enhancement enhancement of a already implemented feature/code labels Aug 13, 2024

claucambra added this to the 3.15.0 milestone Oct 11, 2024

mgallien modified the milestones: 3.15.0, 3.16.0 Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tag support in sync process #6400

Add tag support in sync process #6400

Runtemund commented Jan 30, 2024 •

edited

Loading

Runtemund commented Feb 14, 2024

jancborchardt commented Mar 8, 2024

joecomo67 commented Mar 17, 2024

Runtemund commented Mar 17, 2024

claucambra left a comment

claucambra Apr 10, 2024

claucambra Apr 10, 2024

claucambra Apr 10, 2024

claucambra Apr 10, 2024

claucambra Apr 10, 2024

claucambra Apr 10, 2024

claucambra Apr 10, 2024

claucambra Apr 10, 2024

claucambra Apr 10, 2024

Runtemund commented May 6, 2024

claucambra commented Nov 17, 2024

Runtemund commented Dec 2, 2024

	&& !FileSystem::fileChanged(path, record._fileSize, record._modtime,record._tagList)) {
	&& !FileSystem::fileChanged(path, record._fileSize, record._modtime, record._tagList)) {

	if(localEntry.isValid()
	&& dbEntry.isValid()) {
	if(localEntry.isValid() && dbEntry.isValid()) {

		if(gFileTagManger==NULL)gFileTagManger=new FileTagManager();
		return gFileTagManger;

	QString name = reader.name().toString();
	const QString name = reader.name().toString();

	CFStringRef str = (CFStringRef)CFArrayGetValueAtIndex(labels,index);
	const CFStringRef str = (CFStringRef)CFArrayGetValueAtIndex(labels, index);

Add tag support in sync process #6400

Are you sure you want to change the base?

Add tag support in sync process #6400

Conversation

Runtemund commented Jan 30, 2024 • edited Loading

Runtemund commented Feb 14, 2024

jancborchardt commented Mar 8, 2024

joecomo67 commented Mar 17, 2024

Runtemund commented Mar 17, 2024

claucambra 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

Runtemund commented May 6, 2024

claucambra commented Nov 17, 2024

Runtemund commented Dec 2, 2024

Runtemund commented Jan 30, 2024 •

edited

Loading