Conversation
- Cached PageData, but PDFForm not tested.
This issue occurs when you zoom too far and only half a page or less appears on the screen.
# Conflicts: # PdfiumViewer/NativeMethods.Pdfium.cs # PdfiumViewer/PdfFile.cs
… page moved suddenly when right-clicking.
…d number of characters.
Build tool changed. Visual studio 2022 / .NET Framework 4.8.
…e comparison page.
…during initialization of controls including PdfViewer.
…ion for a moment.
…g the last page of a long PDF.
…e to rounding error.
…e to rounding error.
…e to rounding error.
Get the start page and end page of the selected bookmark node. Checks if the specified page has any markers.
… do not have a bookmark assigned.
…d 90 degrees and 270 degrees.
Add a note indicating this is a custom version for PDFComp.
…he current page number.
Summary of ChangesHello @username77, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers substantial upgrades to the PdfiumViewer library, focusing on improving user interaction and refining its internal architecture. Users will benefit from a more robust text selection mode with visual feedback, a more stable and precise zooming experience, and a comprehensive set of tools for managing PDF bookmarks. The project also received an update to target .NET Framework 4.8, and its internal page data caching mechanism was optimized for better overall performance and flexibility. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements and refactorings across the PdfiumViewer library, primarily focusing on improved text selection, bookmark management, and rendering accuracy. Key changes include the addition of a 'Text Mode' button and functionality, a new PdfCursorMode enum (a breaking change) with a 'Bounds' option, and a CompareBounds property for defining comparison areas. The CustomScrollControl was refactored to use double-precision coordinates to prevent rounding errors during magnification and includes a robustness check for zero height. The page data caching mechanism in PdfFile.cs was updated to cache all pages, improving performance but potentially increasing memory usage, with corresponding updates to the dispose logic. The PdfRenderer component received extensive updates, including a ResetRenderer method for state management, PointToPdfRounded for precise coordinate conversion, and corrections to rotation logic. New drawing methods (DrawCompareBounds, DrawTextOutline, DrawTextSpans) were added for visual feedback, along with animated text highlighting using SetFlashTextSpans and CalcFlashTextAlpha. Bookmark functionality was greatly expanded with a new toolstrip, allowing for comprehensive control over bookmark tree expansion/collapse, selection based on current page, and page range calculation, including a clever EnsureVisibleWithoutRightScrolling method. Several breaking changes were noted, such as the renaming of PdfViewerCursorMode and the removal of GetTextRectangles from IPdfDocument, requiring careful migration. Additionally, build settings were updated to target .NET Framework 4.8 and strong-naming was disabled.
| public enum PdfCursorMode | ||
| { | ||
| Pan, | ||
| TextSelection, | ||
| Bounds | ||
| } |
There was a problem hiding this comment.
The renaming of PdfViewerCursorMode to PdfCursorMode and the addition of the Bounds enum value is a breaking change for any external code that references the old enum name. While Bounds adds new functionality, the rename requires careful migration. Ensure that all references to the old enum name are updated throughout the codebase and in any consuming projects.
| public PdfCursorMode CursorMode | ||
| { | ||
| get { return _cursorMode; } | ||
| set | ||
| { | ||
| _cursorMode = value; | ||
| MousePanningEnabled = _cursorMode == PdfViewerCursorMode.Pan; | ||
| MousePanningEnabled = _cursorMode == PdfCursorMode.Pan; |
There was a problem hiding this comment.
The change of CursorMode to use the new PdfCursorMode enum is a breaking change. While it allows for new cursor modes like Bounds, it will require updates in any code that previously referenced PdfViewerCursorMode. Ensure this transition is handled smoothly in documentation and examples.
| <SignAssembly>false</SignAssembly> | ||
| </PropertyGroup> | ||
| <PropertyGroup> | ||
| <AssemblyOriginatorKeyFile>Key.snk</AssemblyOriginatorKeyFile> | ||
| <AssemblyOriginatorKeyFile> | ||
| </AssemblyOriginatorKeyFile> |
There was a problem hiding this comment.
Changing SignAssembly to false and clearing AssemblyOriginatorKeyFile means the assembly will no longer be strong-named. This can affect compatibility with other strong-named assemblies or deployment scenarios. Ensure this change is intentional and its implications are understood.
| /// <param name="page">The page to get the rectangles from</param> | ||
| /// <returns>The rectangular areas occupied by a segment of text</returns> | ||
| List<PdfRectangle> GetTextRectangles(int page, int startIndex, int count); | ||
| } |
There was a problem hiding this comment.
The removal of the GetTextRectangles method from the IPdfDocument interface is a breaking change. Any external code that directly implements or consumes this interface and relies on this method will need to be updated. Ensure that this change is intentional and communicated to users of the library.
| if (preventPageRefresh == false) | ||
| { | ||
| _renderer.Page = ((PdfBookmark)e.Node.Tag).PageIndex; | ||
| } |
There was a problem hiding this comment.
The _bookmarks_AfterSelect event handler now correctly uses preventPageRefresh to avoid unnecessary page jumps when the TreeView's selection changes due to programmatic updates (e.g., during expand/collapse operations). This improves the responsiveness and predictability of the UI.
| private void _bookmarks_BeforeExpand(object sender, TreeViewCancelEventArgs e) | ||
| { | ||
| // AfterSelect event is fired when child node has focus. | ||
| preventPageRefresh = true; | ||
| } | ||
|
|
||
| private void _bookmarks_AfterExpand(object sender, TreeViewEventArgs e) | ||
| { | ||
| // AfterSelect event is fired when child node has focus. | ||
| preventPageRefresh = false; | ||
|
|
||
| // Select a bookmark for the renderer page. | ||
| bookmarkPage = -1; | ||
| SelectBookmarkForPage(_renderer.Page); | ||
| } | ||
|
|
||
| private void _bookmarks_BeforeCollapse(object sender, TreeViewCancelEventArgs e) | ||
| { | ||
| // AfterSelect event is fired when child node has focus. | ||
| preventPageRefresh = true; | ||
| } | ||
|
|
||
| private void _bookmarks_AfterCollapse(object sender, TreeViewEventArgs e) | ||
| { | ||
| // AfterSelect event is fired when child node has focus. | ||
| preventPageRefresh = false; | ||
| } |
There was a problem hiding this comment.
The _bookmarks_BeforeExpand, _bookmarks_AfterExpand, _bookmarks_BeforeCollapse, and _bookmarks_AfterCollapse event handlers are crucial for managing the preventPageRefresh flag. This ensures that page changes are only triggered by explicit user selection, not by the TreeView's internal expansion/collapse logic.
| private void toolStripButtonExpand_Click(object sender, EventArgs e) | ||
| { | ||
| // Expand Top-Level | ||
| bool visible = _showBookmarks && _document != null && _document.Bookmarks.Count > 0; | ||
|
|
||
| if (visible) | ||
| { | ||
| foreach (TreeNode node in _bookmarks.Nodes) | ||
| { | ||
| node.Expand(); | ||
| } | ||
|
|
||
| // Show selected nodes. | ||
| EnsureVisibleWithoutRightScrolling(_bookmarks.SelectedNode); | ||
| } | ||
| } | ||
|
|
||
| private void toolStripSplitButtonCollapse_ButtonClick(object sender, EventArgs e) | ||
| { | ||
| // Collapse Top-Level | ||
| bool visible = _showBookmarks && _document != null && _document.Bookmarks.Count > 0; | ||
|
|
||
| if (visible) | ||
| { | ||
| foreach (TreeNode node in _bookmarks.Nodes) | ||
| { | ||
| node.Collapse(); | ||
| } | ||
| // Show selected nodes. | ||
| EnsureVisibleWithoutRightScrolling(_bookmarks.SelectedNode); | ||
| } | ||
| } | ||
|
|
||
| private void expandAllToolStripMenuItem_Click(object sender, EventArgs e) | ||
| { | ||
| // Expand All | ||
| _bookmarks.ExpandAll(); | ||
| // Show selected nodes. | ||
| EnsureVisibleWithoutRightScrolling(_bookmarks.SelectedNode); | ||
| } | ||
|
|
||
| private void collapseAllToolStripMenuItem_Click(object sender, EventArgs e) | ||
| { | ||
| // Collapse All | ||
| CollapseChildrenNodes(_bookmarks.Nodes); | ||
| // Show selected nodes. | ||
| EnsureVisibleWithoutRightScrolling(_bookmarks.SelectedNode); | ||
| } | ||
|
|
||
| private void toolStripButtonExpandCurrent_Click(object sender, EventArgs e) | ||
| { | ||
| // Expand current bookmark | ||
| TreeNode validNode = null; | ||
|
|
||
| // Collapse all nodes initially | ||
| CollapseChildrenNodes(_bookmarks.Nodes); | ||
| GetPageNode(_renderer.Page, _bookmarks.Nodes, true, ref validNode); | ||
| ExpandParentNodes(validNode); | ||
|
|
||
| // Show selected nodes. | ||
| EnsureVisibleWithoutRightScrolling(_bookmarks.SelectedNode); | ||
| } | ||
|
|
||
| private void ExpandParentNodes(TreeNode node) | ||
| { | ||
| if (node != null) | ||
| { | ||
| node.Expand(); | ||
| if (node.Parent != null) | ||
| { | ||
| ExpandParentNodes(node.Parent); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void CollapseChildrenNodes(TreeNodeCollection nodes) | ||
| { | ||
| foreach (TreeNode node in nodes) | ||
| { | ||
| if (node != null) | ||
| { | ||
| if (node.Nodes != null) | ||
| { | ||
| CollapseChildrenNodes(node.Nodes); | ||
| } | ||
| node.Collapse(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void EnsureVisibleWithoutRightScrolling(TreeNode node) | ||
| { | ||
| // we do the standard call.. | ||
| node?.EnsureVisible(); | ||
|
|
||
| // ..and afterwards we scroll to the left again! | ||
| SendMessage(_bookmarks.Handle, WM_HSCROLL, SB_LEFT, 0); | ||
| } |
There was a problem hiding this comment.
The new toolstrip button event handlers (toolStripButtonExpand_Click, toolStripSplitButtonCollapse_ButtonClick, expandAllToolStripMenuItem_Click, collapseAllToolStripMenuItem_Click, toolStripButtonExpandCurrent_Click, toolStripButtonClose_Click) and their helper methods (ExpandParentNodes, CollapseChildrenNodes, EnsureVisibleWithoutRightScrolling) provide comprehensive control over bookmark tree manipulation. The EnsureVisibleWithoutRightScrolling is particularly clever for maintaining horizontal scroll position.
| y = minY; | ||
|
|
||
| SetDisplayRectLocation(x, y); | ||
| SetDisplayRectLocation(x, y, false); |
There was a problem hiding this comment.
Changing SetDisplayRectLocation to always pass false for preserveContents when setting the display rectangle size might have performance implications or visual effects. If preserveContents is intended to optimize redrawing, forcing false could lead to unnecessary full redraws. Consider if this is the desired behavior in all scenarios, or if preserveContents should be determined dynamically.
| if (fullClient.Height == 0) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Adding a check for fullClient.Height == 0 is a good defensive programming practice to prevent potential division-by-zero errors or other layout issues when the control has no height. This improves the robustness of the ApplyScrollbarChanges method.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a6075aabe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!MousePanningEnabled || e.Button != MouseButtons.Left || !_canPan) | ||
| return; | ||
|
|
||
| Capture = true; | ||
| _capture = true; | ||
| _dragStart = e.Location; |
There was a problem hiding this comment.
The drag state now only flips the private _capture flag and never calls Capture = true. If the user drags outside the control and releases the mouse button, WinForms will not deliver OnMouseUp back to this control, leaving _capture stuck true and causing panning on later mouse moves even with no button pressed. This shows up any time a drag exits the control bounds; consider re‑enabling mouse capture (or resetting on mouse leave) so the control always receives the button‑up event.
Useful? React with 👍 / 👎.
| int rect_count = NativeMethods.FPDFText_CountRects(pageData.TextPage, textSpan.Offset, textSpan.Length); | ||
|
|
||
| for (int i = 0; i < rect_count; i++) | ||
| { | ||
| NativeMethods.FPDFText_GetRect(pageData.TextPage, i, out var left, out var top, out var right, out var bottom); |
There was a problem hiding this comment.
Here FPDFText_CountRects and FPDFText_GetRect are invoked in separate, individually locked calls. PDFium’s GetRect uses internal state set by the last CountRects, so without a single shared lock another thread can interleave its own CountRects call and cause rectangles to be read from the wrong range or throw. This becomes visible under concurrent text‑highlight or search usage; use the existing FPDFText_GetRectangles helper or wrap both calls in one lock.
Useful? React with 👍 / 👎.
…m-binaries. # resolved: # PdfiumViewer/PdfFile.cs
BREAKING: Change FS_RECTF from class to struct
3 participants
No description provided.