Factor out the duplicated logic in Field::SetDisplay

fpdfsdk/javascript/Field.cpp

 // Copyright 2014 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/javascript/Field.h"
 
 #include <algorithm>
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "core/fpdfapi/fpdf_font/include/cpdf_font.h"
 #include "core/fpdfapi/fpdf_page/include/cpdf_page.h"
 #include "core/fpdfdoc/include/cpdf_interform.h"
 #include "fpdfsdk/include/fsdk_mgr.h"
 #include "fpdfsdk/javascript/Document.h"
 #include "fpdfsdk/javascript/Icon.h"
 #include "fpdfsdk/javascript/JS_Define.h"
 #include "fpdfsdk/javascript/JS_EventHandler.h"
 #include "fpdfsdk/javascript/JS_Object.h"
 #include "fpdfsdk/javascript/JS_Value.h"
 #include "fpdfsdk/javascript/PublicMethods.h"
 #include "fpdfsdk/javascript/cjs_context.h"
 #include "fpdfsdk/javascript/cjs_runtime.h"
 #include "fpdfsdk/javascript/color.h"
 
+static bool SetWidgetDisplayStatus(CPDFSDK_Widget* pWidget, int value) {

[Tom Sepez, 2016/08/17 17:33:31]

nit: prefer

namespace {

bool SetW...

}  // Namespace

[tonikitoo, 2016/08/17 17:51:53]

Done.

+  if (!pWidget)
+    return false;
+
+  uint32_t dwFlag = pWidget->GetFlags();
+  switch (value) {
+    case 0:
+      dwFlag &= (~ANNOTFLAG_INVISIBLE);

[Tom Sepez, 2016/08/17 17:33:31]

nit: the () aren't strictly necessary here.

[tonikitoo, 2016/08/17 17:51:53]

Done.

+      dwFlag &= (~ANNOTFLAG_HIDDEN);
+      dwFlag &= (~ANNOTFLAG_NOVIEW);
+      dwFlag |= ANNOTFLAG_PRINT;
+      break;
+    case 1:
+      dwFlag &= (~ANNOTFLAG_INVISIBLE);
+      dwFlag &= (~ANNOTFLAG_NOVIEW);
+      dwFlag |= (ANNOTFLAG_HIDDEN | ANNOTFLAG_PRINT);
+      break;
+    case 2:
+      dwFlag &= (~ANNOTFLAG_INVISIBLE);
+      dwFlag &= (~ANNOTFLAG_PRINT);
+      dwFlag &= (~ANNOTFLAG_HIDDEN);
+      dwFlag &= (~ANNOTFLAG_NOVIEW);
+      break;
+    case 3:
+      dwFlag |= ANNOTFLAG_NOVIEW;
+      dwFlag |= ANNOTFLAG_PRINT;
+      dwFlag &= (~ANNOTFLAG_HIDDEN);
+      break;
+  }
+
+  if (dwFlag != pWidget->GetFlags()) {
+    pWidget->SetFlags(dwFlag);
+    return true;
+  }
+
+  return false;
+}
+
 BEGIN_JS_STATIC_CONST(CJS_Field)
 END_JS_STATIC_CONST()
 
 BEGIN_JS_STATIC_PROP(CJS_Field)
 JS_STATIC_PROP_ENTRY(alignment)
 JS_STATIC_PROP_ENTRY(borderStyle)
 JS_STATIC_PROP_ENTRY(buttonAlignX)
 JS_STATIC_PROP_ENTRY(buttonAlignY)
 JS_STATIC_PROP_ENTRY(buttonFitBounds)
 JS_STATIC_PROP_ENTRY(buttonPosition)
@@ skipping 1192 matching lines @@
 }
 
 void Field::SetDisplay(CPDFSDK_Document* pDocument,
                        const CFX_WideString& swFieldName,
                        int nControlIndex,
                        int number) {
   CPDFSDK_InterForm* pInterForm = pDocument->GetInterForm();
   std::vector<CPDF_FormField*> FieldArray =
       GetFormFields(pDocument, swFieldName);
   for (CPDF_FormField* pFormField : FieldArray) {
     if (nControlIndex < 0) {

[Tom Sepez, 2016/08/17 17:33:31]

Optional:  I didn't see nControlIndex being updated in this loop, maybe refactor
it outside and make two loops.  Or not.

       FX_BOOL bSet = FALSE;

[Tom Sepez, 2016/08/17 17:45:37]

nit: maybe call this bAnySet and make it a bool.

[tonikitoo, 2016/08/17 17:51:53]

Done.

       for (int i = 0, sz = pFormField->CountControls(); i < sz; ++i) {
         CPDF_FormControl* pFormControl = pFormField->GetControl(i);
         ASSERT(pFormControl);
 
-        if (CPDFSDK_Widget* pWidget =
-                pInterForm->GetWidget(pFormControl, true)) {
-          uint32_t dwFlag = pWidget->GetFlags();
-          switch (number) {
-            case 0:
-              dwFlag &= (~ANNOTFLAG_INVISIBLE);
-              dwFlag &= (~ANNOTFLAG_HIDDEN);
-              dwFlag &= (~ANNOTFLAG_NOVIEW);
-              dwFlag |= ANNOTFLAG_PRINT;
-              break;
-            case 1:
-              dwFlag &= (~ANNOTFLAG_INVISIBLE);
-              dwFlag &= (~ANNOTFLAG_NOVIEW);
-              dwFlag |= (ANNOTFLAG_HIDDEN | ANNOTFLAG_PRINT);
-              break;
-            case 2:
-              dwFlag &= (~ANNOTFLAG_INVISIBLE);
-              dwFlag &= (~ANNOTFLAG_PRINT);
-              dwFlag &= (~ANNOTFLAG_HIDDEN);
-              dwFlag &= (~ANNOTFLAG_NOVIEW);
-              break;
-            case 3:
-              dwFlag |= ANNOTFLAG_NOVIEW;
-              dwFlag |= ANNOTFLAG_PRINT;
-              dwFlag &= (~ANNOTFLAG_HIDDEN);
-              break;
-          }
-
-          if (dwFlag != pWidget->GetFlags()) {
-            pWidget->SetFlags(dwFlag);
-            bSet = TRUE;
-          }
-        }
+        CPDFSDK_Widget* pWidget = pInterForm->GetWidget(pFormControl, true);
+        bSet = SetWidgetDisplayStatus(pWidget, number);

[Tom Sepez, 2016/08/17 17:38:40]

actually, we don't want to loose  previous TRUE value, so we have to do
if (...)
  bSet = TRUE;

[tonikitoo, 2016/08/17 17:51:53]

Done.

       }
 
       if (bSet)
         UpdateFormField(pDocument, pFormField, TRUE, FALSE, TRUE);
     } else {
       if (nControlIndex >= pFormField->CountControls())
         return;
-      if (CPDF_FormControl* pFormControl =
-              pFormField->GetControl(nControlIndex)) {
-        if (CPDFSDK_Widget* pWidget =
-                pInterForm->GetWidget(pFormControl, true)) {
-          uint32_t dwFlag = pWidget->GetFlags();
-          switch (number) {
-            case 0:
-              dwFlag &= (~ANNOTFLAG_INVISIBLE);
-              dwFlag &= (~ANNOTFLAG_HIDDEN);
-              dwFlag &= (~ANNOTFLAG_NOVIEW);
-              dwFlag |= ANNOTFLAG_PRINT;
-              break;
-            case 1:
-              dwFlag &= (~ANNOTFLAG_INVISIBLE);
-              dwFlag &= (~ANNOTFLAG_NOVIEW);
-              dwFlag |= (ANNOTFLAG_HIDDEN | ANNOTFLAG_PRINT);
-              break;
-            case 2:
-              dwFlag &= (~ANNOTFLAG_INVISIBLE);
-              dwFlag &= (~ANNOTFLAG_PRINT);
-              dwFlag &= (~ANNOTFLAG_HIDDEN);
-              dwFlag &= (~ANNOTFLAG_NOVIEW);
-              break;
-            case 3:
-              dwFlag |= ANNOTFLAG_NOVIEW;
-              dwFlag |= ANNOTFLAG_PRINT;
-              dwFlag &= (~ANNOTFLAG_HIDDEN);
-              break;
-          }
-          if (dwFlag != pWidget->GetFlags()) {
-            pWidget->SetFlags(dwFlag);
-            UpdateFormControl(pDocument, pFormControl, TRUE, FALSE, TRUE);
-          }
-        }
-      }
+
+      CPDF_FormControl* pFormControl = pFormField->GetControl(nControlIndex);
+      if (!pFormControl)
+        return;
+
+      CPDFSDK_Widget* pWidget = pInterForm->GetWidget(pFormControl, true);
+      if (SetWidgetDisplayStatus(pWidget, number))
+        UpdateFormControl(pDocument, pFormControl, TRUE, FALSE, TRUE);
     }
   }
 }
 
 FX_BOOL Field::doc(IJS_Context* cc, CJS_PropValue& vp, CFX_WideString& sError) {
   if (!vp.IsGetting()) {
     return FALSE;
   }
   vp << m_pJSDoc->GetCJSDoc();
   return TRUE;
@@ skipping 2208 matching lines @@
   }
 }
 
 void Field::AddField(CPDFSDK_Document* pDocument,
                      int nPageIndex,
                      int nFieldType,
                      const CFX_WideString& sName,
                      const CFX_FloatRect& rcCoords) {
   // Not supported.
 }