Cleanup some CPDFSDK_PageView annotation code.

fpdfsdk/cba_annotiterator.cpp

 // Copyright 2016 PDFium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
 
 #include "fpdfsdk/cba_annotiterator.h"
 
 #include "core/fpdfapi/page/cpdf_page.h"
 #include "fpdfsdk/cpdfsdk_annot.h"
@@ skipping 49 matching lines @@
 CPDFSDK_Annot* CBA_AnnotIterator::GetPrevAnnot(CPDFSDK_Annot* pAnnot) {
   auto iter = std::find(m_Annots.begin(), m_Annots.end(), pAnnot);
   if (iter == m_Annots.end())
     return nullptr;
   if (iter == m_Annots.begin())
     iter = m_Annots.end();
   return *(--iter);
 }
 
 void CBA_AnnotIterator::GenerateResults() {
-  switch (m_eTabOrder) {
-    case STRUCTURE: {
-      for (size_t i = 0; i < m_pPageView->CountAnnots(); ++i) {
-        CPDFSDK_Annot* pAnnot = m_pPageView->GetAnnot(i);
-        if (pAnnot->GetAnnotSubtype() == m_nAnnotSubtype &&
-            !pAnnot->IsSignatureWidget())
-          m_Annots.push_back(pAnnot);
+  if (m_eTabOrder == STRUCTURE) {

[Tom Sepez, 2016/10/05 17:08:28]

Dunno, I kinda like the switch and pulling the common stuff into helper methods.
 There's still a lot of duplication even after the refactor.

[dsinclair, 2016/10/05 17:52:51]

Done.

+    for (auto pAnnot : m_pPageView->GetAnnotList()) {
+      if (pAnnot->GetAnnotSubtype() == m_nAnnotSubtype &&
+          !pAnnot->IsSignatureWidget()) {
+        m_Annots.push_back(pAnnot);
       }
-      break;
     }
-    case ROW: {
-      std::vector<CPDFSDK_Annot*> sa;
-      for (size_t i = 0; i < m_pPageView->CountAnnots(); ++i) {
-        CPDFSDK_Annot* pAnnot = m_pPageView->GetAnnot(i);
-        if (pAnnot->GetAnnotSubtype() == m_nAnnotSubtype &&
-            !pAnnot->IsSignatureWidget())
-          sa.push_back(pAnnot);
-      }
+    return;
+  }
 
-      std::sort(sa.begin(), sa.end(), CompareByLeftAscending);
-      while (!sa.empty()) {
-        int nLeftTopIndex = -1;
-        FX_FLOAT fTop = 0.0f;
-        for (int i = sa.size() - 1; i >= 0; i--) {
-          CFX_FloatRect rcAnnot = GetAnnotRect(sa[i]);
-          if (rcAnnot.top > fTop) {
-            nLeftTopIndex = i;
-            fTop = rcAnnot.top;
-          }
-        }
-        if (nLeftTopIndex >= 0) {
-          CPDFSDK_Annot* pLeftTopAnnot = sa[nLeftTopIndex];
-          CFX_FloatRect rcLeftTop = GetAnnotRect(pLeftTopAnnot);
-          m_Annots.push_back(pLeftTopAnnot);
-          sa.erase(sa.begin() + nLeftTopIndex);
+  std::vector<CPDFSDK_Annot*> sa;

[dsinclair, 2016/10/05 14:06:47]

This chunk of code was the same between ROW and COLUMN so wanted to factor it
out.

[Tom Sepez, 2016/10/05 17:08:28]

Can we have a helper method like

void CBA_AnnotIter::CollectAnnots(std::vector<CPDFDSK_Annot*>* pArray {

  for (auto pAnnot : m_pPageView->GetAnnotList()) {
    if (pAnnot->GetAnnotSubtype() == m_nAnnotSubtype &&
        !pAnnot->IsSignatureWidget()) {
      pArray->push_back(pAnnot);
    }
  }
}

then line 71 is
  CollectAnnots(&m_Annots);
  return;

and the code here and for the other case statement is
  CollectAnnots(&sa);

[dsinclair, 2016/10/05 17:52:51]

Done.

+  for (auto pAnnot : m_pPageView->GetAnnotList()) {
+    if (pAnnot->GetAnnotSubtype() == m_nAnnotSubtype &&
+        !pAnnot->IsSignatureWidget()) {
+      sa.push_back(pAnnot);
+    }
+  }
+  if (sa.empty())
+    return;
 
-          std::vector<int> aSelect;
-          for (size_t i = 0; i < sa.size(); ++i) {
-            CFX_FloatRect rcAnnot = GetAnnotRect(sa[i]);
-            FX_FLOAT fCenterY = (rcAnnot.top + rcAnnot.bottom) / 2.0f;
-            if (fCenterY > rcLeftTop.bottom && fCenterY < rcLeftTop.top)
-              aSelect.push_back(i);
-          }
-          for (size_t i = 0; i < aSelect.size(); ++i)
-            m_Annots.push_back(sa[aSelect[i]]);
-
-          for (int i = aSelect.size() - 1; i >= 0; --i)
-            sa.erase(sa.begin() + aSelect[i]);
+  if (m_eTabOrder == ROW) {
+    std::sort(sa.begin(), sa.end(), CompareByLeftAscending);
+    while (!sa.empty()) {
+      int nLeftTopIndex = -1;

[Tom Sepez, 2016/10/05 17:08:28]

Maybe code from 93-100 becomes a function GetLeftTopIndex(const
std::vector<CPDFDSK_Annot*>& sa) ?

[dsinclair, 2016/10/05 17:52:51]

The actual comparisons done at ~97 are different between the two blocks.

[Tom Sepez, 2016/10/05 17:59:52]

Ah, ok.  nevermind.

+      FX_FLOAT fTop = 0.0f;
+      for (int i = sa.size() - 1; i >= 0; i--) {
+        CFX_FloatRect rcAnnot = GetAnnotRect(sa[i]);
+        if (rcAnnot.top > fTop) {
+          nLeftTopIndex = i;
+          fTop = rcAnnot.top;
         }
       }
-      break;
+      if (nLeftTopIndex >= 0) {
+        CPDFSDK_Annot* pLeftTopAnnot = sa[nLeftTopIndex];
+        CFX_FloatRect rcLeftTop = GetAnnotRect(pLeftTopAnnot);
+        m_Annots.push_back(pLeftTopAnnot);
+        sa.erase(sa.begin() + nLeftTopIndex);
+
+        std::vector<int> aSelect;
+        for (size_t i = 0; i < sa.size(); ++i) {
+          CFX_FloatRect rcAnnot = GetAnnotRect(sa[i]);
+          FX_FLOAT fCenterY = (rcAnnot.top + rcAnnot.bottom) / 2.0f;
+          if (fCenterY > rcLeftTop.bottom && fCenterY < rcLeftTop.top)
+            aSelect.push_back(i);
+        }
+        for (size_t i = 0; i < aSelect.size(); ++i)
+          m_Annots.push_back(sa[aSelect[i]]);
+
+        for (int i = aSelect.size() - 1; i >= 0; --i)
+          sa.erase(sa.begin() + aSelect[i]);
+      }
     }
-    case COLUMN: {
-      std::vector<CPDFSDK_Annot*> sa;
-      for (size_t i = 0; i < m_pPageView->CountAnnots(); ++i) {
-        CPDFSDK_Annot* pAnnot = m_pPageView->GetAnnot(i);
-        if (pAnnot->GetAnnotSubtype() == m_nAnnotSubtype &&
-            !pAnnot->IsSignatureWidget())
-          sa.push_back(pAnnot);
+    return;
+  }
+
+  std::sort(sa.begin(), sa.end(), CompareByTopDescending);
+  while (!sa.empty()) {
+    int nLeftTopIndex = -1;
+    FX_FLOAT fLeft = -1.0f;
+    for (int i = sa.size() - 1; i >= 0; --i) {
+      CFX_FloatRect rcAnnot = GetAnnotRect(sa[i]);
+      if (fLeft < 0) {
+        nLeftTopIndex = 0;
+        fLeft = rcAnnot.left;
+      } else if (rcAnnot.left < fLeft) {
+        nLeftTopIndex = i;
+        fLeft = rcAnnot.left;
       }
+    }
 
-      std::sort(sa.begin(), sa.end(), CompareByTopDescending);
-      while (!sa.empty()) {
-        int nLeftTopIndex = -1;
-        FX_FLOAT fLeft = -1.0f;
-        for (int i = sa.size() - 1; i >= 0; --i) {
-          CFX_FloatRect rcAnnot = GetAnnotRect(sa[i]);
-          if (fLeft < 0) {
-            nLeftTopIndex = 0;
-            fLeft = rcAnnot.left;
-          } else if (rcAnnot.left < fLeft) {
-            nLeftTopIndex = i;
-            fLeft = rcAnnot.left;
-          }
-        }
+    if (nLeftTopIndex >= 0) {
+      CPDFSDK_Annot* pLeftTopAnnot = sa[nLeftTopIndex];
+      CFX_FloatRect rcLeftTop = GetAnnotRect(pLeftTopAnnot);
+      m_Annots.push_back(pLeftTopAnnot);
+      sa.erase(sa.begin() + nLeftTopIndex);
 
-        if (nLeftTopIndex >= 0) {
-          CPDFSDK_Annot* pLeftTopAnnot = sa[nLeftTopIndex];
-          CFX_FloatRect rcLeftTop = GetAnnotRect(pLeftTopAnnot);
-          m_Annots.push_back(pLeftTopAnnot);
-          sa.erase(sa.begin() + nLeftTopIndex);
+      std::vector<int> aSelect;
+      for (size_t i = 0; i < sa.size(); ++i) {
+        CFX_FloatRect rcAnnot = GetAnnotRect(sa[i]);
+        FX_FLOAT fCenterX = (rcAnnot.left + rcAnnot.right) / 2.0f;
+        if (fCenterX > rcLeftTop.left && fCenterX < rcLeftTop.right)
+          aSelect.push_back(i);
+      }
+      for (size_t i = 0; i < aSelect.size(); ++i)
+        m_Annots.push_back(sa[aSelect[i]]);
 
-          std::vector<int> aSelect;
-          for (size_t i = 0; i < sa.size(); ++i) {
-            CFX_FloatRect rcAnnot = GetAnnotRect(sa[i]);
-            FX_FLOAT fCenterX = (rcAnnot.left + rcAnnot.right) / 2.0f;
-            if (fCenterX > rcLeftTop.left && fCenterX < rcLeftTop.right)
-              aSelect.push_back(i);
-          }
-          for (size_t i = 0; i < aSelect.size(); ++i)
-            m_Annots.push_back(sa[aSelect[i]]);
-
-          for (int i = aSelect.size() - 1; i >= 0; --i)
-            sa.erase(sa.begin() + aSelect[i]);
-        }
-      }
-      break;
+      for (int i = aSelect.size() - 1; i >= 0; --i)
+        sa.erase(sa.begin() + aSelect[i]);
     }
   }
 }
 
 CFX_FloatRect CBA_AnnotIterator::GetAnnotRect(const CPDFSDK_Annot* pAnnot) {
   return pAnnot->GetPDFAnnot()->GetRect();
 }