Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gui: Mark devices that haven't connected for a long time (fixes #7703) #8530

Merged
merged 15 commits into from
Nov 3, 2022

Conversation

tomasz1986
Copy link
Contributor

@tomasz1986 tomasz1986 commented Sep 11, 2022

Currently, a disconnected device is marked as "Disconnected" without any
consideration whether the state is just temporary or rather it has been
like that for a long time, potentially requiring user intervention.

This commit adds a new state called "Disconnected (Inactive)" which is
shown for devices that have been disconnected for longer than a week. It
also changes the "Last seen" information, so that "Never" is used only
when the device hasn't connected at all, and the exact date is displayed
otherwise.

Additionally, when the date is older than 1 week, a note about that is
displayed below the date. Furthermore, when the date becomes older than
1 month, the note text colour changes to orange, and when it exceeds 1
year, the colour changes again to red. This is done to provide the user
with a visual clue that something may potentially be wrong and requires
a manual fix.

Signed-off-by: Tomasz Wilczyński twilczynski@naver.com

Screenshots

Before

image

After (ignore the date)

image

image

image

calmh
calmh previously approved these changes Sep 11, 2022
Copy link
Member

@calmh calmh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me! (Giving others a chance to weigh in, if nobody does feel free to merge.)

@tomasz1986
Copy link
Contributor Author

One thing that I am worried about is that "stale" may be tricky to translate.

@calmh
Copy link
Member

calmh commented Sep 11, 2022

Indeed. In Swedish it essentially becomes either just "old" (no real value judgement) or something akin to "mouldy"...

@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Sep 11, 2022

I usually like to check for technical terms in https://microsoft.com/language, which is obviously a little biased and limited to Windows and other Microsoft products, but the translations there are generally done by professional translators and are hence good quality. In case of "stale" though, the Polish translation is more like "outdated", "inactive", or "obsolete", which isn't exactly the same.

Although, to tell the truth, "inactive" doesn't sound bad, does it? Is it better than "stale"? Definitely easier to translate, that's for sure, but the meaning is quite not exactly the same…

On the other hand, the generic dictionary translation is similar to yours, and it's more about food (= not fresh) or something not interesting (= boring).

@acolomb
Copy link
Member

acolomb commented Sep 11, 2022

Agree that stale is difficult to translate and not very accurate. How about something involving "gone", "long gone" or "missing"? The latter is a common military term (MIA, missing in action) in English, but probably just as hard to translate or grasp. But maybe an inspiration?

Dated?

Red-shifted? ;-)

Another thought: why not just put a time directly in those parens? "Disconnected (8 days ago)" where anything below one week is not shown.

@imsodin
Copy link
Member

imsodin commented Sep 11, 2022

What about "Long disconnected" or "Disconnected for a long time" (latter seems too long, former might not really be English?). I wouldn't add more information to the "title bar" - I think that should stay as minimal as possible. On that note I also dislike the "more than a" line - to me that uses space for very little added value (the date already conveys the info). If you want the color thing, that could go on the date and that "more than" thing into a tooltip. However just a weak personal oppinion and suggestion, feel free to disregard. As in only engage the bikeshedding if you want, I am totally fine with merging as is.

@calmh
Copy link
Member

calmh commented Sep 11, 2022

Gone (long time no see)

@acolomb
Copy link
Member

acolomb commented Sep 11, 2022

Good direction, @calmh ... I guess we don't need the term "Disconnected" at all when "Gone" already implies that.

@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Sep 11, 2022

Thank you very much for all the ideas 😁! I will experiment and see what fits best here. I agree that the "Disconnected (something)" label should be rather short though, as very long strings are already problematic (e.g. they can overflow breaking the GUI, or they become truncated hiding useful information).

Another thought: why not just put a time directly in those parens? "Disconnected (8 days ago)" where anything below one week is not shown.

I like this suggestion quite a bit, but it may be impossible to properly translate as well, as in some languages (e.g. Polish) the noun form changes depending on the number 😕 (and here we would end up with a lot of different forms, i.e. "day(s)", "week(s)", "month(s)", "year(s)", etc.). Also, it could potentially be too long to display in full.

On that note I also dislike the "more than a" line - to me that uses space for very little added value (the date already conveys the info). If you want the color thing, that could go on the date and that "more than" thing into a tooltip.

I actually tested doing just that first, but the coloured date with nothing else looked kind of poor in terms of aesthetics 😞 (and of course, totally inaccessible on touch screens…). That's why I switched to the current format instead. Obviously, the note below the main part is based on the lines "Reduced by ignore patterns" and "Altered by ignore deletes" that are used similarly under folder local state.

…hing#7703)

Currently, a disconnected device is marked as "Disconnected" without any
consideration whether the state is just temporary or rather it has been
like that for a long time, potentially requiring user intervention.

This commit adds a new state called "Disconnected (Inactive)" which is
shown for devices that have been disconnected for longer than a week. It
also changes the "Last seen" information, so that "Never" is used only
when the device hasn't connected at all, and the exact date is displayed
otherwise.

Additionally, when the date is older than 1 week, a note about that is
displayed below the date. Furthermore, when the date becomes older than
1 month, the note text colour changes to orange, and when it exceeds 1
year, the colour changes again to red. This is done to provide the user
with a visual clue that something may potentially be wrong and requires
a manual fix.

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Sep 19, 2022

I've changed the name from "stale" to "inactive". I'm not 100% sure if it's the best option, but at least it seems similar enough in meaning, and it should be much easier to translate too. I did try the other alternatives out and browsed through various English synonym dictionaries looking for other examples, but I really think this should only be a single word (like the existing "unused"), so the number of possibilities is limited here.

I've also added an exception not to calculate the date when it equals the Unix epoch (i.e. 1970-01-01), and I've added yet another case of "Disconnected (Unused, Inactive)", which was missing previously. The screenshots show the current state.

Copy link
Member

@acolomb acolomb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't looked at the result in action, just commenting code. I think it's going in the right direction.

<span ng-switch-when="unused-disconnected"><span class="hidden-xs" translate>Disconnected (Unused)</span><span class="visible-xs" aria-label="{{'Disconnected (Unused)' | translate}}"><i class="fas fa-fw fa-unlink"></i></span></span>
<span ng-switch-when="unused-disconnected-inactive"><span class="hidden-xs" translate>Disconnected (Unused, Inactive)</span><span class="visible-xs" aria-label="{{'Disconnected (Unused, Inactive)' | translate}}"><i class="fas fa-fw fa-unlink"></i></span></span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one I find a bit confusing. If we don't share anything with a device, does it matter whether the connection had been established short or long ago?

Suggested change
<span ng-switch-when="unused-disconnected-inactive"><span class="hidden-xs" translate>Disconnected (Unused, Inactive)</span><span class="visible-xs" aria-label="{{'Disconnected (Unused, Inactive)' | translate}}"><i class="fas fa-fw fa-unlink"></i></span></span>
<span ng-switch-when="unused-disconnected-inactive"><span class="hidden-xs" translate>Disconnected (Unused)</span><span class="visible-xs" aria-label="{{'Disconnected (Unused)' | translate}}"><i class="fas fa-fw fa-unlink"></i></span></span>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your latest commit removed the whole case though, so nothing will be displayed for unused-disconnected-inactive. We should just display the same label as for unused-disconnected, since both are still true.

Copy link
Contributor Author

@tomasz1986 tomasz1986 Sep 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for noticing! I've fixed this now by reverting to what the code looked in the first iteration of the PR. What do you think?

Copy link
Member

@acolomb acolomb Sep 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant dropping the , Inactive part within parentheses. Just use the same label Disconnect (Unused) as stated above.

EDIT: Sorry I just noticed you changed the logic in the JS code. Might work, but I couldn't say for sure right now if it hurts any of the other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I've reverted the JS change and re-added the HTML. There's a lot of duplicated code in that whole HTML block, which I think could be simplified quite a bit, but I'll leave this for a different PR.

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
This reverts commit 325128e.

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
acolomb
acolomb previously approved these changes Sep 24, 2022
Comment on lines 1097 to 1103
return status + 'disconnected';
if ($scope.deviceStats[deviceCfg.deviceID].lastSeenDays >= 7) {
return status + 'disconnected-inactive';
} else {
return status + 'disconnected';
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a little nit: I'd check for unused here and not add the -inactive suffix then. The two states are the exact same in the web UI and I agree with that semantically: I think unused is the stronger/more relevant state than inactive, and having both doesn't add much IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
@@ -1096,7 +1096,7 @@ angular.module('syncthing.core')
}

// Disconnected
if ($scope.deviceStats[deviceCfg.deviceID].lastSeenDays >= 7) {
if (status !== 'unused-' && $scope.deviceStats[deviceCfg.deviceID].lastSeenDays >= 7) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer defining a unused variable before Line 1078 and using that in L1078 and here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Comment on lines 1075 to 1081

if ($scope.deviceFolders(deviceCfg).length === 0) {
var unused = true;
}

if (unused) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be correct JS, but scoping at least looks wrong - something like this I'd expect

            var unused =($scope.deviceFolders(deviceCfg).length === 0

            if (unused) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Is there any difference in scope though? Variables declared with var are available inside the whole function anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I meant: It likely is correct and doesn't matter, but looks wrong and definitely isn't elegant (to exagerate you never want to do if condition { conditionVariable = true } else { conditionVariable = false} when you can just do conditionVariable = condition.

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
imsodin
imsodin previously approved these changes Sep 26, 2022
@calmh calmh merged commit d8296ce into syncthing:main Nov 3, 2022
calmh added a commit to imsodin/syncthing that referenced this pull request Nov 8, 2022
* main: (36 commits)
  lib/protocol: Ignore inode time when xattr&ownership is ignored (fixes syncthing#8654) (syncthing#8655)
  lib/fs: Try to remove read only Windows files (fixes syncthing#3744) (syncthing#8650)
  gui: Add copy to clipboard, share by email, and share by SMS buttons to device IDs (fixes syncthing#2771, ref syncthing#3868) (syncthing#7984)
  gui, man, authors: Update docs, translations, and contributors
  build: Add GitHub actions build for Windows (syncthing#8627)
  gui: Fix connection type icon width (fixes syncthing#8592) (syncthing#8644)
  gui: Adjust connection type icon size scaling and alignment (syncthing#8645)
  docker: Use healthcheck endpoint (syncthing#8640)
  lib/connections: Use adaptive write size for rate limited connections (fixes syncthing#8630) (syncthing#8631)
  gui: Mark devices that haven't connected for a long time (fixes syncthing#7703) (syncthing#8530)
  gui: Fix rescan interval when add encrypted folder with watch for changes enabled (fixes syncthing#8570) (syncthing#8571)
  gui: Always show Out of Sync Items for remote devices (syncthing#8632)
  lib/fs: Let xattr test avoid non-test attributes (fixes syncthing#8601) (syncthing#8628)
  build: Add GitHub actions build for Windows
  gui, man, authors: Update docs, translations, and contributors
  gui: Display folder and device count number (syncthing#8615)
  gui, man, authors: Update docs, translations, and contributors
  lib/model, lib/protocol: Fix file comparisons (fixes syncthing#8594) (syncthing#8603)
  lib/scanner: More sensible debug output (syncthing#8596)
  gui: Allow automatic device ID selection on WebKit browsers (ref syncthing#8544) (syncthing#8597)
  ...
@calmh calmh added this to the v1.22.2 milestone Nov 9, 2022
calmh added a commit to calmh/syncthing that referenced this pull request Nov 22, 2022
* main: (23 commits)
  lib/fs: Optimize WindowsInvalidFilename (syncthing#8687)
  gui, man, authors: Update docs, translations, and contributors
  cmd/syncthing: Use main logger in generate subcommand (fixes syncthing#8682) (syncthing#8685)
  build: Update all dependencies (fixes syncthing#8679) (syncthing#8680)
  gui, man, authors: Update docs, translations, and contributors
  lib/model: Correctly set xattrs on temp files (fixes syncthing#8667) (syncthing#8670)
  gui: Automatically dismiss authentication reminder when in LDAP mode (fixes syncthing#8661) (syncthing#8663)
  lib/model: Correctly handle xattrs on directories (fixes syncthing#8657) (syncthing#8658)
  lib/protocol: Ignore inode time when xattr&ownership is ignored (fixes syncthing#8654) (syncthing#8655)
  lib/fs: Try to remove read only Windows files (fixes syncthing#3744) (syncthing#8650)
  gui: Add copy to clipboard, share by email, and share by SMS buttons to device IDs (fixes syncthing#2771, ref syncthing#3868) (syncthing#7984)
  gui, man, authors: Update docs, translations, and contributors
  build: Add GitHub actions build for Windows (syncthing#8627)
  gui: Fix connection type icon width (fixes syncthing#8592) (syncthing#8644)
  gui: Adjust connection type icon size scaling and alignment (syncthing#8645)
  docker: Use healthcheck endpoint (syncthing#8640)
  lib/connections: Use adaptive write size for rate limited connections (fixes syncthing#8630) (syncthing#8631)
  gui: Mark devices that haven't connected for a long time (fixes syncthing#7703) (syncthing#8530)
  gui: Fix rescan interval when add encrypted folder with watch for changes enabled (fixes syncthing#8570) (syncthing#8571)
  gui: Always show Out of Sync Items for remote devices (syncthing#8632)
  ...
@rubin55
Copy link

rubin55 commented Dec 6, 2022

I just noticed this change after updating today. One of my hosts has been off for about a year. I suppose a lot of users will see this for the first time.

Question: what are the consequences of a host being unavailable for a longer time? Wouldn't it just catch up with a lot of sync traffic? Is there a risk of dataloss?

Also, check this:
remote-devices-disconnected-inactive

In the above image, the warning is "just" "orange", even though the host is not available for a year minus 10 days; should it not be "red"?

@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Dec 6, 2022

The purpose of the message is just to provide information. There are valid use cases for having devices not connected for a longer period of time. However, when there are many devices, it's also easy to miss a specific one that haven't been syncing even though it was supposed to, hence the new "inactive" state that is visible in the main part of the Web GUI.

There's no risk of "data loss" per se, but if the device was modifying files that were supposed to sync right away yet for some reason they haven't (and at the same time you kept modifying them on other devices), you may then end up with a lot of conflicts once it finally connects.

When it comes to the colour, it will change to red as soon as the time period reaches 365 days 😉.

tomasz1986 added a commit to tomasz1986/syncthing that referenced this pull request Dec 25, 2022
… check (ref syncthing#8530)

The current code assumes that lastSeenDays is always set which is not
the case when device reports its lastSeen as equal to the Unix epoch.
Thus, make sure that lastSeenDays is set before proceeding with the
disconnected-inactive status check to avoid JS errors in the browser.

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
calmh pushed a commit that referenced this pull request Dec 26, 2022
… check (ref #8530) (#8730)

The current code assumes that lastSeenDays is always set which is not
the case when device reports its lastSeen as equal to the Unix epoch.
Thus, make sure that lastSeenDays is set before proceeding with the
disconnected-inactive status check to avoid JS errors in the browser.

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
calmh added a commit to calmh/syncthing that referenced this pull request Jan 23, 2023
* main: (69 commits)
  Handle relay connect timeout (fixes syncthing#8749) (syncthing#8755)
  gui, man, authors: Update docs, translations, and contributors
  build: Go 1.19.5
  gui, man, authors: Update docs, translations, and contributors
  script: Add weblatedl.go for downloading updated translations (syncthing#8723)
  gui: Allow to translate action and type in Recent Changes modal (syncthing#8548)
  gui, man, authors: Update docs, translations, and contributors
  gui: Fix undefined lastSeenDays error in disconnected-inactive status check (ref syncthing#8530) (syncthing#8730)
  gui, man, authors: Update docs, translations, and contributors
  gui, api: Indicate running under container (syncthing#8728)
  lib/fs: Use io/fs errors as recommended in std lib (syncthing#8726)
  build: Handle co-authors (ref syncthing#3744) (syncthing#8708)
  lib/fs: Watching is unsupported on android/amd64 (fixes syncthing#8709) (syncthing#8710)
  lib/model: Only log at info level if setting change time fails (syncthing#8725)
  lib/model: Don't lower rescan interval from default on auto accepted enc folder (fixes syncthing#8572) (syncthing#8573)
  gui, man, authors: Update docs, translations, and contributors
  gui: Remove unmaintained language variant nl-BE (syncthing#8722)
  gui, script: Fix indentation in lang-en.json to match others (syncthing#8721)
  docker: Ensure entrypoint is executable (syncthing#8719)
  Go 1.19.4
  ...
@tomasz1986 tomasz1986 deleted the tomasz86/gui/mark-stale-devices branch September 12, 2023 18:31
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Mar 10, 2024
@syncthing syncthing locked and limited conversation to collaborators Mar 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants