-
-
Notifications
You must be signed in to change notification settings - Fork 7
feat(PdfReader): add TwoPagesOnView function #721
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
b980542
b0a2b75
7b3659b
6922099
dc35378
950e6e6
6367dd5
b0bc84f
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,6 +20,12 @@ public partial class PdfReader | |||||||||
| [NotNull] | ||||||||||
| public PdfReaderOptions? Options { get; set; } | ||||||||||
|
|
||||||||||
| /// <summary> | ||||||||||
| /// 获得/设置 更多按钮图标 默认为 null 使用内置图标 | ||||||||||
| /// </summary> | ||||||||||
| [Parameter] | ||||||||||
| public string? MoreButtonIcon { get; set; } | ||||||||||
|
|
||||||||||
| private string? ClassString => CssBuilder.Default("bb-pdf-reader") | ||||||||||
| .AddClassFromAttributes(AdditionalAttributes) | ||||||||||
| .Build(); | ||||||||||
|
|
@@ -38,6 +44,8 @@ public partial class PdfReader | |||||||||
| private uint _currentPage; | ||||||||||
| private string? _url; | ||||||||||
| private string? _currentScale; | ||||||||||
| private bool _enableTwoPagesOneView; | ||||||||||
| private string? _twoPagesOneViewIcon; | ||||||||||
|
|
||||||||||
| private readonly HashSet<string> AllowedScaleValues = ["page-actual", "page-width", "page-height", "page-fit", "auto"]; | ||||||||||
|
|
||||||||||
|
|
@@ -82,6 +90,14 @@ private void SetCurrentScale(string value) | |||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private void OnToggleTwoPagesOneView() | ||||||||||
| { | ||||||||||
| _enableTwoPagesOneView = !_enableTwoPagesOneView; | ||||||||||
| Options.EnableTwoPagesOnView = _enableTwoPagesOneView; | ||||||||||
|
|
||||||||||
| _twoPagesOneViewIcon = _enableTwoPagesOneView ? "fa-solid fa-fw fa-check" : "fa-solid fa-fw"; | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 To avoid this, either recompute 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";
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| /// <summary> | ||||||||||
| /// <inheritdoc/> | ||||||||||
| /// </summary> | ||||||||||
|
|
@@ -96,6 +112,9 @@ protected override void OnParametersSet() | |||||||||
| Options.CurrentPage = 1; | ||||||||||
| } | ||||||||||
| _docTitle = Path.GetFileName(Options.Url); | ||||||||||
|
|
||||||||||
| MoreButtonIcon ??= "fa-solid fa-ellipsis-vertical"; | ||||||||||
| _twoPagesOneViewIcon ??= "fa-solid fa-fw"; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /// <summary> | ||||||||||
|
|
@@ -113,6 +132,7 @@ protected override async Task OnAfterRenderAsync(bool firstRender) | |||||||||
| _currentPage = Options.CurrentPage; | ||||||||||
| _url = Options.Url; | ||||||||||
| _currentScale = Options.CurrentScale; | ||||||||||
| _enableTwoPagesOneView = Options.EnableTwoPagesOnView; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (_url != Options.Url) | ||||||||||
|
|
@@ -136,6 +156,11 @@ protected override async Task OnAfterRenderAsync(bool firstRender) | |||||||||
| _currentScale = Options.CurrentScale; | ||||||||||
| await InvokeVoidAsync("scale", Id, _currentScale); | ||||||||||
| } | ||||||||||
| if (_enableTwoPagesOneView != Options.EnableTwoPagesOnView) | ||||||||||
| { | ||||||||||
| _currentScale = Options.CurrentScale; | ||||||||||
|
||||||||||
| _currentScale = Options.CurrentScale; | |
| _enableTwoPagesOneView = Options.EnableTwoPagesOnView; |
Copilot
AI
Nov 26, 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 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'.
| _currentScale = Options.CurrentScale; | |
| _enableTwoPagesOneView = Options.EnableTwoPagesOnView; |
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 (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.)
Copilot
AI
Nov 26, 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.
Corrected spelling of 'Tow' to 'Two' in property name 'TriggerTowPagesOnViewChanged'.
| TriggerTowPagesOnViewChanged = Options.OnTwoPagesOneViewAsync != null | |
| TriggerTwoPagesOnViewChanged = Options.OnTwoPagesOneViewAsync != null |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -102,13 +102,25 @@ export function scale(id, scale) { | |||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| export function setPages(id, enableTowPagesOnView) { | ||||||||||||||||||||||||||||||||||||
| const { el, pdfViewer } = Data.get(id); | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
| const { el, pdfViewer } = Data.get(id); | |
| const { pdfViewer } = Data.get(id); |
Copilot
AI
Nov 26, 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.
Corrected spelling of 'Tow' to 'Two' in parameter name 'enableTowPagesOnView'.
| 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) { |
Copilot
AI
Nov 26, 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.
Corrected spelling of 'tow' to 'two' in variable name 'towPagesOneView'.
Copilot
AI
Nov 26, 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 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.
| 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); | |
| } |
Copilot
AI
Nov 26, 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 initial value of v is unused, since it is always overwritten.
| let v = 100; | |
| let v; |
Copilot
AI
Nov 26, 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.
Corrected spelling of 'tow' to 'two' in variable name 'towPagesOneView'.
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -39,6 +39,16 @@ public class PdfReaderOptions | |||||||||||||
| /// </summary> | ||||||||||||||
| public bool IsFitToPage { get; set; } | ||||||||||||||
|
|
||||||||||||||
| /// <summary> | ||||||||||||||
| /// 获得/设置 是否显示双页单视图按钮 默认 true 显示 | ||||||||||||||
| /// </summary> | ||||||||||||||
| public bool ShowTwoPagesOnViewButton { get; set; } = true; | ||||||||||||||
|
|
||||||||||||||
| /// <summary> | ||||||||||||||
| /// 获得/设置 是否启用双页单视图模式 默认 false | ||||||||||||||
| /// </summary> | ||||||||||||||
| public bool EnableTwoPagesOnView { get; set; } | ||||||||||||||
|
|
||||||||||||||
| /// <summary> | ||||||||||||||
| /// 页面初始化回调方法 | ||||||||||||||
| /// </summary> | ||||||||||||||
|
|
@@ -48,4 +58,9 @@ public class PdfReaderOptions | |||||||||||||
| /// 页面初始化回调方法 | ||||||||||||||
| /// </summary> | ||||||||||||||
| public Func<uint, Task>? OnPageChangedAsync { get; set; } | ||||||||||||||
|
|
||||||||||||||
| /// <summary> | ||||||||||||||
| /// 设置双页单视图模式回调方法 | ||||||||||||||
| /// </summary> | ||||||||||||||
| public Func<bool, Task>? OnTwoPagesOneViewAsync { get; set; } | ||||||||||||||
|
Comment on lines
+63
to
+65
|
||||||||||||||
| /// 设置双页单视图模式回调方法 | |
| /// </summary> | |
| public Func<bool, Task>? OnTwoPagesOneViewAsync { get; set; } | |
| /// 双页单视图模式变更回调方法 | |
| /// </summary> | |
| public Func<bool, Task>? OnTwoPagesOnViewChangedAsync { get; set; } |
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.
[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.