Martchus / syncthingtray

Tray application and Dolphin/Plasma integration for Syncthing
https://martchus.github.io/syncthingtray/
Other
1.6k stars 43 forks source link

Tray Icon for Conflicts #140

Open metal450 opened 2 years ago

metal450 commented 2 years ago

Relevant components

Is your feature request specific to a certain platform/environment? Please specify. Nope

Is your feature request related to a problem? Please describe. SyncThing Tray's icons are very for showing when a sync is in process, or there's some error state. However, one status that is easily missed are conflicts - I often find myself navigating into the sync dir, only to discover that there's been a conflict there for awhile that went unnoticed. It would be very useful if the tray icon could also show an indicator when a conflict exists (reusing the "warning" indicator would be fine IMO, so as to eliminate the need for any extra UI work, though a separate/explicit icon would also be great too). A different tray icon to indicate a conflict is really the sole feature I miss from SyncTrayzor, which is Windows only.

Describe the solution you'd like When a conflict is present, show the tray icon in a warning state (or a separately configurable UI icon).

Describe alternatives you've considered n/a

Additional context n/a

Martchus commented 2 years ago

Sounds useful, indeed. Not sure whether the official GUI shows this information (and therefore Syncthing provides an API to query it). If not, maybe it'll be better to implement it in Syncthing first (instead of doing this work on GUI level).

metal450 commented 2 years ago

Hmm, not sure how SyncTrayzor detects it then? They actually have a whole built-in conflict resolver too, which is pretty cool (screenshot below), but perhaps a bit overkill haha. Still, they seem to be detecting with a lot of specificity exactly what files are conflicting & where

2022-06-04 11 26 13

2022-06-04 11 27 32

Martchus commented 2 years ago

That yellow notification doesn't look in-line with the official UI at all so I suppose it is custom code. I personally would refrain from doing any file lookups or even modifications in the GUI level. That's the job of the Syncthing backend and having two processes manipulating those files is rather messy. Doing it in the GUI also has the disadvantage that this particular feature would only work locally and if the GUI process has sufficient read/write access (which might differ from the backend process). So it makes more sense to check what the official API has to offer and perhaps create an upstream Syncthing feature request first.

metal450 commented 2 years ago

That yellow notification doesn't look in-line with the official UI at all

Oh yeah, that window essentially wraps the browser UI. More or less like showing a notification at the top of a Firefox window.

I personally would refrain from doing any file lookups or even modifications in the GUI level. That's the job of the Syncthing backend and having two processes manipulating those files is rather messy.

As far as file modifications, yeah, no need to mess with files or a resolver or anything - was just showing how that other app does it out of possible interest.

Doing it in the GUI also has the disadvantage

Just to clarify, all I was really hoping for is a distinguishing tray icon & nothing more. Knowing that a conflict exists somewhere is more than sufficient (can just look at the log to find it if not immediately apparent). The main problem is when there's one, but I'm not aware of it.

So it makes more sense to check what the official API has to offer and perhaps create an upstream Syncthing feature request first.

Looks like this is where SyncTrayzor's author was discussing it some years back: https://forum.syncthing.net/t/conflict-notification-via-rest-api/9255/6

"If you watch both the “conflict created” events, and the “file updated” events, you should catch all conflicts."

Martchus commented 2 years ago

Ok, although I'm currently not sure to which exact events (from the documentation) these correspond to. Maybe it is also required to use the normal REST-API to get the initial status.

Normally I'm just checking what requests the official GUI does and do the same but it seems that the official GUI doesn't show this info so far.

metal450 commented 2 years ago

Hmm...Looking at that, it seems the ones that would make the most sense are

https://docs.syncthing.net/events/localchangedetected.html and https://docs.syncthing.net/events/remotechangedetected.html.

Then there's a notification each time a file has changed, & it's only necessary to quickly glance at its filename to see if it contains the conflict string, right? If it does, set the conflict icon & store the filename in hashmap (so that if a future change, whether local or remote "un-conflicts" the same file, it would remove the icon).

metal450 commented 2 years ago

Normally I'm just checking what requests the official GUI does and do the same but it seems that the official GUI doesn't show this info so far.

Yeah, to be honest I'm not sure how useful this would even really be in the official GUI, since most people running SyncThing just let it go in the background, & only open the UI when it's time to config. That's why tools like SyncThing Tray & SyncTrayzor are so useful - it provides a more UI-centric way to use the software, with tray icon indicators of what's going on, without needing to bring up the whole UI :)

Martchus commented 2 years ago

I'm already looking for LocalChangeDetected and RemoteChangeDetected events. However, those do not help to detect conflicts that occurred before Syncthing Tray has connected. It also doesn't look like these would be emitted for conflicts.

Yeah, to be honest I'm not sure how useful this would even really be in the official GUI

Even if not, the backend part should still be in the backend and not in the GUI. And currently it doesn't look like the backend (Syncthing itself) provides an API for this. I'll ask on the Syncthing Matrix or IRC channel for help.

Martchus commented 2 years ago

The main Syncthing maintainer's answer is a clear no: https://forum.syncthing.net/t/yet-another-syncthing-tray/8502/165

I suggest filing an upstream issue for such an API first.

metal450 commented 2 years ago

Yeah, looks like SyncTrayzor shows their little icon by simply looking at the filesystem too: https://github.com/canton7/SyncTrayzor/blob/1ec7e4fadd5e7e64fb56348de630b85ccab35f07/src/SyncTrayzor/Services/Conflicts/ConflictFileManager.cs#L105

Martchus commented 2 years ago

Still not the right approach in my opinion.

metal450 commented 2 years ago

Fair enough. Personally, I could see an argument either way: if viewed as a frontend+backend system, you're right in that it's more logical to have all file watching take place on the backend. However, I could also see it as unlikely to be added there, given that watching conflicts doesn't provide much use on the the "core" backend itself, & this tray app is more of a "non-core" addon.

Since you have more experience with their dev process, how are they typically with stuff like this? i.e. do requests normally get punted for years (& thus it's unlikely to see such a nice conflict tray icon available on Linux), or they're fairly responsive?

Martchus commented 2 years ago

I suppose it depends on the request and it's usefulness/effort ratio. Judging by Syncthing's code which I've read to search for an API I would guess the effort is quite high since Syncthing does not really keep track of these conflict files in the database after they have been created. But I could be totally wrong here.

Note that it is open source so you can already go ahead and implement it yourself (after clarifying whether a certain approach would be generally acceptable).

metal450 commented 2 years ago

Note that it is open source so you can already go ahead and implement it yourself

Unfortunately, outside of my ability at this time.

I would guess the effort is quite high

Yeah, my fear/assumption was the same. Kinda why I'd hoped there might be a willingness for something similar to SyncTrayzor, as even if not strictly ideal in terms of internal implementation, from a usability perspective the net result would be the same. If not then not, I guess just stick with finding out about the conflicts from the Windows side.

metal450 commented 2 years ago

For anyone else interested in this, I cobbled together a stopgap solution to notify about conflicts on Linux, if or until this gets implemented :)

#!/bin/bash

monitor(){
    # Watch for files with "sync-conflict" in the name
    inotifywait --monitor --recursive -e create,moved_to --include "sync-conflict" --format '%w%f' "$1" |
    while read NEWFILE; do
        if [[ "${NEWFILE}" =~ .*tmp$ ]]; then
            : #Ignore .tmp files (every syncing file starts as tmp, so notifying for those makes every notification happen 2x)
        else
            # Use sed to parse out just the final dirname/filename
            dirAndFilename=$(echo "${NEWFILE}" | sed -E 's/(.*)\/(.*)\//\2\//')
            # Show system notification. --hint makes it appear as syncthing.
            notify-send "${dirAndFilename}" --hint='string:desktop-entry:syncthing-ui'
        fi
    done
}

monitor "/your/syncthing/folder1/" &
monitor "/your/syncthing/folder2/" &
monitor "/your/syncthing/folder3/"

Note: If the version of inotifywait in your distro's repo doesn't support the --include option (it was added recently), install it from source per https://unix.stackexchange.com/questions/323901/how-to-use-inotifywait-to-watch-a-directory-for-creation-of-files-of-a-specific/606213#606213

Other references:

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

metal450 commented 1 year ago

no activity != solved.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

metal450 commented 1 year ago

no activity != solved.

Martchus commented 1 year ago

no activity -> Is someone still interested in this?

Apparently the answer is yes :-)

cjc7373 commented 1 year ago

I think what we need here is an endpoint from syncthing that returns every file matches the pattern *.sync-conflict-*. Maybe an extension of /rest/db/browse? I'll look into that..

And then syncthingtray can monitor LocalChangeDetected and RemoteChangeDetected events to get notified of subsequent conflicts.

Martchus commented 1 year ago

Sounds generally reasonable from my side.

Of course it needed to be implemented in an efficient way. Just going though the filesystem each and every time a client connects that wants to show this kind of information is maybe not the best idea.

I suppose LocalChangeDetected and RemoteChangeDetected also needed to be extended to cover conflicts. At least I'd suspect those events are currently not emitted for conflict files. If I'm right, then I'm also not sure whether emitting those events by default for conflicts would be a good idea considering backwards compatibility. Maybe a new event type would be better.

cjc7373 commented 1 year ago

Well I just tested that, and these events do emit for conflicted files, like:

[{
    "id": 1418,
    "globalID": 1665,
    "time": "2023-03-12T01:22:58.620227647+08:00",
    "type": "RemoteChangeDetected",
    "data": {
      "action": "modified",
      "folder": "xxx",
      "folderID": "xxx",
      "label": "",
      "modifiedBy": "S37NE43",
      "path": "tmp/test.sync-conflict-20230311-172258-BQ6TKV6.txt",
      "type": "file"
    }
  },
  {
    "id": 1419,
    "globalID": 1696,
    "time": "2023-03-12T01:27:46.258095946+08:00",
    "type": "LocalChangeDetected",
    "data": {
      "action": "deleted",
      "folder": "xxx",
      "folderID": "xxx",
      "label": "",
      "modifiedBy": "BQ6TKV6",
      "path": "tmp/test.sync-conflict-20230311-172258-BQ6TKV6.txt",
      "type": "file"
    }
  }]
Martchus commented 1 year ago

Good, that makes things easier (especially considering that Syncthing Tray already monitors those events in general).

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

metal450 commented 1 year ago

not stale

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

metal450 commented 1 year ago

not stale

metal450 commented 1 year ago

Not sure why this auto-closed, it's not finished nor stale (per prior comment)

stale[bot] commented 10 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

metal450 commented 10 months ago

Seems odd to have to keep commenting on this over & over & over. Shouldn't be closed unless it's actually done.

Martchus commented 10 months ago

It is unfortunately common that people create issues, then lose interest but never close the issue. So I added the bug to keep the tracker free of such issues.

I'm afraid nobody currently wants this feature bad enough to actually take the effort of implementing it. Maybe I'll work on it at some point but I won't promise anything.

metal450 commented 10 months ago

I'm afraid nobody currently wants this feature bad enough

Not sure I'd equate "nobody is coming here to proactively request it" with "nobody would benefit from it." Many product improvements (the vast majority?) don't come from direct user requests.

Martchus commented 10 months ago

I didn't mean to say nobody would benefit from it. I'm just observing that nobody has created a PR yet; so apparently nobody thinks this is worth the effort. And I'm on the fence myself. (Currently e.g. porting to Plasma 6 is more important to me.)

(And by the way, we don't need an open issue for this to move forward; just someone who spends the effort to come up with a mergable solution. The stalebot closing this issue would not prevent that.)

metal450 commented 9 months ago

Sure, but if it's not an issue, it's not on the "Wish List" & would probably be forgotten?

stale[bot] commented 7 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

metal450 commented 7 months ago

not stale

stale[bot] commented 5 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

metal450 commented 5 months ago

not stale

stale[bot] commented 3 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

metal450 commented 3 months ago

not stale

stale[bot] commented 1 month ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

andreymal commented 1 month ago

@stale "stale" doesn't mean fixed, I guess?

Martchus commented 1 month ago

It just means noone is working on this or even just discussing this further. Not sure when/whether I'll implement this feature (or someone else will give it a try).

soredake commented 1 month ago

@Martchus why this bot even trying to close feature requests when they are supposed to be exempt from it?

Martchus commented 1 month ago

I don't know. I guess the exclusion worked at least at some point for "enhancement". Maybe it breaks if the tag contains a white-space. But I don't find it that bad to provoke some reactions also on feature requests to know whether someone is actually still interested in that. And if the ticket is eventually closed it doesn't really make a difference as well¹.


¹ Whether this is implemented or not does not depend on having an open ticket about it. It depends on someone implementing it.

metal450 commented 1 month ago

Personally I still find it odd that I have to keep coming back & saying "not stale" over & over. I interact with a lot of projects on Github & this is the only place I've ever had to do this.

Like...there really isn't a reason a long-term wishlist needs to be constantly emptied - it's just a wishlist.

I guess maybe after a few years it could be stale or something. But needing more "reactions" every 2 months to be considered as "interest hasn't been lost" seems extremely frequent.

Martchus commented 1 month ago

Let's see whether using "enhancement" works.