search

sugarlabs / browse-activity

Sugar activity to browse the internet; WebKit on GTK on Sugar Toolkit
GNU General Public License v2.0
8 stars 44 forks source link

Ensure that a PDF is only downloaded once #102

Closed nswarup14 closed 5 years ago

nswarup14 commented 5 years ago

Fixes #54

Changes 1) browser.py 2) pdfviewer.py

Updates At the present master HEAD (https://github.com/sugarlabs/browse-activity/commit/540c93fa5fe4ca1a741c6460abed2607aa8a8078), if the mime_type, in __decide_policy_cb is application/pdf, a new GET request was being issued after setting up the PDF tab in pdfviewer.py/_download_from_http, where the context.download_uri(remote_uri) was the culprit.

Rather than make a new GET request, I used the response obtained from the first GET request to download the PDF directly, using the WebKit2.PolicyDecision.download(). The function also emits the download-started signal which is connected to pdfviewer.py/__download_started_cb, which handles the rest.

I found a thread in the WebKit2 mailing list (here), that seems to explain the issue in more detail.

@quozl @pro-panda Please let me know if I have approached this issue in the right step.

Thanks.

Tested on Ubuntu 18.04, Sugar v0.114

quozl commented 5 years ago

I've reviewed the patch. I can see the removal of download_uri, and the change from ignore to download, which makes sense. But I don't understand what the other changes are for, and the commit message didn't explain. (Please make sure the explanation and justification for changes is in the commit message, because that's what will end up in the git log, which is used by other developers through git bisect or git blame long after GitHub pull requests may have been archived.) I did re-read your pull request opening comment, but still didn't understand what the other changes are for.

nswarup14 commented 5 years ago

@quozl Thanks I will update the commit message as well.

I refactored the code which saved the metadata of the browse-activity in a journal instance. The state of a regular tab was a bytes object, which needs to be decoded and the state of a PDF tab was a regular dictionary. Hence I was handled both the cases in separate if-else cases.

I realize the other changes are not required after reading the documentation once again. I shall remove them immediately.