GNOME Bugzilla – Bug 760709
Add progress bars to XFS and EXT2/3/4 file system specific copy methods
Last modified: 2016-04-26 15:55:06 UTC
I've turned a mole hill into a bit of a mountain. I started out looking at adding progress bars to XFS and EXT2/3/4 file system specific copying methods and ended up creating a new progress bar class and using everywhere. I'm working of the patchset for this, Mike Quoting one of the commit messages ---------------------------------- Add a single ProgressBar for all OperationDetail objects (#XXXXXX) 1) Multiple progress bars The OperationDetail class contains member fraction which is used to feed data to the current operation progress bar shown in the Applying pending operations dialog. Dialog_Progress::on_signal_update() gets called for every updated OperationDetail object and depending on whether fraction is > 0.0 or not, switches between showing a growing or pulsing progress bar. This leads to the conclusion that every OperationDetail object currently being updated is effectively driving the single on screen progress bar with different data. The Copy_Blocks code is careful to update text and faction in a single OperationDetail object and everything is good. The on screen progress bar is switched into growing mode and then grows to 100%. Since external command output is updated in real time [1] there are two OperationDetail objects, one for stdout and one for stderr, which are updated whenever data is read from the relevant stream. Also now that progress is interpreted from some external command output [2][3][4] a separate OperationDetail object is getting updated with the progress fraction. (Actually the grandparent OperationDetail of the ones receiving stdout and stderr updates as used by the file system specific *_progress() methods). In the normal case of an external command which is reporting it's progress two OperationDetails are constantly being updated together, the OperationDetail object tracking stdout and it's grandparent receiving progress fraction updates. This causes the the code in Dialog_Progress::on_signal_update() to constantly switch between growning and pulsing progress bar mode. The only reason this doesn't flash the progress bar is because the stdout OperationDetail object is updated first and before the 100 ms timeout fires to pulse the bar, it's grandparent is updated with the new fraction to keep growing the bar instead. 2) Common code The Copy_Blocks code currently tracks the progress of blocks copied against target amount, which it has to do anyway. That information is then used to generate the text and fraction to update into the OperationDetail object and drive the on screen progress bar. This same level of tracking is wanted for the XFS and EXT2/3/4 file system specific copy methods. Conclusion and solution Having multiple sources of progress bar data is a problem and makes it clear that there must be only one source of progress data. Also some code can be shared for tracking the amount of blocks copied and generating the display. Therefore have a single ProgressBar object which is used everywhere. This change This change just creates a single ProgressBar object which is available via all OperationDetail objects. The ProgressBar class still contains debugging. Copy_Blocks is updated to use the single ProgressBar object via any OperationDetail object it has access to. Referenced commits: [1] 52a2a9b00a32996921ace055e71d0e09fb33c5fe Reduce threading (#685740) [2] ae434579e160ca2c306921b12d4211bf9abacef7 Display progress for e2fsck (#467925) [3] baea186138cc08cac1f13f28300ca0f4b09ae16d Display progress for mke2fs (#467925) [4] 57b028bb8e7c9fa5e750624f405cc268dfa3b4af Display progress during resize (#467925) Debugging details ----------------- Debugging was added into Dialog_Progress::on_signal_update(). It reported the OperationDetail objects being signalled for update and what the description was set to and the intention of the fraction value. (fraction == 0 => pulse bar; fraction > 0 => progress bar amount). Debugging of the internal block copy looks like this: TreePath Description Progress 0:4:3:0 <i>48.00 MiB of 4.92 GiB copied</i> progress bar 0.009 0:4:3:0 <i>98.00 MiB of 4.92 GiB copied</i> progress bar 0.019 0:4:3:0 <i>148.00 MiB of 4.92 GiB copied</i> progress bar 0.029 0:4:3:0 <i>198.00 MiB of 4.92 GiB copied</i> progress bar 0.039 0:4:3:0 <i>250.00 MiB of 4.92 GiB copied</i> progress bar 0.049 0:4:3:0 <i>300.00 MiB of 4.92 GiB copied</i> progress bar 0.059 Debugging of an ext2/3/4 file system check looks like this: TreePath Description Progress 0 <b>Check and repair file system (ext... pulse bar 0:1 check file system on /dev/sdd1 for e... progress bar 0.001 0:1:0:0 <i>Pass 1: Checking inodes, blocks, ... pulse bar 0:1 check file system on /dev/sdd1 for e... progress bar 0.071 0:1:0:0 <i>Pass 1: Checking inodes, blocks, ... pulse bar 0:1 check file system on /dev/sdd1 for e... progress bar 0.113 0:1:0:0 <i>Pass 1: Checking inodes, blocks, ... pulse bar 0:1 check file system on /dev/sdd1 for e... progress bar 0.211 0:1:0:0 <i>Pass 1: Checking inodes, blocks, ... pulse bar 0:1 check file system on /dev/sdd1 for e... progress bar 0.246 This shows the OperationDetail object containing the fsck output being updated (TreePath "0:1:0:0") with no bar progress (fraction set to 0 so "pulse bar"), followed by the grandparent being updated with the progress bar (fraction set to > 0).
Created attachment 320555 [details] [review] XFS and ext2/3/4 copy progress bars (Draft 1) Hi Curtis, I'm currently a bit stuck with this code. The current last commit which is trying to simplify the use of timed progress tracking callbacks doesn't compile. I have posted a question to the libsigc++ email list to see if anyone can help. If not I will have to use separately named execute_command() methods to work around. Subject: [sigc] Functor not resolving between overloaded methods with different slot types https://mail.gnome.org/archives/libsigc-list/2016-February/msg00000.html Thanks, Mike
Hi Mike, From looking through the email thread listed in comment #1, it appears that you have a work around. If this is not the case then let me know and I can do some digging. Curtis
Hi Curtis, Yes I have good solution to the problem. Initially I wasn't confident as the volume of mails on the libsigc++ list has been very low over the last few years, with more months than not having no emails. Anyway it's all turned out well. Just working on what I hope will be the last patch before doing the final comment review and stripping of debugging code. Should then be ready for submission. Thanks, Mike
Created attachment 320798 [details] [review] XFS and ext2/3/4 copy progress bars (v1) Hi Curtis, Here's patchset v1. Thanks, Mike
Hi Mike, Thanks for the detailed patch set containing explanations and reference links. This helped me with the code review. The new single progress bar class you implemented helps to make the code cleaner too. I plan to test all file system operations on one distro, and then fewer tests on other distros. Following are my suggestions for minor code/git comment changes. [01/22] Write a generic progress bar class (#760709) - no change. [02/22] Add a single ProgressBar for all OperationDetail objects (#760709) - Sentence fragment in commit comment should be removed. This change This change just creates... [03/22] Display progress from the single ProgressBar in the GUI (#760709) - no change. [04/22] Update ext2 resize progress tracker to use the new ProgressBar (#760709) - Potential issue with using text string to determine when to stop progress tracker. // Ending summary line looks like: // "The filesystem on /dev/sdb3 is now 256000 block long." else if ( output.find( " is now " ) != output.npos ) ^^^^^^^^ Rhetorical Question: Is there a better way to minimize the impact if this text string changes? Self Answer: Currently I do not have a better idea. Further we use text strings in other portions of this code as well. [05/22] Update ext2 create progress tracker to use the new ProgressBar (#760709) - Same potential issue as 04/22. [06/22] Update ext2 fsck progress tracker to use the new ProgressBar (#760709) - Minor text change in commit comment "used" s/b "use". Adapt the ext2 fsck progress tracker to used the new ProgressBar class. ^^^^ use [07/22] Update ntfs resize progress tracker to use the new ProgressBar (#760709) - no change. [08/22] Fix ntfs resize progress tracker matching spurious text (#760709) - no change. [09/22] Fix rounding of negative numbers (#760709) - no change. [10/22] Fix formatting of negative time values (#760709) - no change. [11/22] Display progress of XFS file system specific copy operation (#760709) - no change. [12/22] Record file system block size where known (#760709) - no change. [13/22] Call any FS specific progress trackers for stderr updates too (#760709) - Minor grammatical error in commit comment. stdout. However e2image writes it progress information to stderr, ^^^ its [14/22] Display progress of ext2/3/4 file system specific copy and move operations (#760709) - no change. [15/22] Simplify use of the progress bar via OperationDetail (#760709) - Minor (really minor) grammatical error in commit comment. still access the single ProgressBar object for it's querying needs. ^^^^ its [1] [1] http://grammarist.com/spelling/its-its/ [16/22] Remove unused OperationDetail members (#760709) - no change. [17/22] Connect output progress tracking callbacks inside execute_command() (#760709) - no change. [18/22] Remove the unnecessary signal_progress (#760709) - no change. [19/22] Connect timed progress tracking callbacks inside execute_command() (#760709) - no change. [20/22] Simplify XFS copy specific code progress bar usage (#760709) - no change. [21/22] Leave commands with progress bars shown in the applying dialog (#760709) - no change. [22/22] Always be explicit when emitting a signal by calling emit() - no change. Curtis
Created attachment 320935 [details] [review] XFS and ext2/3/4 copy progress bars (v2) Hi Curtis, [02/22] "This change". It's trying to be a section header like the others: 1) Multiple progress bars 2) Common code Conclusion and solution This change Referenced commits: I have reworded slightly avoid repetition to try to make it clearer. [04/22] [05/22] [No changes to these commits, just discussion] Note that it is not critical for the progress trackers to detect the end of the progress information in the command output. It is just a nice to have to accurately track the end of the reported progress from the command. Several of the tracked commands report 100% progress then take several more seconds before completing. When everything works the bar will show progress up to 100% and then detect the completed indication and cause the bar to switch back to pulsing mode until the command completes. Should the tracker miss the completion indication the bar will merely remain at 100% for those several seconds at the end, until the command completes. This is because there is a catchall call to stop_progressbar() right at the end of execute_command(). [06/22] [13/22] [15/22] Grammar corrected. Here's patchset v2 making the above described changes. Also note that I have tested this on the following distros: CentOS 5, CentOS 6, CentOS 7 Fedora 23 Thanks, Mike
Hi Mike, My testing of this enhancement has gone very well. It works as expected and I have not discovered any regressions in expected behaviour. :-) All file system features tested using Ubuntu 15.10. File System All Supported FS Features Still Work? ----------- ------------------------------------- btrfs Yes - New UUID not tested - older btrfs-tools 4.0-2 [1] ext2 Yes ext3 Yes ext4 Yes fat16 Yes fat32 Yes hfs Yes hfs+ Yes jfs Yes linux-swap Yes lvm2-pv Yes nilfs2 Yes ntfs Yes reiser4 Yes reiserfs Yes xfs Yes [1] Test btrfs new-uuid successful under Fedora 23. A subset of file system features was successfully tested on the following distros: debian 7 debian 8 kubuntu 12.04 openSUSE 13.2 Based on the testing results, patch set v2 in comment #6 looks good to me. If there are no objections then I will commit patch set v2 to the git repository tomorrow. Curtis
Thanks Mike for the great work on this enhancement. Patch set v2 from comment #6 has been committed to the git repository for inclusion in the next release of GParted. The relevant git commits can be viewed at the following links: Write a generic progress bar class (#760709) https://git.gnome.org/browse/gparted/commit/?id=0ca8ed73696eaf12d6c32828e0d746e7d0e3b442 Add a single ProgressBar for all OperationDetail objects (#760709) https://git.gnome.org/browse/gparted/commit/?id=c3669c3a9603d65ee771cf13df75874416944464 Display progress from the single ProgressBar in the GUI (#760709) https://git.gnome.org/browse/gparted/commit/?id=b0d9d2de7e785493167db26ed131062f51f09e99 Update ext2 resize progress tracker to use the new ProgressBar (#760709) https://git.gnome.org/browse/gparted/commit/?id=608060f82dae66cca0a8be2590bdd12ddcdf8be7 Update ext2 create progress tracker to use the new ProgressBar (#760709) https://git.gnome.org/browse/gparted/commit/?id=ac949e30030a4a9461594856beaed1a5932ff508 Update ext2 fsck progress tracker to use the new ProgressBar (#760709) https://git.gnome.org/browse/gparted/commit/?id=97f836869f29c6748172af2ee9a22b8d34668899 Update ntfs resize progress tracker to use the new ProgressBar (#760709) https://git.gnome.org/browse/gparted/commit/?id=9f7a38e6b3498a66a465a691fb99a577edb05814 Fix ntfs resize progress tracker matching spurious text (#760709) https://git.gnome.org/browse/gparted/commit/?id=af0ed90d497d045482f480108ce3ef42cda47359 Fix rounding of negative numbers (#760709) https://git.gnome.org/browse/gparted/commit/?id=7049a8bc449f0347ed6fb3fbfe80cd1df6f9ed2a Fix formatting of negative time values (#760709) https://git.gnome.org/browse/gparted/commit/?id=b0bd4650982f5c804eae24eb59ec8ac015be5ba1 Display progress of XFS file system specific copy operation (#760709) https://git.gnome.org/browse/gparted/commit/?id=809a7e095444f881256871d915b460af5cb4bab2 Record file system block size where known (#760709) https://git.gnome.org/browse/gparted/commit/?id=324d99a172848e4ff3fb7eb189f490bb4e6c53e5 Call any FS specific progress trackers for stderr updates too (#760709) https://git.gnome.org/browse/gparted/commit/?id=965d88d197c4c932bcd5cf4654e4cd44ca997377 Display progress of ext2/3/4 file system specific copy and move operations (#760709) https://git.gnome.org/browse/gparted/commit/?id=32622f4d579998537584b513ea98ea446815f26b Simplify use of the progress bar via OperationDetail (#760709) https://git.gnome.org/browse/gparted/commit/?id=b1313281bdaa40a7afc19687a14ac96c919f333c Remove unused OperationDetail members (#760709) https://git.gnome.org/browse/gparted/commit/?id=27e30a570ff50509d710ee8dccdad848f01c0e4d Connect output progress tracking callbacks inside execute_command() (#760709) https://git.gnome.org/browse/gparted/commit/?id=c00927c23d7b569078473734a3685b5d449d0f70 Remove the unnecessary signal_progress (#760709) https://git.gnome.org/browse/gparted/commit/?id=e67bbe906ff761396968a93a57cd5de716fea5c6 Connect timed progress tracking callbacks inside execute_command() (#760709) https://git.gnome.org/browse/gparted/commit/?id=438b35aed9aabc9f0e634831764df2b632129f69 Simplify XFS copy specific code progress bar usage (#760709) https://git.gnome.org/browse/gparted/commit/?id=075154b4e896576d0098256e481619efac9e95a9 Leave commands with progress bars shown in the applying dialog (#760709) https://git.gnome.org/browse/gparted/commit/?id=2a55c65876ec602f0718f4f81156833e82ff909b Always be explicit when emitting a signal by calling emit() https://git.gnome.org/browse/gparted/commit/?id=822028b504f0b85830bb15a0762def49cc30674f
This enhancement was included in the GParted 0.26.0 release on April 26, 2016.