Skip to content

Set resource total size to content size when using a qrc:// resource #1068

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

Merged
merged 1 commit into from
Mar 11, 2021

Conversation

daleglass
Copy link
Contributor

When a Resource is requested from qrc://, the Resource::handleDownloadProgress
event is never called, and therefore _bytesTotal remains at zero. This confuses
ResourceCache later, which has a sanity check that doesn't pass when resources
are retrieved, yet no data accumulates.

Fixes #1065

When a Resource is requested from qrc://, the Resource::handleDownloadProgress
event is never called, and therefore _bytesTotal remains at zero. This confuses
ResourceCache later, which has a sanity check that doesn't pass when resources
are retrieved, yet no data accumulates.

Fixes vircadia#1065
@@ -809,6 +807,14 @@ void Resource::handleReplyFinished() {
}

auto data = _request->getData();
if (_request->getUrl().scheme() == "qrc") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure if this check is necessary. It may be perfectly safe to do it always

_bytesTotal = data.length();
}

setSize(_bytesTotal);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note we're not calling setSize in the case of failure anymore.

I think this should be fine since on failure no data arrives, and this can cause confusion in ResourceCache.

@digisomni digisomni added this to the 2021.1.1 Eos Release milestone Feb 27, 2021
@ctrlaltdavid ctrlaltdavid added CR Approved At least one code reviewer has approved the PR. and removed needs CR (code review) labels Feb 28, 2021
@digisomni
Copy link
Member

What might be a good way to test this to confirm it works?

@digisomni digisomni removed the needs testing (QA) The PR is ready for testing label Mar 11, 2021
Copy link
Member

@digisomni digisomni left a comment

Choose a reason for hiding this comment

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

The tutorial and Hub worked fine with this PR.

@digisomni digisomni added the QA Approved The PR has been tested successfully. label Mar 11, 2021
@digisomni digisomni merged commit 5e2cc4c into vircadia:master Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix CR Approved At least one code reviewer has approved the PR. QA Approved The PR has been tested successfully.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libraries/networking/src/ResourceCache.cpp:515: void ResourceCache::updateTotalSize(const qint64&): Assertion `_totalResourcesSize >= 0' failed.
4 participants