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

VFS Windows: Fix download of large files #2958

Merged
merged 1 commit into from
Mar 4, 2021

Conversation

FlexW
Copy link

@FlexW FlexW commented Mar 1, 2021

Fixes #2952

Signed-off-by: Felix Weilbach felix.weilbach@nextcloud.com

@FlexW FlexW force-pushed the bugfix/vfs-win-download-large-files branch from 3c53e68 to c91e8d9 Compare March 1, 2021 15:20
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
@allexzander
Copy link
Contributor

@FlexW Left my thoughts. Basically, the code is small enough to read, but, still, I'd try to simplify it if possible. We are risking of human-errors when working with arrays and offsets, so, the code must be as straightforward as possible in this case. I am going to go through everything one more time in the morning with my head fresh again :)

@FlexW FlexW force-pushed the bugfix/vfs-win-download-large-files branch 4 times, most recently from 82733d1 to 05f083b Compare March 2, 2021 12:13
src/libsync/vfs/cfapi/cfapiwrapper.cpp Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
@FlexW FlexW force-pushed the bugfix/vfs-win-download-large-files branch 2 times, most recently from 917f61a to 0bf54be Compare March 2, 2021 15:21
@FlexW FlexW force-pushed the bugfix/vfs-win-download-large-files branch 2 times, most recently from 2061952 to 9049458 Compare March 3, 2021 08:52
Copy link
Member

@er-vin er-vin left a comment

Choose a reason for hiding this comment

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

Ah! Now we're talking, it's much smaller and straightforward now. Much closer to what I envisioned initially. Not too expensive either.

src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/cfapiwrapper.cpp Outdated Show resolved Hide resolved
Copy link
Member

@er-vin er-vin left a comment

Choose a reason for hiding this comment

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

Oops, clicked the wrong one :-)

Almost there anyway

@allexzander
Copy link
Contributor

@FlexW Now, I like it too. I guess we just need those nitpicks from @er-vin addressed, and we are good to go! Thank you for being patient towards us :)

@allexzander
Copy link
Contributor

@FlexW The only other thing I advise is to not be in hurry with squashing everything. These changes are small, so, it's ok to read them all multiple times as you keep pushing review comments fixes, but, for bigger PRs it would be easier if we could see the review comment fixes progression while we can also just browse all changes if needed. And then, we squash/rebase them right before the merge :)

@github-actions github-actions bot force-pushed the bugfix/vfs-win-download-large-files branch from 9049458 to 86d96ed Compare March 3, 2021 19:30
@FlexW FlexW force-pushed the bugfix/vfs-win-download-large-files branch from 86d96ed to 6af52d5 Compare March 4, 2021 08:02
@FlexW
Copy link
Author

FlexW commented Mar 4, 2021

@FlexW The only other thing I advise is to not be in hurry with squashing everything. These changes are small, so, it's ok to read them all multiple times as you keep pushing review comments fixes, but, for bigger PRs it would be easier if we could see the review comment fixes progression while we can also just browse all changes if needed. And then, we squash/rebase them right before the merge :)

Okay, I keep that in mind for the next pr. I have now squashed the last nitpicks. Hope that is fine.

Signed-off-by: Felix Weilbach <felix.weilbach@nextcloud.com>
@FlexW FlexW force-pushed the bugfix/vfs-win-download-large-files branch from 6af52d5 to 523f1bc Compare March 4, 2021 08:05
@nextcloud-desktop-bot
Copy link

AppImage file: Nextcloud-PR-2958-523f1bcadd08e159278c920e5a1d728f44ea595e-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@allexzander
Copy link
Contributor

@FlexW Looks good. Now, just need @er-vin to lift his request so we can proceed :)

Copy link
Member

@er-vin er-vin left a comment

Choose a reason for hiding this comment

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

Woot! We got there now! Nice.

@FlexW FlexW merged commit 3796876 into master Mar 4, 2021
@FlexW FlexW deleted the bugfix/vfs-win-download-large-files branch March 4, 2021 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Daily Build 3.1.50.20210225 - Virtual Folders unable to load images in the Photo Folder
4 participants