PDF Viewer - Search results don't work after rotating pdf

pdf/pdfium/pdfium_engine.cc

Index: pdf/pdfium/pdfium_engine.cc
diff --git a/pdf/pdfium/pdfium_engine.cc b/pdf/pdfium/pdfium_engine.cc
index e6713271139afb89d816956c6a3d07bbd1b41917..da3931dc9c933c37587f9e5caa65934e5e589671 100644
--- a/pdf/pdfium/pdfium_engine.cc
+++ b/pdf/pdfium/pdfium_engine.cc
@@ -1629,6 +1629,13 @@ void PDFiumEngine::StartFind(const char* text, bool case_sensitive) {
   if (end_of_search) {
     // Send the final notification.
     client_->NotifyNumberOfFindResultsChanged(find_results_.size(), true);
+
+    // Restore find result state when find is started after rotation.
+    if (find_result_state_.find_index_ != -1) {

[raymes, 2014/09/24 03:57:50]

It doesn't look like we need a new struct, it looks like we can just store the
index to resume the find at?

// When searching is complete, resume finding at a particular index.
if (resume_find_index_ != -1) {
  current_find_index_ = resume_find_index_ - 1;
  SelectFindResult(true);
  resume_find_index_ = -1;
}

[Nikhil, 2014/09/29 08:22:11]

Thanks, I've modified the code accordingly. Please see.

Also, I wanted to remove the call to SelectFindResult() to match Adobe Reader's
behavior but it looks like it is required. The way find operation is implemented
is that whenever we start a find, the first index gets highlighted. This
behavior is not desirable in the given use case and so I maintained the call to
SelectFindResult().

The call to SelectFindResult() causes a slight jitter on some pages, depending
on the content, because of the combination of - 

[*] Rotation
[*] First index is highlighted
[*] Desired index is highlighted

Please take a look and let me know WDYT.

+      current_find_index_ = find_result_state_.find_index_ - 1;
+      SelectFindResult(true);
+      find_result_state_ = FindResultState();
+    }
   } else {
     pp::CompletionCallback callback =
         find_factory_.NewCallback(&PDFiumEngine::ContinueFind);
@@ -1832,18 +1839,31 @@ void PDFiumEngine::ZoomUpdated(double new_zoom_level) {
 
 void PDFiumEngine::RotateClockwise() {
   current_rotation_ = (current_rotation_ + 1) % 4;
+  find_result_state_ = FindResultState(current_find_text_, current_find_index_);
+
   InvalidateAllPages();
+
+  // Restore find state if needed.

[raymes, 2014/09/24 03:57:50]

// Store the current find index so that we can resume finding at that 
// particular index after we have recomputed the find results.

[Nikhil, 2014/09/29 08:22:11]

Done.

+  if (!find_result_state_.find_text_.empty())
+    StartFind(find_result_state_.find_text_.c_str(), false);

[raymes, 2014/09/22 05:16:45]

I don't fully understand the find stuff but can we not modify StartFind and
instead to change this code to do:

StartFind(...)
current_find_index_ = ...
SelectFindResult(true)

That way you don't need to add a new member variable or class.

[Nikhil, 2014/09/23 12:18:08]

The problem that I faced earlier with this code flow was that StartFind() is
async and it takes time to complete search. So, I needed a way to store find
index before which rotation happened and update current find index to it once
find operation completes. I introduced new class and member variable for this
purpose.

Updating current find index (once find finishes) to find index prior to rotation
is required to maintain consistency. For example :

[*] Search for text and go to 2nd search item.
[*] Rotate pdf document.
[*] Sarch for previous/next item.
[*] Now we should go to 1st/3rd search item.

Adobe Reader follows the above behavior. 

Please let me know if you see foresee issues with this approach or if there
could be a better way to achieve this.

A few links that might be useful -

[*] Start find :
https://code.google.com/p/chromium/codesearch#chromium/src/pdf/pdfium/pdfium_...

[*] Update find results :
https://code.google.com/p/chromium/codesearch#chromium/src/pdf/pdfium/pdfium_...

[*] End of find :
https://code.google.com/p/chromium/codesearch#chromium/src/pdf/pdfium/pdfium_...

[raymes, 2014/09/24 03:57:50]

Ah I see how StartFind is asynchronous now, thanks for explaining.

[Nikhil, 2014/09/29 08:22:11]

Acknowledged.

 }
 
 void PDFiumEngine::RotateCounterclockwise() {
   current_rotation_ = (current_rotation_ - 1) % 4;
+  find_result_state_ = FindResultState(current_find_text_, current_find_index_);
+
   InvalidateAllPages();
+
+  // Restore find state if needed.
+  if (!find_result_state_.find_text_.empty())
+    StartFind(find_result_state_.find_text_.c_str(), false);
 }
 
 void PDFiumEngine::InvalidateAllPages() {
   selection_.clear();
   find_results_.clear();

[raymes, 2014/09/24 03:57:50]

These two lines and UpdateTickMarks() are already inside StopFind(). 

[Nikhil, 2014/09/29 08:22:11]

Done.

 
+  StopFind();

[raymes, 2014/09/24 03:57:50]

You might want to move this after CancelPaints() to ensure that it doesn't
interfere with it.

[Nikhil, 2014/09/29 08:22:11]

Done.

   CancelPaints();
   LoadPageInfo(true);
   UpdateTickMarks();