Sync transfers for realistic progress bar updates by vjr · Pull Request #2819 · elementary/files

vjr · 2026-08-28T07:08:54Z

BEFORE:

files-progress-before.webm

AFTER:

files-progress-after.webm

vjr · 2026-08-28T07:19:09Z

I see the progress bar dialog is not "modal" so if I click the unmount button while the transfer is in progress Files crashes - but this is a different issue that already exists in the current main branch.

jeremypw · 2026-08-28T13:13:02Z

The value of 15 was set in 2011 by the original author (!) Certainly not appropriate for modern hardware especially SSD. I vaguely remember thinking that the linked issue was something to do with caching the write but if it fixes the issue for you there seems little harm in reducing this value. I'll check the code where this value is used though.

jeremypw · 2026-08-28T13:16:23Z

I see the progress bar dialog is not "modal" so if I click the unmount button while the transfer is in progress Files crashes - but this is a different issue that already exists in the current main branch.

The dialog is intended to be modal so we will have to block unmounting of any volume being written to (I am surprised the system does not do that) and show a warning if necessary. Doesn't the system show a "this volume is busy" warning?

jeremypw · 2026-08-28T13:28:00Z

I wonder whether this constant can be removed altogether or reduced to a small value 🤔 How fast can a reasonably accurate transfer rate be known? Maybe better to use a minimum number of bytes transferred rather than a fixed time?

vjr · 2026-08-28T13:40:59Z

I see the progress bar dialog is not "modal" so if I click the unmount button while the transfer is in progress Files crashes - but this is a different issue that already exists in the current main branch.

The dialog is intended to be modal so we will have to block unmounting of any volume being written to (I am surprised the system does not do that) and show a warning if necessary. Doesn't the system show a "this volume is busy" warning?

No warning, the app quits (crashes) if I click unmount while transfer is in progress. Should I open another PR to force the transfer dialog to be modal and not allow interacting with the app?

vjr · 2026-08-28T13:42:46Z

I wonder whether this constant can be removed altogether or reduced to a small value 🤔 How fast can a reasonably accurate transfer rate be known? Maybe better to use a minimum number of bytes transferred rather than a fixed time?

I've reduced the const down from 15 seconds to 3 seconds - seems reasonable for both slow and fast disks/hardware... if the hardware is fast the copy dialog completes and closes quickly anyway, if the hardware is slow then about 3 seconds seems okay for the transfer speed indication to show up?

vjr · 2026-08-28T13:44:46Z

The value of 15 was set in 2011 by the original author (!) Certainly not appropriate for modern hardware especially SSD. I vaguely remember thinking that the linked issue was something to do with caching the write but if it fixes the issue for you there seems little harm in reducing this value. I'll check the code where this value is used though.

Yep I thought as much haha. Thanks for looking at this PR. I think we can leave this reduced value in place for now, dont want to mess with further code changes to use a different way of calculating the duration before transfer speeds show up.

flodavid · 2026-08-28T13:58:29Z

Doesn't the system show a "this volume is busy" warning?

I have had the same issue. Another app failed to unmount one of my hard drive volume saying “This volume is busy”. So I tried with Files and it just crashed.

jeremypw · 2026-08-28T14:07:14Z

@flodavid Could you open an issue regarding your crash please? I'll try and look into file transfers again soon.

vjr · 2026-08-28T14:18:22Z

@flodavid Could you open an issue regarding your crash please? I'll try and look into file transfers again soon.

@jeremypw do you want me to make the transfer dialog modal thus preventing user interaction with the main app while it's visible? i can do that in this PR itself presuming it's a "one liner" change?

jeremypw · 2026-08-28T14:45:26Z

@Vishal No, I think how Files deals with trying to unmount a busy volume is better addressed with a separate issue/PR. File transfers should be non-modal and occur in the background if possible. Not sure how to stop people pulling out a USB stick prematurely We need to get a true (i.e. unaffected by caching) indication of whether a file transfer is ongoing/complete and show a warning and/or style the sidebar item approriately.

vjr · 2026-08-28T14:52:03Z

@Vishal No, I think how Files deals with trying to unmount a busy volume is better addressed with a separate issue/PR. File transfers should be non-modal and occur in the background if possible. Not sure how to stop people pulling out a USB stick prematurely We need to get a true (i.e. unaffected by caching) indication of whether a file transfer is ongoing/complete and show a warning and/or style the sidebar item approriately.

@jeremypw okay fair enough - so is this PR acceptable as an interim solution to make it more evident to users that a transfer is still in progress and to not unplug or unmount while so? further fixes/improvements in future PRs?

jeremypw · 2026-08-28T18:41:25Z

@vjr Yes. I wasn't able to test this today. Should do it tomorrow.

jeremypw

This appears to slow file transfers significantly. Maybe because the progress is more realistic? But it also seems to slow internal transfers between SSDs noticeably. I wonder if the syncing can be done less often and/or only when transferring to an external drive? Or should that wait for a future PR?

I noticed several areas that need cleaning up and reviewing in the file operations area (and it is still awaiting complete transfer to Vala) anyway.

I experimented with only syncing when a certain number of bytes had been transferred which seemed to reduce the impact on speed.

vjr · 2026-08-30T06:30:57Z

Yes I see the impact of the sync () calls slowing down transfers on fast media , it was not noticeable on the very slow media.

Plus, it might be that the global posix sync () call is not a good thing especially if there are multiple transfers in progress.

Marking this PR as draft while I come up with a better approach.

jeremypw · 2026-08-30T11:24:43Z

it was not noticeable on the very slow media.

I tested on a slow antique MTP player tranferring many small (100 byte) files and the reported transfer rate was about half with this PR. I must admit I did not manually time the complete transfer but cancelled before it finished. The situation is complicated by the fact that when transferring many small files the transfer slows down so the predicted time left is not very helpful - it often goes up instead of down. This was reported long ago but has never really been diagnosed. I think it may be to do with the "undo" processing taking longer and longer but I haven't dug into file operations nitty-gritty for a long time.

Replace g_file_copy () and g_file_move () with custom copy_move_with_sync () method which conditionally calls fsync () periodically.

vjr · 2026-08-30T17:09:29Z

@jeremypw I've added a (WIP) custom copy_move_with_sync () intended to be a drop-in replacement for the g_file_copy () and g_file_move () glib/gio methods.

Yes, the periodic g_fsync () calls seem to be slowing down the transfers compared to the main branch so I'll boot up live sessions of ubuntu and kubuntu to compare against gnome and kde file managers, see what they do and how fast or slow they are.

I just tested individual file transfers of various sizes on two usb storage I have, a fast ssd and a slow flash stick, also tried multiple small files.

At the moment this new approach needs work to fix crashes/errors trying to copy directories, haven't yet tried other copy/move styles, folders, overwrite, recurse whatever.

Will continue working on this, see if the original goal of the issue (avoid users inadvertently unplugging their usb devices) can be resolved.

Thanks for looking into this.

flodavid · 2026-08-30T18:19:51Z

@jeremypw here it is: #2821. I assigned you to the issue.

jeremypw · 2026-08-30T21:00:08Z

@vjr Thanks for working on this! At the end of the day a (modest) reduction in speed is worth it to fix serious issues like crashes and data loss. I'll have another look at this tomorrow.

jeremypw · 2026-09-04T11:01:47Z

If you are interested in doing more work on file operations there is a long outstanding issue #137 which it would be good to fix! No pressure though.

jeremypw · 2026-09-04T11:14:30Z

One thing I am not too sure about: what happens when an error is thrown e.g. at line 127 in CopyMoveJob.vala? Are streams left open? Would it be better to handle errors internally and have finally clauses to ensure cleanup? We can always throw our own error afterwards.

vjr · 2026-09-04T11:33:47Z

One thing I am not too sure about: what happens when an error is thrown e.g. at line 127 in CopyMoveJob.vala? Are streams left open? Would it be better to handle errors internally and have finally clauses to ensure cleanup? We can always throw our own error afterwards.

IINM (might have read this on the vala.dev website) it auto closes the stream when going out of scope aka final ref released, regardless of success or failure or error thrown/returned?

vjr · 2026-09-04T11:35:28Z

If you are interested in doing more work on file operations there is a long outstanding issue #137 which it would be good to fix! No pressure though.

Yep I think I can give this a shot, see if I can fix.

vjr · 2026-09-04T11:39:58Z

@vjr This seems to be working well afaict! The progressbar now stays open until the transfer onto a unpluggable device completes and the progress bar actually reaches 100%.

There are code style issues but I don't mind fixing those after merging if you are not familiar with elementary code styling.

In the longer run I was envisaging this method being an instance method of CopyMoveJob and being a Vala async method perhaps but for an initial port its best to stick close to the original C code as you have done.

There are only a couple of (old) issues that mention MTP related to connecting mobile phones and I think these are more related to permissions than Files itself. Modern phones make direct USB connections difficullt anyway and most people make transfers another way so I don't think this is a priority.

I raised a couple of unrelated issues I noticed while testing this PR which I will address at some point.

If you point out the code style issues (hopefully not too many) I'm happy to address in this PR itself, is it the initial new method return value/signature being on separate line? Also, maybe the first few lines where I check do_sync then the is_move ? return... the way they are on separate lines? I assumed since the linter was passing I was good hah.

If you don't mind retesting I would like to fix/address the MTP issue as I mentioned in #2819 (comment) I think that might work.

Other than that, please point out any remaining issues in this PR that I can address to get it approved and merged... I'm really motivated to have this fix done!

Thanks!

vjr · 2026-09-04T11:44:30Z

Marking as draft while I push some more pending commits and address any review feedback...

This reverts commit a737e58. Was left over from attempt to fix MTP device not being detected as can_unplug.

jeremypw · 2026-09-04T12:18:13Z

@vjr fyi elementary code style info here https://docs.elementary.io/develop/writing-apps/code-style. I'll go through and point out any remaining issues later today. Note that old code (particularly C) does not necessary fully comply but new code should.

vjr · 2026-09-04T12:20:39Z

@jeremypw apologies for the repeated drafting/undrafting but could you take another look at the code and maybe redo a few of your tests, especially with your MTP device? thank you!

jeremypw · 2026-09-04T12:29:53Z

A convention I use (which I do not think is mentioned in the guide) is to use only one indent for the parameters of functions if there are too many to fit on one line and all parameters are indented the same. Closing bracket groups should be on the next line:

private void some_function (
    param1,
    param2,
    ....
) {
}

The same applies to function calls that do not fit within the 120 char line liimit except one more indent that the current line indent is used.

The scope and type of functions etc are always inline with the name.

In general I do not like using whitespace between lines that are logically related e.g. initializing parameters for the same clause.

But whitespace is put after clauses ending in } but not necessarily after statements ending in };

vjr · 2026-09-04T12:46:12Z

A convention I use (which I do not think is mentioned in the guide) is to use only one indent for the parameters of functions if there are too many to fit on one line and all parameters are indented the same. Closing bracket groups should be on the next line:
private void some_function (
    param1,
    param2,
    ....
) {
}
The same applies to function calls that do not fit within the 120 char line liimit except one more indent that the current line indent is used.

The scope and type of functions etc are always inline with the name.

In general I do not like using whitespace between lines that are logically related e.g. initializing parameters for the same clause.

But whitespace is put after clauses ending in } but not necessarily after statements ending in };

@jeremypw i've hopefully understood and properly applied your advice in cc54180 ?

jeremypw · 2026-09-04T16:16:11Z

+            }
+        }
+
+        var overwrite = (flags & FileCopyFlags.OVERWRITE) != 0;


This variable is not use 'til later so can be moved to before the clause in which it is used, and whitespace reduced to a single line.

jeremypw · 2026-09-04T16:17:37Z

+                    cancellable,
+                    progress_callback
+                );
+            } finally {


Shouldn't need finally here as we did not setup anything that needs to be undone at this point.

jeremypw · 2026-09-04T16:24:26Z

+
+        if (src_is_dir) {
+            if (dest_is_dir) {
+                error = overwrite ? IOError.WOULD_MERGE : IOError.EXISTS;


Can't we throw an error straightaway here and in similar other places in this clause? You can use the simpler form:

throw new IO.Error.EXISTS ("Put a suitable error message here if needed");

for example.

jeremypw · 2026-09-04T16:25:05Z

+            }
+        }
+
+        if (error >= 0) {


If errors are thrown straightaway this clauses would not be needed.

jeremypw · 2026-09-04T16:26:05Z

+        var dest_is_dir = FileUtils.file_is_dir (dest);
+        var dest_exists = dest.query_exists ();
+
+        var error = -1;


If errors are thrown straightaway this variable would not be needed here.

jeremypw · 2026-09-04T16:38:50Z

+            fd = out.get_fd ();
+        }
+
+        var success = false;


Whitespace between here and the related while clause can be removed

jeremypw · 2026-09-04T16:40:43Z

+        int64 last_sync_time = 0;
+
+        while (true) {
+            var read = in.read (buffer, cancellable);


This can throw and error and immediately return without closing streams. Do we need to put this in a try/catch/finally clause to ensure streams are closed?

jeremypw · 2026-09-04T16:44:44Z

+            var now = get_monotonic_time ();
+            if (last_sync_time == 0 || (now - last_sync_time).abs () >= SYNC_INTERVAL_MICROS) {
+                if (fd >= 0) {
+                    Posix.fsync (fd);


Have you considered using flush method of GLib.OutputStream instead of the Posix function. We would then not need fd. I am not sure whether they are equivalent but its worth a try.

jeremypw · 2026-09-04T16:48:58Z

+            }
+        }
+
+        if (fd >= 0) {


Closing the stream will flush its data.

Apparently streams are automatically closed when the last reference is dropped but it might be better to close them explicitly and handle any errors.

jeremypw

With an unpluggable MTP device, it is identified as unpluggable but fails to copy or move files onto it. This is because creating the output stream fails and the code does not handle that - it should fall back to a "normal" copy/move.

Sync transfers for realistic progress bar updates

4644373

vjr self-assigned this Aug 28, 2026

vjr requested review from a team and jeremypw August 28, 2026 07:09

jeremypw reviewed Aug 29, 2026

View reviewed changes

Merge branch 'main' into vishal/transfer-progress

dd7122c

vjr marked this pull request as draft August 30, 2026 06:31

vjr added 2 commits August 30, 2026 16:34

Remove unused consts

bb5b7f8

Don't use global posix sync

c8ee883

vjr added 2 commits August 30, 2026 21:56

Add a 'can_unplug ()' helper to file utils

a332b74

Add custom file copy and move method which does fsync

d063faa

Replace g_file_copy () and g_file_move () with custom copy_move_with_sync () method which conditionally calls fsync () periodically.

Handle overwrite case for file move

fa1351a

vjr marked this pull request as draft September 4, 2026 11:44

vjr added 5 commits September 4, 2026 17:19

Revert "Try to mark old MTP drives as can_unmount/can_eject"

b790e3b

This reverts commit a737e58. Was left over from attempt to fix MTP device not being detected as can_unplug.

Remove a remnant of initial C code ported to Vala

37d140f

Fix likely code style issues

8a339c1

Shorten util methods' names to can_unplug ()

409a6ff

Additional checks for can_unplug () to maybe fix for MTP devices

76666b3

vjr marked this pull request as ready for review September 4, 2026 12:19

Remove extra space left over

c553c64

vjr added 2 commits September 4, 2026 18:05

More code style fixes - pepper with var keyword

43a4e5c

Try more code style fixes and hopefully don't break linter

cc54180

jeremypw reviewed Sep 4, 2026

View reviewed changes

jeremypw requested changes Sep 4, 2026

View reviewed changes

Uh oh!

Uh oh!

Conversation

vjr commented Aug 28, 2026

Uh oh!

vjr commented Aug 28, 2026

Uh oh!

jeremypw commented Aug 28, 2026

Uh oh!

jeremypw commented Aug 28, 2026

Uh oh!

jeremypw commented Aug 28, 2026

Uh oh!

vjr commented Aug 28, 2026

Uh oh!

vjr commented Aug 28, 2026

Uh oh!

vjr commented Aug 28, 2026

Uh oh!

flodavid commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeremypw commented Aug 28, 2026

Uh oh!

vjr commented Aug 28, 2026

Uh oh!

jeremypw commented Aug 28, 2026

Uh oh!

vjr commented Aug 28, 2026

Uh oh!

jeremypw commented Aug 28, 2026

Uh oh!

jeremypw left a comment

Choose a reason for hiding this comment

Uh oh!

vjr commented Aug 30, 2026

Uh oh!

jeremypw commented Aug 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vjr commented Aug 30, 2026

Uh oh!

flodavid commented Aug 30, 2026

Uh oh!

jeremypw commented Aug 30, 2026

Uh oh!

jeremypw commented Sep 4, 2026

Uh oh!

jeremypw commented Sep 4, 2026

Uh oh!

vjr commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vjr commented Sep 4, 2026

Uh oh!

vjr commented Sep 4, 2026

Uh oh!

vjr commented Sep 4, 2026

Uh oh!

jeremypw commented Sep 4, 2026

Uh oh!

vjr commented Sep 4, 2026

Uh oh!

jeremypw commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vjr commented Sep 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flodavid commented Aug 28, 2026 •

edited

Loading

jeremypw commented Aug 30, 2026 •

edited

Loading

vjr commented Sep 4, 2026 •

edited

Loading

jeremypw commented Sep 4, 2026 •

edited

Loading