-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
File-upload: smooth time remaining, bitrate and stabilize user information #30147
Conversation
Besides the existing moving average, a smoothing factor is introduced for the time remaining display as well as the bitrate. Furthermore, half of the buffer needs to be filled before the first prediction is displayed to the user. This reduces volatile and jumping durations towards the user and improves usability.
Signed-off-by: Cyrill H. <phlogi@posteo.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool feature, I left some code style but I didn't test it yet locally. I will give it a try tomorrow :)
Code Style Co-authored-by: Carl Schwan <carl@carlschwan.eu>
Code Style Co-authored-by: Carl Schwan <carl@carlschwan.eu>
Code Style Co-authored-by: Carl Schwan <carl@carlschwan.eu>
Code Style Co-authored-by: Carl Schwan <carl@carlschwan.eu>
Code Style Co-authored-by: Carl Schwan <carl@carlschwan.eu>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, canceling an upload seems to fail now
} else { | ||
smoothRemainingSeconds = 1; | ||
} else{ | ||
smoothRemainingSeconds = smoothing * (bufferTotal / bufferSize) + ((1-smoothing) * smoothRemainingSeconds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smoothRemainingSeconds
value depends on itself, this leads to NaN
when I test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe update the above condition to smoothRemainingSeconds === undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should check for undefined.
if (smoothRemainingSeconds === undefined){
Same for the bitrate:
if (smoothBitrate === undefined){
I just tested this and the NaN
problem is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you push the change?
@artonge this is unrelated, afaik. |
Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 |
Interesting approach to merge it into a different branch in our org :) |
The bots do not work on foreign repositories, so I decided to merge this into an internal branch and resubmit the PR. BTW: thank you @Phlogi, nice work :) |
Improving the upload progress bar's "Time remaining".
Tested with a single large file and multiple smaller files.
Besides the existing moving average, a smoothing factor is introduced for the time remaining display as well as the bitrate.
Furthermore, half of the buffer needs to be filled before the first prediction is displayed to the user. This reduces volatile and jumping durations towards the user and improves usability.