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

PDF renderer completion fixes #8937

Merged
merged 3 commits into from Dec 16, 2021

Conversation

sairina
Copy link
Contributor

@sairina sairina commented Dec 16, 2021

Summary

  • Fix edge case of 2-page pdfs
  • Add visit to 1st pdf page for all pdfs
  • Fix progress tracking from PDF to content card

2021-12-15 16 19 06

References

Fixes #8868

Reviewer guidance

  1. Sign in as learner
  2. Go to the library and choose the 2-pg PDF in the QA channel
  3. Read through the PDF; check to see that it gets completed and that the completed icon shows up in the app bar
  4. Go back to the library and check to see that the progress on that content card is now complete
  5. Repeat for PDFs of other lengths, but also periodically check to see that the progress on the content card shows accurate progress while you're reading through the PDF

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@sairina sairina added the TODO: needs review Waiting for review label Dec 16, 2021
@@ -323,8 +325,9 @@
// TODO: there is a miscalculation that causes a wrong position change on scale
this.savePosition(this.calculatePosition());

// determine how many pages user has viewed/visited
let currentPage = parseInt(this.currentLocation * this.totalPages) + 1;
// determine how many pages user has viewed/visited; fix edge case of 2 pages
Copy link
Member

Choose a reason for hiding this comment

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

What was the edge case here for 2 pages? Seems like both 1 and 2 are edge cases, which seems odd.

Copy link
Member

Choose a reason for hiding this comment

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

My thought here is that the subsequent progress_delta fix might also fix this issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes, it would make sense that it was both 1 and 2, but I didn't have a PDF that was only 1 page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, no, that doesn't seem to fix it. When there are two pages in the pdf, currentPage seems to be constantly set to 1 because parseInt(this.currentLocation * this.totalPages) always ends up at 0 because this.currentLocation doesn't get high enough.

Copy link
Member

Choose a reason for hiding this comment

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

Hrm - my I am curious why that is specific to 2 page PDFs, and why on a 3 page PDF '2' does become a currentPage at some point, makes me think there might be something slightly off about our currentLocation calculation.

Copy link
Member

Choose a reason for hiding this comment

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

(this is just curiosity, not a blocker)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello sir @rtibbles, I spent my time learning about the rendering of PDFs in Kolibri. I tried to understand the files LearnImmersivelayout, LearningActivityBar, ProgressIcon etc. If I am not wrong the PDFs are rendered here on a canvas. I have a doubt, about how can we track the current page from the rendered canvas? , I think if we get to know which page the user is at then we can fix the ProgressIcon not updating bug.
(I will try to research more about PDF.js to understand this problem more)

Copy link
Member

Choose a reason for hiding this comment

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

We don't render every page at once (as this causes performance issues for larger PDFs) so we know which pages are currently rendered, and control that based on where the user is.

The component you linked to controls the rendering of a single page - it is then used inside a RecycleList component that handles only actually rendering the pages that are visible by the user at the current time: https://github.com/learningequality/kolibri/blob/develop/kolibri/plugins/pdf_viewer/assets/src/views/PdfRendererIndex.vue#L47

It's in ^ this component that the progress tracking logic is happening, this is where the fix would almost certainly need to be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @rtibbles and @sairina, thanks for all your help I found a solution but I am not sure it is correct.
Let me explain what I came up with...
In the <progressIcon> we have a prop called "progress".
The value of "progress" determines the state of <progressIcon>
Screenshot from 2022-03-13 12-47-40
Screenshot from 2022-03-13 13-02-54
In React we use a hook useEffect which lets you perform side effects in function components (I am new to vue.js so I don't know what features we have in vue.js, we can use something like that).

My Idea is when we scroll we can keep on updating the "progress" value maybe from the PdfRendererIndex.vue (not sure from where) something like:

console output:
> 0.638
> 0.639
> 0.640
> 0.641
> 0.642
(this output is a simple console log of an onScroll event, all I wanted to show is my idea)

but I don't know how we can access the value of "this.progress" when we scroll

In the end when the "progress" value is equal to 1. we can change the isComplete value to True and isInProgress value to False (I think, this part already exists in the code).
Thank you for reading this, please tell me if wrong, I will search for another way to fix it. Or maybe it's just a small bug that I need to find and don't need all the stuff I mentioned above (sorry 😛)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh, I never even considered looking at ProgressIcon! @rtibbles would probably know more about whether this is the right way to go in terms of efficiency, but if you want to explore it a bit more (if you haven't already):

@rtibbles rtibbles merged commit 16614a8 into learningequality:release-v0.15.x Dec 16, 2021
@sairina sairina mentioned this pull request Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PDF resource completion
3 participants