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.

Sign up for GitHub

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
Merged

PDF renderer completion fixes #8937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2371feb
Fix problem of skipping over having visited 1st page
sairina Dec 9, 2021
6bb3895
Fix edge case of 2-page pdf
sairina Dec 10, 2021
259722c
Add updated progress to existing progress_delta
sairina Dec 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions kolibri/plugins/learn/assets/src/composables/__tests__/useProgressTracking.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,13 @@ describe('useProgressTracking composable', () => {
expect(get(progress_delta)).toEqual(0.1);
expect(client).not.toHaveBeenCalled();
});
it('should increment progress_delta if progress is updated twice', async () => {
const { updateContentSession, progress_delta } = await initStore();
await updateContentSession({ progress: 0.6 });
await updateContentSession({ progress: 0.7 });
expect(get(progress_delta)).toEqual(0.2);
expect(client).not.toHaveBeenCalled();
});
it('should update progress and store progress_delta if progress is updated over threshold', async () => {
const { updateContentSession, progress } = await initStore();
await updateContentSession({ progress: 1 });
Expand Down
6 changes: 4 additions & 2 deletions kolibri/plugins/learn/assets/src/composables/useProgressTracking.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export default function useProgressTracking(store) {
store = store || getCurrentInstance().proxy.$store;
const complete = ref(null);
const progress_state = ref(null);
const progress_delta = ref(null);
const progress_delta = ref(0);
const time_spent = ref(null);
const time_spent_delta = ref(null);
const session_id = ref(null);
Expand Down Expand Up @@ -350,7 +350,9 @@ export default function useProgressTracking(store) {
progress = _zeroToOne(progress);
progress = threeDecimalPlaceRoundup(progress);
if (get(progress_state) < progress) {
set(progress_delta, threeDecimalPlaceRoundup(progress - get(progress_state)));
const newProgressDelta =
get(progress_delta) + threeDecimalPlaceRoundup(progress - get(progress_state));
set(progress_delta, newProgressDelta);
set(progress_state, progress);
}
}
Expand Down
7 changes: 5 additions & 2 deletions kolibri/plugins/pdf_viewer/assets/src/views/PdfRendererIndex.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,8 @@
}
this.$emit('startTracking');
this.updateContentStateInterval = setInterval(this.updateProgress, 30000);
// Even if user does not pause while scrolling on first page, we store that as visited
this.storeVisitedPage(1);
});
},
beforeDestroy() {
Expand Down Expand Up @@ -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):

let currentPage =
this.totalPages === 2 ? 2 : parseInt(this.currentLocation * this.totalPages) + 1;
this.storeVisitedPage(currentPage);
this.updateProgress();
this.updateContentState();
Expand Down