feat(PdfReader): add OnRotationChanged parameter#758
Merged
Conversation
Reviewer's GuideAdds rotation angle state and rotation-changed callback support to the PdfReader component, wiring it through Blazor parameters, JS interop, and the underlying PDF.js event bus. Sequence diagram for PdfReader rotation change handlingsequenceDiagram
actor User
participant PdfViewer as Pdf.js_viewer
participant EventBus as PdfJs_eventBus
participant JsModule as PdfReader_js
participant DotNet as PdfReader_component
participant Host as Host_app_callback
User->>PdfViewer: Rotate page (UI control)
PdfViewer-->>EventBus: rotationchanging evt
EventBus-->>JsModule: rotationchanging evt
alt options.triggerRotationChanged is true
JsModule->>DotNet: invokeMethodAsync RotationChanged(evt.pagesRotation)
DotNet->>DotNet: RotationChanged(angle)
alt OnRotationChanged is not null
DotNet->>Host: OnRotationChanged(angle)
Host-->>DotNet: Task completion
end
end
rect rgb(230,230,250)
User->>Host: Set CurrentRotation parameter
Host->>DotNet: Render PdfReader with CurrentRotation
DotNet->>DotNet: OnAfterRenderAsync(firstRender or update)
DotNet->>JsModule: rotate(Id, _currentRotation)
JsModule-->>PdfViewer: Apply rotation
end
Class diagram for updated PdfReader rotation supportclassDiagram
class PdfReader {
+uint CurrentPage
+int CurrentRotation
+bool FitWidth
+Func_float_Task OnScaleChangedAsync()
+Func_int_Task OnRotationChanged()
-string _docTitle
-uint _currentPage
-float _currentRotation
-string _url
-string _dropdownItemCheckIcon
-string _dropdownItemDefaultIcon
+Task OnAfterRenderAsync(bool firstRender)
+Task Printing()
+Task RotationChanged(int angle)
}
class PdfReaderJsInterop {
+void addEventBus(el, pdfViewer, eventBus, invoke, options)
}
PdfReader --> PdfReaderJsInterop : uses JS interop
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 - here's some feedback:
- The new rotation state field is declared as a float while the public
CurrentRotationparameter is an int; consider making_currentRotationan int as well to avoid unnecessary type conversions and potential comparison issues. - The
OnRotationChangedparameter returns aTaskbut does not follow the existing*Asyncnaming convention (e.g.,OnScaleChangedAsync); consider renaming for consistency across the component’s async callbacks. - Since
CurrentRotationis documented as only supporting 0/90/180/270, consider validating or normalizing incoming values before passing them to the JSrotatecall to prevent undefined behavior with other angles.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new rotation state field is declared as a float while the public `CurrentRotation` parameter is an int; consider making `_currentRotation` an int as well to avoid unnecessary type conversions and potential comparison issues.
- The `OnRotationChanged` parameter returns a `Task` but does not follow the existing `*Async` naming convention (e.g., `OnScaleChangedAsync`); consider renaming for consistency across the component’s async callbacks.
- Since `CurrentRotation` is documented as only supporting 0/90/180/270, consider validating or normalizing incoming values before passing them to the JS `rotate` call to prevent undefined behavior with other angles.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.cs:151` </location>
<code_context>
private string? _docTitle;
private uint _currentPage;
+ private float _currentRotation;
private string? _url;
private string? _dropdownItemCheckIcon;
</code_context>
<issue_to_address>
**issue (bug_risk):** Use a consistent integer type for rotation instead of mixing int and float.
`CurrentRotation` is an `int` with documented discrete values (0, 90, 180, 270), but the backing field is a `float`. This mismatch is unnecessary and could mask accidental non-discrete values in future changes. Please make `_currentRotation` an `int` so the type matches the public API, the value domain, and the comparisons in `OnAfterRenderAsync`.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.cs:351-363` </location>
<code_context>
+ /// <param name="angle"></param>
+ /// <returns></returns>
+ [JSInvokable]
+ public async Task RotationChanged(int angle)
+ {
+ if (OnRotationChanged != null)
+ {
+ await OnRotationChanged(angle);
+ }
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider updating the component’s internal rotation state when the JS callback fires.
`RotationChanged(int angle)` only calls `OnRotationChanged` and never updates `CurrentRotation` / `_currentRotation`, so the internal state can diverge from the viewer’s actual rotation when changes come from JS. If `CurrentRotation` is expected to always reflect the latest rotation, consider assigning it here as well, or clearly document that this method is notification-only and does not update state.
```suggestion
/// <summary>
/// 页面旋转回调方法
/// </summary>
/// <param name="angle">当前页面旋转角度</param>
/// <returns></returns>
[JSInvokable]
public async Task RotationChanged(int angle)
{
// Synchronize internal rotation state with the value coming from JS
if (CurrentRotation != angle)
{
CurrentRotation = angle;
}
if (_currentRotation != angle)
{
_currentRotation = angle;
}
if (OnRotationChanged != null)
{
await OnRotationChanged(angle);
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| private string? _docTitle; | ||
| private uint _currentPage; | ||
| private float _currentRotation; |
There was a problem hiding this comment.
issue (bug_risk): Use a consistent integer type for rotation instead of mixing int and float.
CurrentRotation is an int with documented discrete values (0, 90, 180, 270), but the backing field is a float. This mismatch is unnecessary and could mask accidental non-discrete values in future changes. Please make _currentRotation an int so the type matches the public API, the value domain, and the comparisons in OnAfterRenderAsync.
There was a problem hiding this comment.
Pull request overview
This PR adds a rotation change callback feature to the PdfReader component, allowing consumers to track when the PDF rotation angle changes. The implementation follows the existing event callback pattern used for other PDF viewer events like page changes and scale changes.
- Adds
OnRotationChangedcallback parameter to notify when PDF rotation changes - Implements JavaScript event handler for
rotationchangingevent with callback to C# - Updates initialization options to include rotation change trigger flag
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| PdfReader.razor.js | Added async callback invocation for rotationchanging event when triggerRotationChanged is enabled |
| PdfReader.razor.cs | Added OnRotationChanged parameter, RotationChanged method, CurrentRotation property with change detection, and TriggerRotationChanged option |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// 页面旋转回调方法 | ||
| /// </summary> | ||
| [Parameter] | ||
| public Func<int, Task>? OnRotationChanged { get; set; } |
There was a problem hiding this comment.
The callback parameter name 'OnRotationChanged' is inconsistent with the naming convention used by other callbacks in this class. All other callback parameters follow the 'OnXxxAsync' pattern (e.g., 'OnPagesInitAsync', 'OnPageChangedAsync', 'OnScaleChangedAsync'). This should be renamed to 'OnRotationChangedAsync' to maintain consistency.
Suggested change
| public Func<int, Task>? OnRotationChanged { get; set; } | |
| public Func<int, Task>? OnRotationChangedAsync { get; set; } |
|
|
||
| private string? _docTitle; | ||
| private uint _currentPage; | ||
| private float _currentRotation; |
There was a problem hiding this comment.
Type mismatch: The field '_currentRotation' is declared as 'float', but the 'CurrentRotation' property is 'int'. This inconsistency could lead to type conversion issues. Since rotation angles are discrete values (0, 90, 180, 270) as documented, '_currentRotation' should be 'int' to match the property type.
Suggested change
| private float _currentRotation; | |
| private int _currentRotation; |
|
|
||
| /// <summary> | ||
| /// 获得/设置 当前缩放倍率 默认 null 使用 100% | ||
| /// 获得/设置 当前旋转角度 默认 0 数值范围 0 90 180 270 |
There was a problem hiding this comment.
The comment describes the CurrentRotation property but appears in the diff as replacing the previous CurrentScale comment. While the comment itself is correct for CurrentRotation, verify this is intentional and that CurrentScale was truly removed. If CurrentScale was actually removed as a breaking change, this should be documented in the PR description or CHANGELOG.
| _currentPage = CurrentPage; | ||
| await NavigateToPageAsync(_currentPage); | ||
| } | ||
| if (_currentRotation != CurrentRotation) |
There was a problem hiding this comment.
Equality checks on floating point values can yield unexpected results.
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 #757
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add rotation state and callbacks to PdfReader to support reacting to page rotation changes.
New Features:
Enhancements: