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: Remaining sync bytes in folder header (fixes #3908) #3928

Closed
wants to merge 9 commits into from

Conversation

benshep
Copy link
Contributor

@benshep benshep commented Jan 24, 2017

The progress indicator in the folder header in the GUI now shows the
remaining bytes to sync as well as the percentage.

The progress indicator in the folder header in the GUI now shows the
remaining bytes to sync as well as the percentage.
}

var bytes = $scope.model[folder].globalBytes - $scope.model[folder].inSyncBytes;
if (isNaN(bytes)) {
Copy link
Member

Choose a reason for hiding this comment

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

Here too.

@@ -722,6 +722,26 @@ angular.module('syncthing.core')
return Math.floor(pct);
};

$scope.syncRemaining = function (folder) {
// Formats the remaining sync bytes into a string with a suffix
Copy link
Member

Choose a reason for hiding this comment

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

Got a bit of a tab/space mixup here, by the looks of things.

@AudriusButkevicius
Copy link
Member

A screenshot how this looks would be nice.

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.

Need screenshot. :)

if (isNaN(bytes)) {
return 0;
}
var k = 1024;
Copy link
Member

Choose a reason for hiding this comment

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

We have a directive for this, {{ ... | binary}} I think.

@@ -285,7 +285,7 @@ <h4 class="panel-title">
<span ng-switch-when="idle"><span class="hidden-xs" translate>Up to Date</span><span class="visible-xs">&#9724;</span></span>
<span ng-switch-when="syncing">
<span class="hidden-xs" translate>Syncing</span>
({{syncPercentage(folder.id)}}%)
({{syncPercentage(folder.id)}}%, {{syncRemaining(folder.id)}})
Copy link
Member

Choose a reason for hiding this comment

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

It seems that it might be unclear what the amount means. Size? Transferred? Remaining?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to make it concise! Could add "to go" I suppose, but I think it should be pretty clear when you watch it for a few seconds and it goes down.

Tabs -> spaces
Gets rid of nasty transient 'NaN' display on start of sync.
@benshep
Copy link
Contributor Author

benshep commented Jan 24, 2017

Here's a screenshot.

image

I fixed the tab thing, and also put in an extra check for zero.

@calmh
Copy link
Member

calmh commented Jan 24, 2017

I think my comment about the directive got lost in the indentation change. There is no need to do formatting, we have a binary directive. Check the other places that display byte counts.

@AudriusButkevicius
Copy link
Member

I am still of an opinion that we're duplicating stuff, and I agree that it's not obvious what that number actually means, but I am not against it given you've produced a PR.

@uok
Copy link
Contributor

uok commented Jan 24, 2017

Looks like a good improvement, but I'm not sure about screen space on smaller screens. Keep in mind that some translations are a lot longer than "syncing". I was thinking about two lines, also adding ETA (#899) to the mix: e.g. first line "Syncing (55%)", second line "40 MB, 1h 3min"

@calmh
Copy link
Member

calmh commented Jan 24, 2017

There is literally no way to guess ETA from the outside. We have no idea at what rate their sync progresses. Also, they may have 250 GiB out of sync, and we'll show "250 GiB" with this change, but that might be a change to the permissions bits of a 250 GiB file which takes literally zero time and nothing to transfer or copy. This is one of the things I mentioned in the original issue - this number doesn't actually mean what most people would think it means.

@imsodin
Copy link
Member

imsodin commented Jan 24, 2017

The same argument applies to the sync percentage, making it useless in such a scenario, right?
The reason why I like displaying the absolute number of bytes more than percentage is mentioned in the original issue: No matter how big the folder, you will get some progress report. With the relative percentage it just duplicates the "syncing" (=99%) - "idle" (=100%) indication.

@fti7
Copy link
Contributor

fti7 commented Jan 25, 2017

+1 i think it could be an improvement in terms of showing progress to the User. You might want to use an icon to indicate what the Number does.
Of course it should be self-explanatory if the refresh interval is high enough/the decrease is fluid/snappy

ETA / Transferate would also be nice... but of course we need to gather this Data somehow first.
Also multiline might be an good Option.

singleline

Or Multiline:
multiline

multiline2

No need to do formatting in syncRemaining function, thanks @calmh.
@benshep
Copy link
Contributor Author

benshep commented Jan 25, 2017

@uok The longest I can find is Turkish! But the GUI seems to resize very gracefully on narrower displays. The box first expands vertically to accommodate the text:

image

Then the "Syncing" text is hidden altogether, so it's less of an issue:

image

The only GUI issue I can see is the "Syncing" text overlaps with the computer name in the right-hand box:

image

But that's a totally separate issue.

@benshep
Copy link
Contributor Author

benshep commented Jan 25, 2017

@calmh Yes, I get that showing ETA is hard. That should be a separate issue if people want it. I see your point that 250GiB might not mean 250GiB to transfer. I have some multi-GiB virtual machine files, and they usually sync very fast due to the hashing thing (thanks for that, BTW!). But in my "normal" use-case (which I admit might not be everyone's), 250GiB usually does mean 250GiB to transfer, and so it's usually meaningful. In any case, it's a conservative estimate, right?

@benshep
Copy link
Contributor Author

benshep commented Jan 25, 2017

@fti7 Nice mockups, but that text is a bit small. The idea of this is really an at-a-glance indication of how much is left to transfer.

Small down-pointing triangle next to "syncRemaining" byte count (thanks
@fti7).
@benshep
Copy link
Contributor Author

benshep commented Jan 25, 2017

@fti7 Here's one with a little icon.

image

@uok
Copy link
Contributor

uok commented Jan 25, 2017

Nice mockups, I really like the multi line with ETA very much! 👍
For me "ETA = (remaining size per folder / current global speed)" would be enough although I know that Syncthing re-uses blocks, etc. it would just be a rough estimate. But if your are e.g. on a mobile connection it would give you something to work with.

If we just keep % and MB at the moment, I think the little icon is not needed, maybe use | or - as a separator.

@calmh
Copy link
Member

calmh commented Jan 25, 2017

current global speed

There is no such thing that we're currently aware of. Not that a device couldn't itself tell us this, it's just not part of the protocol at the moment.

@Ferroin
Copy link

Ferroin commented Jan 25, 2017

I'd argue that the icon shown could be misunderstood to indicate that the given number is the download rate, not the amount remaining.

I also kind of agree with @uo, regarding the ETA, but some slightly more complicated math would give a better estimate, Something along the lines of:
(total remaining data for this folder / (current download speed / % of total state across all folders that need to be synced that this folder requires))
should give a slightly more accurate representation. FWIW that's essentially the math used by most web browsers and download managers, they just don't need the division to calculate the stream bandwidth for each file because each file is it's own stream.

@benshep
Copy link
Contributor Author

benshep commented Jan 25, 2017

OK, removed the icon.

@calmh
Copy link
Member

calmh commented Jan 25, 2017

Ok, lets shelf the discussions on a theoretical ETA and transfer rate that is not currently in evidence and focus on the PR at hand...

@benshep I'm mostly fine with this. I'd like to preemptively suggest only showing it when the number is >0 though. There are known cases where it will turn negative, which already affects the percentage weirdly. We don't need to add insult to injury by also claiming that there are -542 KiB left to sync.

In edge cases, bytes remaining may show less than zero. Return zero in
these cases.
If remaining bytes evaluates to zero (including negative or NaN), hide
the (100%, 0 B) part.
@benshep
Copy link
Contributor Author

benshep commented Jan 26, 2017

Slightly out of scope of this issue, but now the percentage and bytes are hidden when there's nothing useful to show (100%, 0 B).

image

@calmh
Copy link
Member

calmh commented Jan 26, 2017

All good

@calmh
Copy link
Member

calmh commented Jan 26, 2017

@st-review merge it

@st-review
Copy link

👌 Merged as e03be91. Thanks, @benshep!

@st-review st-review closed this Jan 26, 2017
st-review pushed a commit that referenced this pull request Jan 26, 2017
The progress indicator in the folder header in the GUI now shows the
remaining bytes to sync as well as the percentage.

GitHub-Pull-Request: #3928
@calmh
Copy link
Member

calmh commented Jan 26, 2017

(For the record, in much of the discussion above I was, erroneously, thinking about showing this for remote devices, not local folders. My misunderstanding. We can of course calculate a sync rate and an ETA locally. Carry on.)

@st-review st-review added the pr-merged Legacy label used in the past for pull requests merged to the main tree label Jan 15, 2018
@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 Jul 15, 2018
@syncthing syncthing locked and limited conversation to collaborators Jul 15, 2018
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 pr-merged Legacy label used in the past for pull requests merged to the main tree
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants