Conversation
This reverts commit b980542.
# Conflicts: # src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js
Reviewer's GuideAdds support for a two-pages-on-view mode to the PdfReader component, including UI, options, JS interop wiring, and safer event handling, while adjusting default scale behavior. Sequence diagram for toggling two-pages-on-view modesequenceDiagram
actor User
participant PdfReader as PdfReader_component
participant Dropdown as Toolbar_dropdown
participant JSRuntime as Blazor_JSRuntime
participant PdfJs as PdfReader_razor_js
participant Viewer as PdfViewer_instance
User->>Dropdown: click dropdown-item-pages
Dropdown->>PdfReader: invoke OnToggleTwoPagesOneView
PdfReader->>PdfReader: _enableTwoPagesOneView = !_enableTwoPagesOneView
PdfReader->>PdfReader: Options.EnableTwoPagesOnView = _enableTwoPagesOneView
PdfReader->>PdfReader: _twoPagesOneViewIcon based on _enableTwoPagesOneView
PdfReader-->>User: re-render toolbar with updated icon
PdfReader->>JSRuntime: InvokeVoidAsync setPages(Id, _enableTwoPagesOneView)
JSRuntime->>PdfJs: setPages(id, enableTowPagesOnView)
PdfJs->>Viewer: set pdfViewer.spreadMode (1 or 0)
Viewer-->>PdfJs: pages re-layout in two-page or single-page
PdfJs-->>User: updated PDF layout displayed
Sequence diagram for initial PdfReader render with two-pages-on-view optionsequenceDiagram
participant App as Parent_component
participant PdfReader as PdfReader_component
participant JSRuntime as Blazor_JSRuntime
participant PdfJs as PdfReader_razor_js
participant Viewer as PdfViewer_instance
App->>PdfReader: set Options (EnableTwoPagesOnView, ShowTwoPagesOnViewButton, Url, CurrentScale)
PdfReader->>PdfReader: OnParametersSet
PdfReader->>PdfReader: set _docTitle from Options.Url
PdfReader->>PdfReader: MoreButtonIcon default if null
PdfReader->>PdfReader: _twoPagesOneViewIcon default if null
PdfReader->>PdfReader: OnAfterRenderAsync(firstRender = true)
PdfReader->>PdfReader: cache _currentPage, _url, _currentScale, _enableTwoPagesOneView
PdfReader->>JSRuntime: initPdfReaderCore with TriggerTowPagesOnViewChanged
JSRuntime->>PdfJs: initialize pdf viewer
PdfJs->>Viewer: create PdfViewer with options
PdfReader->>JSRuntime: InvokeVoidAsync scale(Id, _currentScale)
JSRuntime->>PdfJs: scale(id, scale)
PdfJs->>Viewer: set currentScaleValue
alt _enableTwoPagesOneView true
PdfReader->>JSRuntime: InvokeVoidAsync setPages(Id, _enableTwoPagesOneView)
JSRuntime->>PdfJs: setPages(id, enableTowPagesOnView)
PdfJs->>Viewer: set spreadMode = 1
else _enableTwoPagesOneView false
PdfJs->>Viewer: default spreadMode = 0
end
Class diagram for updated PdfReader and PdfReaderOptionsclassDiagram
class PdfReaderOptions {
bool IsFitToPage
bool ShowTwoPagesOnViewButton
bool EnableTwoPagesOnView
Func_uint_Task OnInitAsync
Func_uint_Task OnPageChangedAsync
Func_bool_Task OnTwoPagesOneViewAsync
}
class PdfReader {
+PdfReaderOptions Options
+string MoreButtonIcon
string ClassString
string DocumentTitleString
uint _currentPage
string _url
string _currentScale
bool _enableTwoPagesOneView
string _twoPagesOneViewIcon
HashSet_string_ AllowedScaleValues
void SetCurrentScale(string value)
void OnToggleTwoPagesOneView()
void OnParametersSet()
Task OnAfterRenderAsync(bool firstRender)
Task InitPdfReaderCore(PdfReaderOptions options)
}
PdfReader --> PdfReaderOptions : uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- There are several inconsistent spellings of the two‑page mode across JS/C# (
TwoPagesOnViewvsTowPagesOnView, e.g.setPagesparameter andTriggerTowPagesOnViewChanged), which will be confusing and make future refactoring/error searching harder; consider standardizing all public and internal names to a single, correctly spelled form. - The two‑pages toggle currently has both a Blazor
@onclickhandler and a JSEventHandler.on(..., "click", ...)on.dropdown-item-pages; this double binding makes the behavior harder to reason about and may surprise future maintainers, so it would be better to centralize the toggle in one place (either JS or Blazor) and keep the other side purely in sync. - In
setPages,Data.get(id)is used without checking for a missing entry, and theelbinding is unused; consider removing the unusedeldestructuring and adding a null/undefined guard for theData.getresult to avoid potential runtime errors if the viewer is disposed or not initialized.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are several inconsistent spellings of the two‑page mode across JS/C# (`TwoPagesOnView` vs `TowPagesOnView`, e.g. `setPages` parameter and `TriggerTowPagesOnViewChanged`), which will be confusing and make future refactoring/error searching harder; consider standardizing all public and internal names to a single, correctly spelled form.
- The two‑pages toggle currently has both a Blazor `@onclick` handler and a JS `EventHandler.on(..., "click", ...)` on `.dropdown-item-pages`; this double binding makes the behavior harder to reason about and may surprise future maintainers, so it would be better to centralize the toggle in one place (either JS or Blazor) and keep the other side purely in sync.
- In `setPages`, `Data.get(id)` is used without checking for a missing entry, and the `el` binding is unused; consider removing the unused `el` destructuring and adding a null/undefined guard for the `Data.get` result to avoid potential runtime errors if the viewer is disposed or not initialized.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:105-114` </location>
<code_context>
}
}
+export function setPages(id, enableTowPagesOnView) {
+ const { el, pdfViewer } = Data.get(id);
+ if (pdfViewer) {
</code_context>
<issue_to_address>
**suggestion (typo):** Align naming ("Two" vs "Tow") for the two-pages-on-view feature across JS and C#.
The parameter `enableTowPagesOnView`, local `towPagesOneView`, and C# flag `TriggerTowPagesOnViewChanged` all use `Tow` instead of `Two`. Please rename them to `enableTwoPagesOnView` / `twoPagesOneView` / `TriggerTwoPagesOnViewChanged` to match `EnableTwoPagesOnView` / `ShowTwoPagesOnViewButton` and the UI text, and to keep the feature easy to search for and reason about.
Suggested implementation:
```javascript
export function setPages(id, enableTwoPagesOnView) {
const { el, pdfViewer } = Data.get(id);
if (pdfViewer) {
if (enableTwoPagesOnView) {
pdfViewer.spreadMode = 1;
}
else {
pdfViewer.spreadMode = 0;
}
}
}
```
```javascript
twoPagesOneView
```
To fully align the naming across the codebase, you should also:
1. In the corresponding C# files (likely in the PdfReader component and any related event args or services), rename:
- `TriggerTowPagesOnViewChanged` → `TriggerTwoPagesOnViewChanged`
- Any other `TowPagesOnView` occurrences → `TwoPagesOnView`.
2. Update any JS interop calls that reference `setPages` so that they pass the parameter named `enableTwoPagesOnView` instead of `enableTowPagesOnView`.
3. Search the entire repository for `TowPagesOnView` and `towPagesOneView` to ensure all usages are consistently renamed.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.cs:159-162` </location>
<code_context>
_currentScale = Options.CurrentScale;
await InvokeVoidAsync("scale", Id, _currentScale);
}
+ if (_enableTwoPagesOneView != Options.EnableTwoPagesOnView)
+ {
+ _currentScale = Options.CurrentScale;
+ await InvokeVoidAsync("setPages", Id, _enableTwoPagesOneView);
+ }
}
</code_context>
<issue_to_address>
**issue (bug_risk):** The two-page view sync block likely uses the wrong value and never updates local state, causing external changes to be reverted.
Here you compare `_enableTwoPagesOneView` with `Options.EnableTwoPagesOnView`, but then call `setPages` with the stale `_enableTwoPagesOneView` and never update it.
If a parent changes `Options.EnableTwoPagesOnView` from `false` to `true` without calling `OnToggleTwoPagesOneView`, this block will fire but still pass `false` to `setPages`, undoing the parent’s change on the JS side.
Consider instead:
```csharp
if (_enableTwoPagesOneView != Options.EnableTwoPagesOnView)
{
_enableTwoPagesOneView = Options.EnableTwoPagesOnView;
await InvokeVoidAsync("setPages", Id, _enableTwoPagesOneView);
}
```
(or pass `Options.EnableTwoPagesOnView` directly and then sync `_enableTwoPagesOneView`.)
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.cs:98` </location>
<code_context>
+ _enableTwoPagesOneView = !_enableTwoPagesOneView;
+ Options.EnableTwoPagesOnView = _enableTwoPagesOneView;
+
+ _twoPagesOneViewIcon = _enableTwoPagesOneView ? "fa-solid fa-fw fa-check" : "fa-solid fa-fw";
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The icon state for two-page view is only updated via the local toggle method, not when Options is changed externally.
Because `_twoPagesOneViewIcon` is only set in `OnToggleTwoPagesOneView`, but `_enableTwoPagesOneView` is also updated from `Options.EnableTwoPagesOnView` in `OnAfterRenderAsync`, a parent updating `Options.EnableTwoPagesOnView` directly can leave the icon out of sync with the actual state.
To avoid this, either recompute `_twoPagesOneViewIcon` whenever `_enableTwoPagesOneView` is updated from `Options`, or derive the icon directly from `_enableTwoPagesOneView` instead of keeping a separate field.
Suggested implementation:
```csharp
private void OnToggleTwoPagesOneView()
{
_enableTwoPagesOneView = !_enableTwoPagesOneView;
Options.EnableTwoPagesOnView = _enableTwoPagesOneView;
}
```
```csharp
private uint _currentPage;
private string TwoPagesOneViewIcon => _enableTwoPagesOneView ? "fa-solid fa-fw fa-check" : "fa-solid fa-fw";
```
1. Remove the `_twoPagesOneViewIcon` field declaration from this file, e.g.:
- `private string _twoPagesOneViewIcon = "fa-solid fa-fw";`
2. In `PdfReader.razor` (the markup file), replace usages of `_twoPagesOneViewIcon` with `TwoPagesOneViewIcon`, for example:
- `class="@_twoPagesOneViewIcon"` → `class="@TwoPagesOneViewIcon"`.
3. Ensure any other references to `_twoPagesOneViewIcon` (if present elsewhere in the code-behind) are updated or removed in favor of the `TwoPagesOneViewIcon` property.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+159
to
+162
| if (_enableTwoPagesOneView != Options.EnableTwoPagesOnView) | ||
| { | ||
| _currentScale = Options.CurrentScale; | ||
| await InvokeVoidAsync("setPages", Id, _enableTwoPagesOneView); |
There was a problem hiding this comment.
issue (bug_risk): The two-page view sync block likely uses the wrong value and never updates local state, causing external changes to be reverted.
Here you compare _enableTwoPagesOneView with Options.EnableTwoPagesOnView, but then call setPages with the stale _enableTwoPagesOneView and never update it.
If a parent changes Options.EnableTwoPagesOnView from false to true without calling OnToggleTwoPagesOneView, this block will fire but still pass false to setPages, undoing the parent’s change on the JS side.
Consider instead:
if (_enableTwoPagesOneView != Options.EnableTwoPagesOnView)
{
_enableTwoPagesOneView = Options.EnableTwoPagesOnView;
await InvokeVoidAsync("setPages", Id, _enableTwoPagesOneView);
}(or pass Options.EnableTwoPagesOnView directly and then sync _enableTwoPagesOneView.)
| _enableTwoPagesOneView = !_enableTwoPagesOneView; | ||
| Options.EnableTwoPagesOnView = _enableTwoPagesOneView; | ||
|
|
||
| _twoPagesOneViewIcon = _enableTwoPagesOneView ? "fa-solid fa-fw fa-check" : "fa-solid fa-fw"; |
There was a problem hiding this comment.
suggestion (bug_risk): The icon state for two-page view is only updated via the local toggle method, not when Options is changed externally.
Because _twoPagesOneViewIcon is only set in OnToggleTwoPagesOneView, but _enableTwoPagesOneView is also updated from Options.EnableTwoPagesOnView in OnAfterRenderAsync, a parent updating Options.EnableTwoPagesOnView directly can leave the icon out of sync with the actual state.
To avoid this, either recompute _twoPagesOneViewIcon whenever _enableTwoPagesOneView is updated from Options, or derive the icon directly from _enableTwoPagesOneView instead of keeping a separate field.
Suggested implementation:
private void OnToggleTwoPagesOneView()
{
_enableTwoPagesOneView = !_enableTwoPagesOneView;
Options.EnableTwoPagesOnView = _enableTwoPagesOneView;
} private uint _currentPage;
private string TwoPagesOneViewIcon => _enableTwoPagesOneView ? "fa-solid fa-fw fa-check" : "fa-solid fa-fw";- Remove the
_twoPagesOneViewIconfield declaration from this file, e.g.:private string _twoPagesOneViewIcon = "fa-solid fa-fw";
- In
PdfReader.razor(the markup file), replace usages of_twoPagesOneViewIconwithTwoPagesOneViewIcon, for example:class="@_twoPagesOneViewIcon"→class="@TwoPagesOneViewIcon".
- Ensure any other references to
_twoPagesOneViewIcon(if present elsewhere in the code-behind) are updated or removed in favor of theTwoPagesOneViewIconproperty.
There was a problem hiding this comment.
Pull request overview
This PR adds a "Two Pages on View" feature to the PdfReader component, allowing users to toggle between single-page and two-page spread modes. The implementation includes UI controls, JavaScript interop, and configuration options for the new viewing mode.
Key Changes:
- Added configuration options for showing/enabling the two-pages-on-view button and mode
- Implemented JavaScript functionality to toggle PDF spread mode
- Added dropdown menu with "More" button to house the new feature
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| PdfReaderOptions.cs | Added properties to control two-pages-on-view button visibility, mode state, and callback |
| PdfReader.razor.js | Implemented setPages function and click handlers for toggling spread mode |
| PdfReader.razor.css | Added CSS styling for the new toolbar button and dropdown |
| PdfReader.razor.cs | Added backend logic to manage two-pages-on-view state and synchronization |
| PdfReader.razor | Updated UI to include dropdown menu with two-pages-on-view toggle option |
| BootstrapBlazor.PdfReader.csproj | Version bump and ItemGroup reorganization |
Comments suppressed due to low confidence (1)
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:160
- Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+105
to
+108
| export function setPages(id, enableTowPagesOnView) { | ||
| const { el, pdfViewer } = Data.get(id); | ||
| if (pdfViewer) { | ||
| if (enableTowPagesOnView) { |
There was a problem hiding this comment.
Corrected spelling of 'Tow' to 'Two' in parameter name 'enableTowPagesOnView'.
Suggested change
| export function setPages(id, enableTowPagesOnView) { | |
| const { el, pdfViewer } = Data.get(id); | |
| if (pdfViewer) { | |
| if (enableTowPagesOnView) { | |
| export function setPages(id, enableTwoPagesOnView) { | |
| const { el, pdfViewer } = Data.get(id); | |
| if (pdfViewer) { | |
| if (enableTwoPagesOnView) { |
| EventHandler.on(minus, "click", e => updateScale(pdfViewer, e.target, -1)); | ||
| EventHandler.on(plus, "click", e => updateScale(pdfViewer, e.target, 1)); | ||
|
|
||
| const towPagesOneView = el.querySelector(".dropdown-item-pages"); |
There was a problem hiding this comment.
Corrected spelling of 'tow' to 'two' in variable name 'towPagesOneView'.
| EventHandler.off(plus, "click"); | ||
| } | ||
|
|
||
| const towPagesOneView = el.querySelector(".dropdown-item-pages"); |
There was a problem hiding this comment.
Corrected spelling of 'tow' to 'two' in variable name 'towPagesOneView'.
Comment on lines
+63
to
+65
| /// 设置双页单视图模式回调方法 | ||
| /// </summary> | ||
| public Func<bool, Task>? OnTwoPagesOneViewAsync { get; set; } |
There was a problem hiding this comment.
The callback property name 'OnTwoPagesOneViewAsync' is inconsistent with the feature name used elsewhere ('TwoPagesOnView'). Consider renaming to 'OnTwoPagesOnViewChangedAsync' to match the naming pattern of other callbacks like 'OnPageChangedAsync' and align with 'ShowTwoPagesOnViewButton' and 'EnableTwoPagesOnView'.
Suggested change
| /// 设置双页单视图模式回调方法 | |
| /// </summary> | |
| public Func<bool, Task>? OnTwoPagesOneViewAsync { get; set; } | |
| /// 双页单视图模式变更回调方法 | |
| /// </summary> | |
| public Func<bool, Task>? OnTwoPagesOnViewChangedAsync { get; set; } |
| TriggerPagesInit = Options.OnInitAsync != null, | ||
| TriggerPageChanged = Options.OnPageChangedAsync != null | ||
| TriggerPageChanged = Options.OnPageChangedAsync != null, | ||
| TriggerTowPagesOnViewChanged = Options.OnTwoPagesOneViewAsync != null |
There was a problem hiding this comment.
Corrected spelling of 'Tow' to 'Two' in property name 'TriggerTowPagesOnViewChanged'.
Suggested change
| TriggerTowPagesOnViewChanged = Options.OnTwoPagesOneViewAsync != null | |
| TriggerTwoPagesOnViewChanged = Options.OnTwoPagesOneViewAsync != null |
Comment on lines
+167
to
+173
| EventHandler.on(towPagesOneView, "click", e => { | ||
| if (pdfViewer.spreadMode === 0) { | ||
| pdfViewer.spreadMode = 1; | ||
| } | ||
| else { | ||
| pdfViewer.spreadMode = 0; | ||
| } |
There was a problem hiding this comment.
The click handler in addEventListener toggles the spreadMode but doesn't invoke the C# callback 'OnTwoPagesOneViewAsync' even though 'TriggerTowPagesOnViewChanged' is passed in options. If the callback should be triggered, add an async invocation similar to other event handlers (e.g., pageChanged). If the callback is not needed, remove the unused 'TriggerTowPagesOnViewChanged' property from options.
Suggested change
| EventHandler.on(towPagesOneView, "click", e => { | |
| if (pdfViewer.spreadMode === 0) { | |
| pdfViewer.spreadMode = 1; | |
| } | |
| else { | |
| pdfViewer.spreadMode = 0; | |
| } | |
| EventHandler.on(towPagesOneView, "click", async e => { | |
| if (pdfViewer.spreadMode === 0) { | |
| pdfViewer.spreadMode = 1; | |
| } | |
| else { | |
| pdfViewer.spreadMode = 0; | |
| } | |
| if (options.TriggerTowPagesOnViewChanged === true) { | |
| await invoke.invokeMethodAsync("OnTwoPagesOneViewAsync", pdfViewer.spreadMode); | |
| } |
| } | ||
| if (_enableTwoPagesOneView != Options.EnableTwoPagesOnView) | ||
| { | ||
| _currentScale = Options.CurrentScale; |
There was a problem hiding this comment.
The JavaScript function is called with the old value '_enableTwoPagesOneView' before it's updated. The assignment should be '_enableTwoPagesOneView = Options.EnableTwoPagesOnView;' and then pass the updated value to 'setPages'.
Suggested change
| _currentScale = Options.CurrentScale; | |
| _enableTwoPagesOneView = Options.EnableTwoPagesOnView; |
| <ItemsTemplate> | ||
| @if (Options.ShowTwoPagesOnViewButton) | ||
| { | ||
| <div class="dropdown-item dropdown-item-pages" @onclick="OnToggleTwoPagesOneView"><i class="@_twoPagesOneViewIcon"></i><span>Two pages on view</span></div> |
There was a problem hiding this comment.
[nitpick] The text 'Two pages on view' is hardcoded in English while other component strings appear to support Chinese (based on C# comments). Consider localizing this string for consistency with the rest of the component, especially since the codebase uses Chinese documentation.
| } | ||
|
|
||
| export function setPages(id, enableTowPagesOnView) { | ||
| const { el, pdfViewer } = Data.get(id); |
There was a problem hiding this comment.
Unused variable el.
Suggested change
| const { el, pdfViewer } = Data.get(id); | |
| const { pdfViewer } = Data.get(id); |
| const step = [25, 33, 50, 67, 75, 80, 90, 100, 110, 125, 150, 175, 200, 250, 300, 400, 500]; | ||
| const findValues = step.filter(s => rate > 0 ? current < s : current > s); | ||
| const findValues = step.filter(s => rate > 0 ? current < s : current > s); | ||
| let v = 100; |
There was a problem hiding this comment.
The initial value of v is unused, since it is always overwritten.
Suggested change
| let v = 100; | |
| let v; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #720
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add configurable two-pages-on-view mode and overflow menu to the PdfReader toolbar, including state synchronization with the underlying PDF viewer.
New Features:
Enhancements: