Skip to content

Conversation

tomasz1986
Copy link
Member

@tomasz1986 tomasz1986 commented Nov 4, 2022

gui: Show folder/device status on small screens

On larger screens, folder and device status is shown in a textual form
directly next to folder and device titles. However, on small screens,
only icons are currently shown, which may be ambiguous to new users, who
cannot possibly know what a specific icon means (see [1]). Thus, on
small screens only, display a new entry in folder/device info that
contains the same textual information that is shown in the title on
larger screens.

[1] https://forum.syncthing.net/t/android-list-of-devices/19251

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

Screenshots

Before:

image

image

After:

image
image

On larger screens, folder and device status is shown in a textual form
directly next to folder and device titles. However, on small screens,
only icons are currently shown, which may be ambiguous to new users, who
cannot possibly know what a specific icon means (see [1]). Thus, on
small screens only, display a new entry in folder/device info that
contains the same textual information that is shown in the title on
larger screens.

[1] https://forum.syncthing.net/t/android-list-of-devices/19251

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

For the record, the whole </span><span gymnastics is done to to prevent double spaces between icons and text in th due to line breaks combined with &nbsp;. I've also experimented with making the already present text in the title visible on small screens upon device/folder unfold to avoid code duplication, but I couldn't really make it look properly without breaking everything and/or covering other elements.

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.

In addition to the strange ></span><span dance, the duplication of string-key to icon / title mapping here seems excessive and bound to get out of sync when something is changed or added. As there is plenty of prior art, I'd suggest moving this to a common JS function instead.

@st-review
Copy link

🤖 beep boop

I'm going to close this pull request as it has been idle for more than 90 days.

This is not a rejection, merely a removal from the list of active pull requests that is periodically reviewed by humans. The pull request can be reopened when there is new activity, and merged once any remaining issues are resolved.

To permanently exempt this PR from bot nagging, add the slow-pr label.

@st-review st-review closed this Mar 10, 2023
@tomasz1986
Copy link
Member Author

Can we keep this one open? I'm still going to work on it when I've managed to find enough time.

@calmh calmh reopened this Mar 10, 2023
@calmh calmh added the slow-pr label Mar 10, 2023
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
@tomasz1986 tomasz1986 marked this pull request as draft September 16, 2023 21:42
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
@tomasz1986 tomasz1986 marked this pull request as ready for review September 18, 2023 15:56
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.

Just some quick remarks, didn't have time to look at the HTML changes thoroughly or do any testing yet.

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>
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.

Very nice, especially how it makes the HTML much more readable. Some nit-picks and the important question whether we want to replace percentage display with just an icon on small screens?

- keep the th tags on the same line
- always show syncing percentage
- sort only alphabetically in JS

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
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.

The current state is a bit inconsistent, see the longer inline comment.

Comment on lines 363 to 364
<span ng-switch="folderStatus(folder)">
<span ng-switch-when="syncing">({{syncPercentage(folder.id) | percent}}, {{model[folder.id].needBytes | binary}}B)</span>
Copy link
Member

@acolomb acolomb Sep 22, 2023

Choose a reason for hiding this comment

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

Maybe move the ng-switch to the <div> above, to have one less level of nested <span>s. That should help avoiding stray whitespace between them. The scanning case then needs to be added here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The same applies to scanning, of course. IIUC the intended behavior is this:

  • In the header: Scanning and syncing shows the percentage after the status text on large screens, and without the status text on small screens.
  • In the newly added table row: Any state just shows the status text, we don't need to repeat the percentage there. The icon always reflects the state icon shown in the header, except for scanning and syncing, where the icon is only visible here.

One problem with this is that I think without any icons, it can be confusing whether the folder is currently scanning or syncing. Although the former displays only percentage, and the latter percentage and size, I don't think this is clear to those who aren't already familiar with the internals.

How about just displaying both the icon, percentage, and size together? The icon doesn't take that much space...

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we could display the icon in addition. But frankly I don't think anyone cares what process is currently running, just that it's chugging along and how much is finished. At least in the overview - if you want more detail, there will be the added row with complete status.

Copy link
Member

Choose a reason for hiding this comment

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

I now agree that displaying the icons in all states (on small screens) is better. Simply because when scanning, there is an extended period of time where no percentage is yet available, thus without an icon we'd have no status at all.

@calmh calmh removed the slow-pr label Sep 30, 2023
acolomb and others added 6 commits November 30, 2023 21:15
Add the panel-status class to make sure there is a small gap when the
device name is long and closes in on the status fields.  Switch to a
<div> element instead of <span> for consistency with the folder
panels.  That better shows the intention to have a right-floating
block in relation to the device label text.
Get rid of an extra <span> wrapping, instead declaring the ng-switch
attribute on the panel-status <div> itself.

Scanning and syncing information is now always shown properly in xs
and larger sizes.

The icon on the panel header is always visible, because there are
short spans where the percentage is not yet known / valid, which would
otherwise lead to a completely blank space instead of the progress
numbers.

Fix a syntax error in folderStatusText() function (missing semicolon).
In the device panels, a separate remote-devices-panel class was
introduced to wrap icons with a "display: inline-block" style.  It is
not needed for pure text nodes though, but on both icons.  However, it
only works when applied in an outer wrapping element around the icon
<span>.

Rename it to more generic inline-icon class and apply to both folder
and device status panels where needed.
This matches up with folders again, and gets rid of the now no longer
needed <span> level.
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.

As this was my last suggested state, I'm obviously OK with it. But let's give @tomasz1986 another chance to look and test.

@tomasz1986
Copy link
Member Author

But let's give @tomasz1986 another chance to look and test.

Just tested and everything looks fine to me!

@acolomb
Copy link
Member

acolomb commented Dec 10, 2023

@calmh OK to merge, or shall we wait until v1.27.1 is out? LGTM from my side.

@calmh
Copy link
Member

calmh commented Dec 10, 2023

Merge away.

@acolomb acolomb merged commit d42fff1 into syncthing:main Dec 10, 2023
@calmh calmh added this to the v1.27.2 milestone Dec 11, 2023
calmh added a commit to cjc7373/syncthing that referenced this pull request Dec 11, 2023
* main: (69 commits)
  lib/nat: Fix test build failure (ref syncthing#9010)
  lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274)
  lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274)
  lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271)
  lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271)
  lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010)
  gui, man, authors: Update docs, translations, and contributors
  gui: Show folder/device status on small screens (syncthing#8643)
  lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271)
  build: Update dependencies (syncthing#9265)
  build: Revert specifics for Go 1.21.4, build using Go 1.21.5 (syncthing#9264)
  lib/fs: Reduce memory usage in xattrs handling (syncthing#9251)
  lib/model: Improve LastSeen handling (syncthing#9256)
  lib/scanner: Record inode change time for directories and symlinks (syncthing#9250)
  lib/api: Improve ignore loading error handling (fixes syncthing#9253) (syncthing#9254)
  gui, man, authors: Update docs, translations, and contributors
  lib/fs: Better equality comparison in mtimefs
  cmd/stcrashreceiver: Add metrics for diskstore inventory
  cmd/stcrashreceiver: Minor cleanup, stricter file permissions
  cmd/stcrashreceiver: Add metrics for incoming reports
  ...
calmh added a commit to calmh/syncthing that referenced this pull request Dec 16, 2023
* main: (89 commits)
  build: Update quic-go (fixes syncthing#9287)
  lib/model: Only handle relevant folder summaries (kqueue) (fixes syncthing#9183) (syncthing#9288)
  lib/model: Use a single lock (phase two: cleanup) (syncthing#9276)
  build(deps): bump actions/setup-go from 4 to 5 (syncthing#9279)
  lib/model: Use a single lock (syncthing#9275)
  cmd/syncthing: Better cli stdin handling (ref syncthing#9166) (syncthing#9281)
  cmd/syncthing: Mostly replace urfave/cli command line parser with alecthomas/kong (syncthing#9166)
  lib/nat: Fix test build failure (ref syncthing#9010)
  lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274)
  lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274)
  lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271)
  lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271)
  lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010)
  gui, man, authors: Update docs, translations, and contributors
  gui: Show folder/device status on small screens (syncthing#8643)
  lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271)
  build: Update dependencies (syncthing#9265)
  build: Revert specifics for Go 1.21.4, build using Go 1.21.5 (syncthing#9264)
  lib/fs: Reduce memory usage in xattrs handling (syncthing#9251)
  lib/model: Improve LastSeen handling (syncthing#9256)
  ...
calmh added a commit to calmh/syncthing that referenced this pull request Jan 14, 2024
* main:
  lib/model: Use a single lock (phase two: cleanup) (syncthing#9276)
  build(deps): bump actions/setup-go from 4 to 5 (syncthing#9279)
  lib/model: Use a single lock (syncthing#9275)
  cmd/syncthing: Better cli stdin handling (ref syncthing#9166) (syncthing#9281)
  cmd/syncthing: Mostly replace urfave/cli command line parser with alecthomas/kong (syncthing#9166)
  lib/nat: Fix test build failure (ref syncthing#9010)
  lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274)
  lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274)
  lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271)
  lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271)
  lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010)
  gui, man, authors: Update docs, translations, and contributors
  gui: Show folder/device status on small screens (syncthing#8643)
  lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271)
@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 Apr 9, 2025
@syncthing syncthing locked and limited conversation to collaborators Apr 9, 2025
@tomasz1986 tomasz1986 deleted the tomasz86/gui/improve-sync-status-on-small-screens branch August 11, 2025 12:05
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.

4 participants