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

File-upload: smooth time remaining, bitrate and stabilize user information #30147

Merged
merged 7 commits into from
Aug 16, 2023

Conversation

Phlogi
Copy link
Contributor

@Phlogi Phlogi commented Dec 7, 2021

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.

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>
@Phlogi Phlogi changed the title Smooth time remaining, bitrate and stabilize user information File-upload: smooth time remaining, bitrate and stabilize user information Dec 7, 2021
@szaimen szaimen added this to the Nextcloud 24 milestone Dec 8, 2021
@szaimen szaimen added 3. to review Waiting for reviews enhancement labels Dec 8, 2021
Copy link
Member

@CarlSchwan CarlSchwan left a 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 :)

apps/files/js/file-upload.js Outdated Show resolved Hide resolved
apps/files/js/file-upload.js Outdated Show resolved Hide resolved
apps/files/js/file-upload.js Outdated Show resolved Hide resolved
apps/files/js/file-upload.js Outdated Show resolved Hide resolved
apps/files/js/file-upload.js Outdated Show resolved Hide resolved
apps/files/js/file-upload.js Show resolved Hide resolved
Phlogi and others added 5 commits December 8, 2021 18:57
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>
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
@blizzz blizzz mentioned this pull request Mar 31, 2022
@blizzz blizzz mentioned this pull request Apr 7, 2022
@szaimen szaimen requested review from a team, skjnldsv, vanpertsch and szaimen and removed request for a team April 7, 2022 15:04
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

I just tested this and somehow the speed does not seem to work correctly:
image

Copy link
Contributor

@artonge artonge left a 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);
Copy link
Contributor

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.

Copy link
Contributor

@artonge artonge Apr 12, 2022

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

Copy link
Contributor Author

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.

Copy link
Contributor

@artonge artonge Apr 19, 2022

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?

@skjnldsv skjnldsv modified the milestones: Nextcloud 24, Nextcloud 25 Apr 12, 2022
@Phlogi
Copy link
Contributor Author

Phlogi commented Apr 18, 2022

Also, canceling an upload seems to fail now

I can reproduce this, however I don't think it's related to the change here. I see a 404 in the frontend:
image

Here is the message I see in the debug log on the server side:

{
  "reqId": "QCf3mPDlqWTxKByNmFtN",
  "level": 3,
  "time": "2022-04-18T19:24:22+00:00",
  "remoteAddr": "192.168.1.244",
  "user": "webadmin",
  "app": "core",
  "method": "PUT",
  "url": "/remote.php/dav/uploads/webadmin/web-file-upload-5368d4c4d0bf8c192c21c77fb3e39e95-1650309856915/0",
  "message": "unable to rename, source directory is not writable : uploads/web-file-upload-5368d4c4d0bf8c192c21c77fb3e39e95-1650309856915",
  "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:99.0) Gecko/20100101 Firefox/99.0",
  "version": "22.2.3.0"
}
{
  "reqId": "QCf3mPDlqWTxKByNmFtN",
  "level": 3,
  "time": "2022-04-18T19:24:22+00:00",
  "remoteAddr": "192.168.1.244",
  "user": "webadmin",
  "app": "webdav",
  "method": "PUT",
  "url": "/remote.php/dav/uploads/webadmin/web-file-upload-5368d4c4d0bf8c192c21c77fb3e39e95-1650309856915/0",
  "message": "renaming part file to final file failed $renameOkay: false, $fileExists: false)",
  "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:99.0) Gecko/20100101 Firefox/99.0",
  "version": "22.2.3.0"
}
{
  "reqId": "QCf3mPDlqWTxKByNmFtN",
  "level": 4,
  "time": "2022-04-18T19:24:22+00:00",
  "remoteAddr": "192.168.1.244",
  "user": "webadmin",
  "app": "webdav",
  "method": "PUT",
  "url": "/remote.php/dav/uploads/webadmin/web-file-upload-5368d4c4d0bf8c192c21c77fb3e39e95-1650309856915/0",
  "message": "Could not rename part file to final file",
  "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:99.0) Gecko/20100101 Firefox/99.0",
  "version": "22.2.3.0",
  "exception": {
    "Exception": "Sabre\\DAV\\Exception",
    "Message": "Could not rename part file to final file",
    "Code": 0,
    "Trace": [
      {
        "file": "/var/www/nextcloud/apps/dav/lib/Connector/Sabre/Directory.php",
        "line": 155,
        "function": "put",
        "class": "OCA\\DAV\\Connector\\Sabre\\File",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/apps/dav/lib/Upload/UploadFolder.php",
        "line": 45,
        "function": "createFile",
        "class": "OCA\\DAV\\Connector\\Sabre\\Directory",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/3rdparty/sabre/dav/lib/DAV/Server.php",
        "line": 1098,
        "function": "createFile",
        "class": "OCA\\DAV\\Upload\\UploadFolder",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/3rdparty/sabre/dav/lib/DAV/CorePlugin.php",
        "line": 504,
        "function": "createFile",
        "class": "Sabre\\DAV\\Server",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/3rdparty/sabre/event/lib/WildcardEmitterTrait.php",
        "line": 89,
        "function": "httpPut",
        "class": "Sabre\\DAV\\CorePlugin",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/3rdparty/sabre/dav/lib/DAV/Server.php",
        "line": 472,
        "function": "emit",
        "class": "Sabre\\DAV\\Server",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/3rdparty/sabre/dav/lib/DAV/Server.php",
        "line": 253,
        "function": "invokeMethod",
        "class": "Sabre\\DAV\\Server",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/3rdparty/sabre/dav/lib/DAV/Server.php",
        "line": 321,
        "function": "start",
        "class": "Sabre\\DAV\\Server",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/apps/dav/lib/Server.php",
        "line": 333,
        "function": "exec",
        "class": "Sabre\\DAV\\Server",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/apps/dav/appinfo/v2/remote.php",
        "line": 35,
        "function": "exec",
        "class": "OCA\\DAV\\Server",
        "type": "->"
      },
      {
        "file": "/var/www/nextcloud/remote.php",
        "line": 166,
        "args": [
          "/var/www/nextcloud/apps/dav/appinfo/v2/remote.php"
        ],
        "function": "require_once"
      }
    ],
    "File": "/var/www/nextcloud/apps/dav/lib/Connector/Sabre/File.php",
    "Line": 295,
    "CustomMessage": "--"
  }
}

@szaimen
Copy link
Contributor

szaimen commented Apr 19, 2022

Also, canceling an upload seems to fail now

@artonge this is unrelated, afaik.
I think it is failing in master and stable24 too...

@skjnldsv skjnldsv removed their request for review April 26, 2022 10:36
This was referenced Aug 12, 2022
@blizzz blizzz mentioned this pull request Aug 24, 2022
This was referenced Aug 30, 2022
@blizzz blizzz mentioned this pull request Sep 9, 2022
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
This was referenced Sep 20, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
This was referenced May 3, 2023
@skjnldsv skjnldsv added this to In progress in Files to vue via automation May 10, 2023
@skjnldsv skjnldsv moved this from In progress to Obsolete in Files to vue May 10, 2023
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels May 10, 2023
@skjnldsv skjnldsv removed this from the Nextcloud 27 milestone May 10, 2023
@skjnldsv skjnldsv marked this pull request as draft May 10, 2023 09:52
@susnux susnux changed the base branch from master to fix/improved-upload-view August 16, 2023 13:01
@susnux susnux marked this pull request as ready for review August 16, 2023 13:02
@susnux susnux merged commit 91c873b into nextcloud:fix/improved-upload-view Aug 16, 2023
2 checks passed
Files to vue automation moved this from Obsolete to Done Aug 16, 2023
@welcome
Copy link

welcome bot commented Aug 16, 2023

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

@szaimen
Copy link
Contributor

szaimen commented Aug 16, 2023

Interesting approach to merge it into a different branch in our org :)

@susnux
Copy link
Contributor

susnux commented Aug 16, 2023

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Files to vue
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants