#1611 Inline "Find" bar (Cmd+F) falsely reports "Not found" while the sidebar full-text search is running

[Alex Jpn]

Skim has two independent search features sharing the same PDFDocument: the inline find bar (SKFindController, synchronous findString:fromSelection:withOptions:) and the sidebar full-text search (SKMainWindowController's search: action, asynchronous beginFindString:/beginFindStrings:).
The inline bar's handler unconditionally bails out with a beep and a "Not found" message whenever the sidebar search is still in flight ([pdfDoc isFinding] == YES), regardless of whether the search term actually exists in the document.

Location: SKMainWindowController.m, -findString:forward:, lines 1779–1784.

• (BOOL)findString:(NSString )string forward:(BOOL)forward {
  PDFDocument pdfDoc = [pdfView document];
  if ([pdfDoc isFinding]) {
  NSBeep();
  return NO;
  }

Repro:

• Open a large, multi-page PDF.
• Start typing a search term in the sidebar search field (this triggers beginFindString:/beginFindStrings:, which restarts on every keystroke — see SKMainWindowController_Actions.m:829-868).
• While the sidebar search is still running (or immediately after finishing typing, before the async search has fully drained — documentDidEndPageFind: only refreshes the visible result count every 50 pages, so the UI can look "done" before isFinding actually clears), open the inline find bar (Cmd+F) and search for the same or another term that is present in the document.

• Observe: instant beep + "Not found," even though the term exists and even on retry, until the sidebar's async search fully completes.

  
  Notes: isFinding is documented by PDFKit (PDFDocument.h) only in relation to the async beginFindString:/beginFindStrings: API family. Nothing in Apple's documentation states that the synchronous findString:fromSelection:withOptions: method (used by the inline bar) is unsafe to call while an async search is in progress on the same document — both are read-only text-search operations. The two features have no actual functional dependency; gating one on the other's busy flag appears to be defensive reuse of a shared boolean rather than a documented requirement.

Suggested fix: Remove the isFinding gate from the inline find path so it no longer depends on the sidebar's search state.

Patch

--- a/SKMainWindowController.m
+++ b/SKMainWindowController.m
@@ -1777,11 +1777,6 @@

• (BOOL)findString:(NSString )string forward:(BOOL)forward {
  PDFDocument pdfDoc = [pdfView document];
• if ([pdfDoc isFinding]) {
• NSBeep();
• return NO;
• }

• if ([self hasOverview]) {

[Christiaan Hofman]

The two find/search functions cannot be used at the same time, as they will otherwise interfere with each other, leading to wrong results.

[Christiaan Hofman]

BTW, the fact that Apple does not document any dependency of the two find methods does not mean it does not exists (there is lots of behavior in Apple's frameworks that is not or badly documented). A lack of documentation on interdependence is not a documentation that there is no interdependence. In practice, the most definitely is a dependency, and they cannot be used at the same time in practice. It may perhaps be safe to try at the moment, but it most definitely was not in older OS versions. It used to be that match callbacks were also send for synchronous find, which cannot be distinguished from async find matches, and therefore pollute our results. (As Apple never documented it, they also never said when that may have changed). ATM, what I see is that trying any synchronous find during an async find (the way you suggest) simply returns no match, exactly as what is happening now. The bottom line is really that precisely because Apple does not document anything, we cannot know how it behaves, in particular we cannot know whether things may go wrong. So we have to code defensively, especially because it is a fact that it has been problematic in the past, and there is no reason why they would go back to that situation (they have reintroduced bugs many times, especially in PDFKit).

[Alex Jpn]

Thanks for the comprehensive and very reasonable feedback.
Understood that concurrent searches may be risky.

Let me describe a scenario I experienced, and a possible alternative approach in case you are prepared to consider it.
I frequently work with large technical PDFs circa 100 MB, 10000 pages, mixture of text, equations, figures, etc. A full async search (left pane) for a term might take 30 secs to complete in some cases. In the mean time, the user might try using the right-side sync Find to search for some other (possibly related) term. (At least for me, this is not an edge case...) The "Not found" text might lead the user to think no instances of that term exist in the document, which can be misleading.
It's not very obvious (at least without scrolling down the left pane) that the async search is still ongoing and, even if it were to made more obvious, a regular user could not be expected to realize a right-side sync Find result during that period would be erroneous.

Given that, an alternative suggestion is that, if a sync Find is invoked while async Search is active, the sync Find is held until the async search completes, with message "Please wait..." until the Find starts. That way, there is no erroneous "Not found", and user doesn't need to repeat the action.
A possible patch, which I tested locally and seemed to work, is attached.

[Christiaan Hofman]

We may display something different from Not Found in this case. But I don't like delaying the search, as a lot of action can happen in the meantime (e.g. removing or adding some UI, finding multiple times, etc.) And this can lead to weird or inconsistent states. For now, i disable the find next/previous button as well as the find next/previous actions in the menu when a search is in progress. This indicates that this action is not available.

[Alex Jpn]

Thanks, that's reasonable. I just tested with latest upstream (up to r16396) and the disablement (and reenablement) of Find UI while async search is ongoing works well for me.

[Alex Jpn]

Actually it seems there is one residual case where the inline find bar still shows enabled during an in-progress search: the find bar's controls (SKFindController's findField/navigationButton) are only disabled in documentDidBeginDocumentFind:. But that controller and its view/outlets are lazily created — if the find bar hasn't been opened yet, documentDidBeginDocumentFind: disables a nil findController (no-op). If the user then opens the find bar for the first time while a search is already running, the freshly-loaded field/button default to enabled, since nothing else syncs their state to isFinding.
Possible fix: also sync state in showFindBar itself, each time it runs.

Index: SKMainWindowController.m
===================================================================
--- SKMainWindowController.m    (revision 16400)
+++ SKMainWindowController.m    (working copy)
@@ -1878,7 +1878,15 @@

     NSView *findBar = [findController view];
     NSTextField *findField = [findController findField];

-    
+
+    // The find bar's controls are only disabled in response to the
+    // document's find-begin/end notifications; if it's shown for the first
+    // time (or re-shown) while a search is already in progress, it would
+    // otherwise come up fully enabled.
+    BOOL isFinding = [[pdfView document] isFinding];
+    [[findController navigationButton] setEnabled:(isFinding == NO)];
+    [findField setEnabled:(isFinding == NO)];
+
     if ([findBar window]) {
         [findField selectText:nil];
     } else if (mwcFlags.isAnimatingFindBar == 0) {

[Alex Jpn]

Hi, thanks for preparing r16401.
That said, it seems the disable calls in showFindBar run right after alloc/init, before [findController view] is ever accessed — so navigationButton/findField are still nil (their IBOutlets only connect once the nib loads via that .view access) and the setEnabled:NO calls are no-ops. Moving that isFinding check to just after [findController view]/[findController findField] are fetched seems to fix it..