-
-
Notifications
You must be signed in to change notification settings - Fork 7
feat(PdfReader): add width observer function #743
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,33 +20,10 @@ export async function init(id, invoke, options) { | |||||||||||||||||||||
| return; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const loadingTask = pdfjsLib.getDocument(options); | ||||||||||||||||||||||
| loadingTask.onProgress = function (progressData) { | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| loadingTask.onPassword = function (updatePassword, reason) { | ||||||||||||||||||||||
| if (reason === pdfjsLib.PasswordResponses.NEED_PASSWORD) { | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
| else if (reason === pdfjsLib.PasswordResponses.INCORRECT_PASSWORD) { | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const container = el.querySelector(".bb-view-container"); | ||||||||||||||||||||||
| const eventBus = new pdfjsViewer.EventBus(); | ||||||||||||||||||||||
| const pdfViewer = new pdfjsViewer.PDFViewer({ | ||||||||||||||||||||||
| container, | ||||||||||||||||||||||
| eventBus | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| addEventListener(el, pdfViewer, eventBus, invoke, options); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const pdfDocument = await loadingTask.promise; | ||||||||||||||||||||||
| pdfViewer.setDocument(pdfDocument); | ||||||||||||||||||||||
| const pdfViewer = await loadPdf(el, invoke, options); | ||||||||||||||||||||||
| const observer = setObserver(el); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Data.set(id, { el, pdfViewer }); | ||||||||||||||||||||||
| Data.set(id, { el, pdfViewer, observer }); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export function setScaleValue(id, value) { | ||||||||||||||||||||||
|
|
@@ -86,6 +63,107 @@ export function resetThumbnails(id) { | |||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const loadPdf = async (el, invoke, options) => { | ||||||||||||||||||||||
| const loadingTask = pdfjsLib.getDocument(options); | ||||||||||||||||||||||
| loadingTask.onProgress = function (progressData) { | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| loadingTask.onPassword = function (updatePassword, reason) { | ||||||||||||||||||||||
| if (reason === pdfjsLib.PasswordResponses.NEED_PASSWORD) { | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
| else if (reason === pdfjsLib.PasswordResponses.INCORRECT_PASSWORD) { | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const container = el.querySelector(".bb-view-container"); | ||||||||||||||||||||||
| const eventBus = new pdfjsViewer.EventBus(); | ||||||||||||||||||||||
| const pdfViewer = new pdfjsViewer.PDFViewer({ | ||||||||||||||||||||||
| container, | ||||||||||||||||||||||
| eventBus | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| addEventListener(el, pdfViewer, eventBus, invoke, options); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const pdfDocument = await loadingTask.promise; | ||||||||||||||||||||||
| pdfViewer.setDocument(pdfDocument); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return pdfViewer; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const setObserver = el => { | ||||||||||||||||||||||
| const title = el.querySelector(".bb-view-title"); | ||||||||||||||||||||||
| const subject = el.querySelector(".bb-view-subject"); | ||||||||||||||||||||||
| const groupPage = el.querySelector(".bb-view-group-page"); | ||||||||||||||||||||||
| const groupScale = el.querySelector(".bb-view-group-scale"); | ||||||||||||||||||||||
| const groupRotate = el.querySelector(".bb-view-group-rotate"); | ||||||||||||||||||||||
| const controls = el.querySelector(".bb-view-controls"); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
||||||||||||||||||||||
| // Null checks for all toolbar elements | |
| if (!title || !subject || !groupPage || !groupScale || !groupRotate || !controls) { | |
| return; | |
| } |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The width calculation is missing subject.offsetWidth. When checking if controls can be unhidden, the formula should include all elements: title.offsetWidth + subject.offsetWidth + groupRotate.offsetWidth + groupScale.offsetWidth + groupPage.offsetWidth + el.widths[4] < toolbar.offsetWidth. The current calculation omits subject, which could cause controls to be shown even when there isn't actually enough space.
| if (title.offsetWidth + el.widths[4] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { | |
| if (title.offsetWidth + el.widths[0] + el.widths[4] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The width calculation is missing subject.offsetWidth and incorrectly includes the hidden element's current width. When checking if groupPage can be unhidden, the formula should be: title.offsetWidth + subject.offsetWidth + groupRotate.offsetWidth + groupScale.offsetWidth + el.widths[3] + controls.offsetWidth < toolbar.offsetWidth. The current calculation omits subject and uses groupPage.offsetWidth (which is 0 when hidden) instead of just using el.widths[3].
| if (title.offsetWidth + el.widths[3] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { | |
| if (title.offsetWidth + subject.offsetWidth + groupRotate.offsetWidth + groupScale.offsetWidth + el.widths[3] + controls.offsetWidth < toolbar.offsetWidth) { |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The width calculation is missing subject.offsetWidth and incorrectly includes the hidden element's current width. When checking if groupScale can be unhidden, the formula should be: title.offsetWidth + subject.offsetWidth + groupRotate.offsetWidth + el.widths[2] + groupPage.offsetWidth + controls.offsetWidth < toolbar.offsetWidth. The current calculation omits subject and uses groupScale.offsetWidth (which is 0 when hidden) instead of just using el.widths[2].
| if (title.offsetWidth + el.widths[2] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { | |
| if (title.offsetWidth + subject.offsetWidth + groupRotate.offsetWidth + el.widths[2] + groupPage.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The width calculation is missing subject.offsetWidth and incorrectly includes the hidden element's current width. When checking if groupRotate can be unhidden, the formula should be: title.offsetWidth + subject.offsetWidth + el.widths[1] + groupScale.offsetWidth + groupPage.offsetWidth + controls.offsetWidth < toolbar.offsetWidth. The current calculation omits subject and uses groupRotate.offsetWidth (which is 0 when hidden) instead of just using el.widths[1].
| if (title.offsetWidth + el.widths[1] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { | |
| if (title.offsetWidth + subject.offsetWidth + el.widths[1] + groupScale.offsetWidth + groupPage.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The width calculation incorrectly includes both el.widths[0] and the sum of other current widths. When checking if subject can be unhidden, since all other elements are visible at this point, the formula should simply be: title.offsetWidth + el.widths[0] + groupRotate.offsetWidth + groupScale.offsetWidth + groupPage.offsetWidth + controls.offsetWidth < toolbar.offsetWidth. Remove the redundant addition as it's already using stored width for subject.
| if (title.offsetWidth + el.widths[0] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { | |
| if (title.offsetWidth + el.widths[0] + groupRotate.offsetWidth + groupScale.offsetWidth + groupPage.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
| else if (subject.classList.contains("d-none")) { | |
| if (title.offsetWidth + el.widths[0] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { | |
| subject.classList.remove("d-none"); | |
| } | |
| } | |
| else if (subject.classList.contains("d-none") && title.offsetWidth + el.widths[0] + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth < toolbar.offsetWidth) { | |
| subject.classList.remove("d-none"); | |
| } | |
Explanation
Reading deeply nested conditional code is confusing, since you have to keep track of whichconditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two
if conditions can be combined usingand is an easy win.
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The width calculation is missing subject.offsetWidth. The while condition should be: title.offsetWidth + subject.offsetWidth + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth > toolbar.offsetWidth. Without including subject's width when it's visible, the function will not hide elements when it should, causing toolbar overflow.
| while (title.offsetWidth + groupPage.offsetWidth + groupScale.offsetWidth + groupRotate.offsetWidth + controls.offsetWidth > toolbar.offsetWidth) { | |
| while ( | |
| title.offsetWidth | |
| + (subject.classList.contains("d-none") ? 0 : subject.offsetWidth) | |
| + groupPage.offsetWidth | |
| + groupScale.offsetWidth | |
| + groupRotate.offsetWidth | |
| + controls.offsetWidth | |
| > toolbar.offsetWidth | |
| ) { |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ResizeObserver callback queries the DOM for .bb-view-toolbar on every resize event. Consider querying this element once during initialization (line 96-104) and storing it in a closure variable, then reuse it in the callback. This avoids repeated DOM queries on every resize.
| const observer = new ResizeObserver(entries => { | |
| const toolbar = el.querySelector(".bb-view-toolbar"); | |
| if (toolbar === null) { | |
| return; | |
| } | |
| const toolbar = el.querySelector(".bb-view-toolbar"); | |
| if (toolbar === null) { | |
| return; | |
| } | |
| const observer = new ResizeObserver(entries => { |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assignment to observer has no effect as it's a function parameter. The assignment observer = null only modifies the local variable and doesn't prevent memory leaks. Since the observer is already disconnected on line 388, this line can be removed or is simply redundant.
| observer = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider refactoring
setObserverto use a data-driven visibility model and correcting thedisposeobserver handling to reduce branching and DOM state complexity.You can simplify
setObserverquite a bit by making the toolbar behavior data‑driven and avoiding ad‑hoc DOM state (el.widths), while also fixing theobserverlifecycle indispose.1. Replace imperative
increaseSpace/decreaseSpacechains with data‑driven logicBoth functions encode the same ordering twice. You can centralize the order and visibility logic in one array and a pair of small helpers:
Key improvements:
groups.getTotalWidth()instead of repeated expressions.el.widthsor mixing layout state into the DOM element.You can tweak the collapse/expand order to exactly match your current behavior (e.g., start collapsing from
subjectfirst, etc.) by changing the order ingroupsor the iteration direction.2. Avoid
el.widthsas ad‑hoc storageThe above refactor removes
el.widthsentirely. If you still want cached widths, keep them in a closure variable instead of mutating the DOM node:This keeps layout state local and self‑contained.
3. Fix
disposeobserver handlingYou’re destructuring
observeras aconstand then trying to reassign it. Also,Data.remove(id)is called before you useel/observer. A safer pattern:This keeps the observer lifecycle clear and avoids illegal reassignment.