bcpierce00 / unison

Unison file synchronizer
GNU General Public License v3.0
4.07k stars 229 forks source link

Add days to ETA display (Nd hh:mm:ss instead of hh:mm:ss) to improve >>1h readability #1038

Open mvglasow opened 4 months ago

mvglasow commented 4 months ago

[EDIT: dropped because master is hh:mm:ss. When displaying progress, ETA is currently given in mm:ss. While this is no issue for times up to 60 minutes, it gets hard to read when ETA is several hours]

Suggestion: change ETA format to something like 1d 18:56:19. Days can be omitted if ETA is less than one day. Hours can also be omitted if ETA is less than an hour – or can be left in to make it clear that ETA is shown as mm:ss, not hh:mm.

gdt commented 4 months ago

Calling this an enhancement request; seems reasonable. I would prefer to leave mm:ss as is when sub-hour; the time will be counting down and I don't think it's actually confusing.

tleedjarv commented 4 months ago

ETA is currently given in mm:ss

Please clarify if you are talking about native macOS GUI. Or attach a screenshot of where you see this.

ETA is currently displayed as hh:mm:ss (also for sub-hour). I don't think adding Nd is worth the effort. How often does one come across >24-hour syncs?

mvglasow commented 4 months ago

This was observed in unison.log contents, in conjunction with the text GUI. (Both machines involved run Linux.)

Use cases may differ – occasionally I do in fact sync several GB of data over a low-bandwidth connection, which does take several days. So for some cases it is definitely helpful, and shouldn’t be difficult to implement if one knows where to look in the code and is familiar with OCaml. Essentially, it’s just if hh >= 24 then {dd == hh mod 24; hh == hh div 24} else dd == 0.

Btw, if different UIs display time differently, this is a sign of code duplication – it’s probably better to move time formatting code into a library function which can be called from wherever it’s needed.

tleedjarv commented 4 months ago

This was observed in unison.log contents, in conjunction with the text GUI. (Both machines involved run Linux.)

Interesting... could you post an example?

So for some cases it is definitely helpful, and shouldn’t be difficult to implement if one knows where to look in the code and is familiar with OCaml. Essentially, it’s just if hh >= 24 then {dd == hh mod 24; hh == hh div 24} else dd == 0.

The code for the latest release is located here: https://github.com/bcpierce00/unison/blob/v2.53.5/src/uicommon.ml#L496-L506

mvglasow commented 4 months ago

A few lines from unison.log:

 22%  1507:37 ETA
 22%  1511:17 ETA
 22%  1514:05 ETA
 22%  1516:53 ETA
 22%  1519:15 ETA
tleedjarv commented 4 months ago

I see. This must be some older version then because this should not happen with the current code. Could you try with the latest release?

mvglasow commented 4 months ago

I was about to write that this output doesn’t look like it came from the code you linked.

This was with 2.52.1.

Trying with the latest release will take a while, as it’s an arm system and I either need to upgrade the entire OS (which is due anyway) or spin up my old guest VM to build Unison myself, and then wait for the next large sync job.

gdt commented 4 months ago

Feedback tag set. Happy to wait until you get updated. Hard to say about days vs lots of hours; it feels to me like it's rare and 57h isn't actually confusing. So I mildly lean to wontfix, and I'm always steering to less code, whenever reasonable.

tleedjarv commented 4 months ago

and then wait for the next large sync job.

or you could probably also simulate an extremely slow network.

mvglasow commented 4 months ago

No need to simulate – the remote end is on a residential DSL line in Germany, and for the test I just reversed the direction (transferring from rather than to Germany).

Starting sync from my laptop, I was able to use an x86_64 binary from CI.

Progress both on the console and in the log file (when running with -batch) was reported in the following form:

  0%  0/1  (46.1 MiB of 263.59 GiB)   162 KiB/s    475:35:00 ETA

So ETA is getting displayed in hh:mm:ss format in Unison 2.53.5; 475h would be 19d 19h.

mvglasow commented 4 months ago

I’ve added PR #1043. @gdt it adds days to ETA (we got hours already), I’ll fix the subject line