Cleanup parts of CPDFSDK_AnnotIterator and CPDFSDK_PageView.

fpdfsdk/src/fsdk_annothandler.cpp

Index: fpdfsdk/src/fsdk_annothandler.cpp
diff --git a/fpdfsdk/src/fsdk_annothandler.cpp b/fpdfsdk/src/fsdk_annothandler.cpp
index bb999e1f08c420f768f0b7fa9935eac71794dd2c..c77261af1d840803ea1c484e3464ceed574a3d54 100644
--- a/fpdfsdk/src/fsdk_annothandler.cpp
+++ b/fpdfsdk/src/fsdk_annothandler.cpp
@@ -9,6 +9,16 @@
 #include "../include/formfiller/FFL_FormFiller.h"
 #include "../include/fsdk_annothandler.h"
 
+namespace {
+
+int LyOrderCompare(CPDFSDK_Annot* p1, CPDFSDK_Annot* p2) {
+  if (p1->GetLayoutOrder() == p2->GetLayoutOrder())

[Tom Sepez, 2015/10/21 19:32:59]

just return p1->GetLayouOrder() - p2->GetLayoutOrder() ?

[Lei Zhang, 2015/10/22 06:56:22]

Oh yes, it's 2 vs 5!

+    return 0;
+  return (p1->GetLayoutOrder() < p2->GetLayoutOrder()) ? -1 : 1;
+}
+
+}  //  namespace
+
 CPDFSDK_AnnotHandlerMgr::CPDFSDK_AnnotHandlerMgr(CPDFDoc_Environment* pApp) {
   m_pApp = pApp;
 
@@ -649,164 +659,70 @@ FX_BOOL CPDFSDK_BFAnnotHandler::HitTest(CPDFSDK_PageView* pPageView,
   return rect.Contains(point.x, point.y);
 }
 
-// CReader_AnnotIteratorEx
-
 CPDFSDK_AnnotIterator::CPDFSDK_AnnotIterator(CPDFSDK_PageView* pPageView,
-                                             FX_BOOL bReverse,
-                                             FX_BOOL bIgnoreTopmost /*=FALSE*/,
-                                             FX_BOOL bCircle /*=FALSE*/,
-                                             CFX_PtrArray* pList /*=NULL*/) {
-  ASSERT(pPageView);
-  m_bReverse = bReverse;
-  m_bIgnoreTopmost = bIgnoreTopmost;
-  m_bCircle = bCircle;
-  m_pIteratorAnnotList.RemoveAll();
-  InitIteratorAnnotList(pPageView, pList);
-}
-
-CPDFSDK_Annot* CPDFSDK_AnnotIterator::NextAnnot(const CPDFSDK_Annot* pCurrent) {
-  int index = -1;
-  int nCount = m_pIteratorAnnotList.GetSize();
-  if (pCurrent) {
-    for (int i = 0; i < nCount; i++) {

[Tom Sepez, 2015/10/21 19:32:58]

Yeah, lets make traversal O(n^2) just for grins.

[Lei Zhang, 2015/10/22 06:56:22]

But it's dead code, so that's O(0) right?

-      CPDFSDK_Annot* pReaderAnnot =
-          (CPDFSDK_Annot*)m_pIteratorAnnotList.GetAt(i);
-      if (pReaderAnnot == pCurrent) {
-        index = i;
-        break;
-      }
-    }
-  }
-  return NextAnnot(index);
-}
-CPDFSDK_Annot* CPDFSDK_AnnotIterator::PrevAnnot(const CPDFSDK_Annot* pCurrent) {
-  int index = -1;
-  int nCount = m_pIteratorAnnotList.GetSize();
-  if (pCurrent) {
-    for (int i = 0; i < nCount; i++) {
-      CPDFSDK_Annot* pReaderAnnot =
-          (CPDFSDK_Annot*)m_pIteratorAnnotList.GetAt(i);
-      if (pReaderAnnot == pCurrent) {
-        index = i;
-        break;
-      }
-    }
-  }
-  return PrevAnnot(index);
-}
-CPDFSDK_Annot* CPDFSDK_AnnotIterator::NextAnnot(int& index) {
-  int nCount = m_pIteratorAnnotList.GetSize();
-  if (nCount <= 0)
-    index = -1;
-  else {
-    if (index < 0) {
-      index = 0;
-    } else {
-      if (m_bCircle) {
-        index = (index < nCount - 1) ? (index + 1) : 0;
-      } else {
-        index = (index < nCount - 1) ? (index + 1) : -1;
-      }
-    }
-  }
-  return (index < 0) ? NULL : (CPDFSDK_Annot*)m_pIteratorAnnotList.GetAt(index);
-}
-
-CPDFSDK_Annot* CPDFSDK_AnnotIterator::PrevAnnot(int& index) {
-  int nCount = m_pIteratorAnnotList.GetSize();
-  if (nCount <= 0)
-    index = -1;
-  else {
-    if (index < 0) {
-      index = nCount - 1;
-    } else {
-      if (m_bCircle) {
-        index = (index > 0) ? (index - 1) : nCount - 1;
-      } else {
-        index = (index > 0) ? (index - 1) : -1;
-      }
+                                             bool bReverse)
+    : m_bReverse(bReverse) {
+  const std::vector<CPDFSDK_Annot*>& annots = pPageView->GetAnnotList();
+  for (auto it = annots.rbegin(); it != annots.rend(); ++it)

[Tom Sepez, 2015/10/21 19:32:58]

Do we care what order we traverse the annots if we are going to sort them
afterwards?

[Lei Zhang, 2015/10/22 06:56:22]

Apparently we do. Probably for z-ordering. If I flip it around, tests fail.

+    m_iteratorAnnotList.push_back(*it);
+  InsertSort();
+
+  CPDFSDK_Annot* pTopMostAnnot = pPageView->GetFocusAnnot();
+  if (!pTopMostAnnot)
+    return;
+
+  for (auto it = m_iteratorAnnotList.begin(); it != m_iteratorAnnotList.end();

[Tom Sepez, 2015/10/21 19:32:58]

can we use std::find or some such?

[Lei Zhang, 2015/10/22 06:56:22]

Done.

+       ++it) {
+    CPDFSDK_Annot* pReaderAnnot = *it;
+    if (pReaderAnnot == pTopMostAnnot) {
+      m_iteratorAnnotList.erase(it);
+      m_iteratorAnnotList.insert(m_iteratorAnnotList.begin(), pReaderAnnot);
+      break;
     }
   }
-  return (index < 0) ? NULL : (CPDFSDK_Annot*)m_pIteratorAnnotList.GetAt(index);
-}
-
-CPDFSDK_Annot* CPDFSDK_AnnotIterator::Next(const CPDFSDK_Annot* pCurrent) {
-  return (m_bReverse) ? PrevAnnot(pCurrent) : NextAnnot(pCurrent);
 }
 
-CPDFSDK_Annot* CPDFSDK_AnnotIterator::Prev(const CPDFSDK_Annot* pCurrent) {
-  return (m_bReverse) ? NextAnnot(pCurrent) : PrevAnnot(pCurrent);
+CPDFSDK_AnnotIterator::~CPDFSDK_AnnotIterator() {
 }
 
-CPDFSDK_Annot* CPDFSDK_AnnotIterator::Next(int& index) {
-  return (m_bReverse) ? PrevAnnot(index) : NextAnnot(index);
-}
+CPDFSDK_Annot* CPDFSDK_AnnotIterator::NextAnnot(size_t* index) {
+  const size_t oldIndex = *index;

[Tom Sepez, 2015/10/21 19:32:58]

there's got to be a simpler way to write this.

[Lei Zhang, 2015/10/22 06:56:22]

Done.

+  if (oldIndex >= m_iteratorAnnotList.size())
+    return nullptr;
 
-CPDFSDK_Annot* CPDFSDK_AnnotIterator::Prev(int& index) {
-  return (m_bReverse) ? NextAnnot(index) : PrevAnnot(index);
+  ++(*index);
+  return m_iteratorAnnotList[oldIndex];
 }
 
-void CPDFSDK_AnnotIterator::InsertSort(CFX_PtrArray& arrayList,
-                                       AI_COMPARE pCompare) {
-  for (int i = 1; i < arrayList.GetSize(); i++) {
-    if (pCompare((CPDFSDK_Annot*)(arrayList[i]),
-                 (CPDFSDK_Annot*)(arrayList[i - 1])) < 0) {
-      int j = i - 1;
-      CPDFSDK_Annot* pTemp = (CPDFSDK_Annot*)arrayList[i];
-
-      do {
-        arrayList[j + 1] = arrayList[j];
-      } while (--j >= 0 && pCompare(pTemp, (CPDFSDK_Annot*)arrayList[j]) < 0);
+CPDFSDK_Annot* CPDFSDK_AnnotIterator::PrevAnnot(size_t* index) {
+  if (m_iteratorAnnotList.empty() || *index >= m_iteratorAnnotList.size())

[Tom Sepez, 2015/10/21 19:32:58]

Wouldn't the empty() check be redundant if size() == 0 for empty lists?

[Lei Zhang, 2015/10/22 06:56:22]

Done.

+    return nullptr;
 
-      arrayList[j + 1] = pTemp;
-    }
-  }
+  const size_t oldIndex = m_iteratorAnnotList.size() - *index - 1;
+  ++(*index);
+  return m_iteratorAnnotList[oldIndex];
 }
 
-int LyOrderCompare(CPDFSDK_Annot* p1, CPDFSDK_Annot* p2) {
-  if (p1->GetLayoutOrder() < p2->GetLayoutOrder())
-    return -1;
-  if (p1->GetLayoutOrder() > p2->GetLayoutOrder())
-    return 1;
-  return 0;
+CPDFSDK_Annot* CPDFSDK_AnnotIterator::Next(size_t* index) {
+  return m_bReverse ? PrevAnnot(index) : NextAnnot(index);
 }
 
-FX_BOOL CPDFSDK_AnnotIterator::InitIteratorAnnotList(
-    CPDFSDK_PageView* pPageView,
-    CFX_PtrArray* pAnnotList) {
-  ASSERT(pPageView);
-
-  if (pAnnotList == NULL) {
-    pAnnotList = pPageView->GetAnnotList();
-  }
-
-  m_pIteratorAnnotList.RemoveAll();
-  if (!pAnnotList)
-    return FALSE;
-
-  CPDFSDK_Annot* pTopMostAnnot =
-      (m_bIgnoreTopmost) ? NULL : pPageView->GetFocusAnnot();
-
-  int nCount = pAnnotList->GetSize();
+CPDFSDK_Annot* CPDFSDK_AnnotIterator::Prev(size_t* index) {
+  return m_bReverse ? NextAnnot(index) : PrevAnnot(index);
+}
 
-  for (int i = nCount - 1; i >= 0; i--) {
-    CPDFSDK_Annot* pReaderAnnot = (CPDFSDK_Annot*)pAnnotList->GetAt(i);
-    m_pIteratorAnnotList.Add(pReaderAnnot);
-  }
+void CPDFSDK_AnnotIterator::InsertSort() {

[Tom Sepez, 2015/10/21 19:32:58]

WHY???

[Lei Zhang, 2015/10/22 06:56:22]

I was lazy and didn't really want to deal with it. We need a stable sort.

+  for (size_t i = 1; i < m_iteratorAnnotList.size(); ++i) {
+    if (LyOrderCompare((CPDFSDK_Annot*)(m_iteratorAnnotList[i]),
+                       (CPDFSDK_Annot*)(m_iteratorAnnotList[i - 1])) < 0) {
+      int j = i - 1;
+      CPDFSDK_Annot* pTemp = m_iteratorAnnotList[i];
 
-  InsertSort(m_pIteratorAnnotList, &LyOrderCompare);
+      do {
+        m_iteratorAnnotList[j + 1] = m_iteratorAnnotList[j];
+      } while (--j >= 0 && LyOrderCompare(pTemp, m_iteratorAnnotList[j]) < 0);
 
-  if (pTopMostAnnot) {
-    for (int i = 0; i < nCount; i++) {
-      CPDFSDK_Annot* pReaderAnnot =
-          (CPDFSDK_Annot*)m_pIteratorAnnotList.GetAt(i);
-      if (pReaderAnnot == pTopMostAnnot) {
-        m_pIteratorAnnotList.RemoveAt(i);
-        m_pIteratorAnnotList.InsertAt(0, pReaderAnnot);
-        break;
-      }
+      m_iteratorAnnotList[j + 1] = pTemp;
     }
   }
-
-  return TRUE;
 }