Print Preview: Making margin selection sticky (part 2/2)

chrome/browser/resources/print_preview/margin_settings.js

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 cr.define('print_preview', function() {
   'use strict';
 
   /**
    * Creates a Margins object that holds four margin values. The units in which
    * the values are expressed can be any numeric value.
@@ skipping 111 matching lines @@
     this.marginsUI_ = null;
 
     // Holds the custom margin values in points (if set).
     this.customMargins_ = new Margins(-1, -1, -1, -1);
     // Holds the previous custom margin values in points.
     this.previousCustomMargins_ = new Margins(-1, -1, -1, -1);
     // Holds the width of the page in points.
     this.pageWidth_ = -1;
     // Holds the height of the page in points.
     this.pageHeight_ = -1;
-    // The last selected margin option.
-    this.lastSelectedOption_ = MarginSettings.MARGINS_VALUE_DEFAULT;
-
+    // @type {number} The value of the margins type that needs to be restored
+    // once the document loads.
+    this.optionToRestore_ = null;

[kmadhusu, 2011/11/02 20:40:05]

Since this is a number, can you default to -1? From my understanding, null is
neither 0 or false or undefined.

[kmadhusu, 2011/11/02 20:40:05]

optionToRestore_ => MarginValueToRestore_?

[dpapad, 2011/11/02 21:33:57]

Margin value is confusing (sounds like it is a value in points which is not), I
would either leave it as is or choose marginType.

[dpapad, 2011/11/02 21:33:57]

Done. In general though !!null returns false and if (null) works as if (false).

     // Holds the currently updated default page layout values.
     this.currentDefaultPageLayout = null;
     // Holds the default page layout values when the custom margins was last
     // selected.
     this.previousDefaultPageLayout_ = null;
 
     // True if the margins UI should be shown regardless of mouse position.
     this.forceDisplayingMarginLines_ = true;
 
     // @type {EventTracker} Used to keep track of certain event listeners.
@@ skipping 50 matching lines @@
      */
     get customMargins() {
       var margins = {};
       margins.marginLeft = this.customMargins_.left;
       margins.marginTop = this.customMargins_.top;
       margins.marginRight = this.customMargins_.right;
       margins.marginBottom = this.customMargins_.bottom;
       return margins;
     },
 
+    set customMargins(margins) {

[kmadhusu, 2011/11/02 20:40:05]

This function is called only from line 231. Can you inline this function
definition there?

[dpapad, 2011/11/02 21:33:57]

I could but what is the benefit? Having one less function? This way it can be
documented better than when it is inlined.

+      this.customMargins_.left = margins.marginLeft;
+      this.customMargins_.top = margins.marginTop;
+      this.customMargins_.right = margins.marginRight;
+      this.customMargins_.bottom = margins.marginBottom;
+    },
+
     /**
      * @return {number} The value of the selected margin option.
      */
     get selectedMarginsValue() {
       var val = this.marginList_.options[this.marginList_.selectedIndex].value;
       return parseInt(val, 10);
     },
 
     /**
      * Sets the current margin selection to |lastUsedMarginsType|.
      * @param {number} lastUsedMarginsType An integer value identifying a margin
      *     type according to MarginType enum in printing/print_job_constants.h.
+     * @param {Object} lastUsedMargins The last used margins
      */
-    setLastUsedMarginsType: function(lastUsedMarginsType) {
+    setLastUsedMargins: function(lastUsedMarginsType, lastUsedMargins) {

[kmadhusu, 2011/11/02 20:40:05]

"lastUsedMargins" param is optional. (Document this).

Can you make this function to take a single object as its parameter.

setLastUsedMargins: function (lastUsedMarginSettings) {}

where, lastUsedMarginSettings is an object with 2 attributes.
  lastUsedMarginSettings.type = integer value.
  lastUsedMarginSettings.values = margin_value_object (which has top, left,
right, bottom values)

[dpapad, 2011/11/02 21:33:57]

Any parameter not specified is initialized to undefined. In that sense all
parameters are optional in javascript. Turning this in an object adds complexity
in print_preview_handler,cc having to create two nested DictionaryValue objects
and I don't see an obvious benefit.

+      this.optionToRestore_ = lastUsedMarginsType;
       this.marginList_.selectedIndex =
-          this.getMarginOptionIndexByValue_(lastUsedMarginsType);
+          this.getMarginOptionIndexByValue_(this.optionToRestore_);
+      if (lastUsedMargins)
+        this.customMargins = lastUsedMargins;
     },
 
     /**
      * @return {number} The total width of the plugin in points.
      */
     get totalWidthInPoints() {
       var pageInformation = previewArea.pageLocationNormalized;
       return this.pageWidth_ / pageInformation.width;
     },
 
@@ skipping 294 matching lines @@
      * Executes when user selects a different margin option, ie,
      * |this.marginList_.selectedIndex| is changed.
      * @private
      */
     onMarginsChanged_: function() {
       if (this.isDefaultMarginsSelected() || this.isMinimumMarginsSelected() ||
           this.isNoMarginsSelected())
         this.onDefaultMinimumNoMarginsSelected_();
       else if (this.isCustomMarginsSelected())
         this.onCustomMarginsSelected_();
-
-      this.lastSelectedOption_ = this.selectedMarginsValue;
     },
 
     /**
      * Executes when the default or minimum or no margins option is selected.
      * @private
      */
     onDefaultMinimumNoMarginsSelected_: function() {
       this.removeCustomMarginEventListeners_();
       this.forceDisplayingMarginLines_ = true;
       setDefaultValuesAndRegeneratePreview(false);
@@ skipping 73 matching lines @@
      * If custom margins is the currently selected option then change to the
      * default margins option.
      * @private
      */
     resetMarginsIfNeeded: function() {
       if (this.isCustomMarginsSelected()) {
         this.marginList_.options[
             MarginSettings.OPTION_INDEX_DEFAULT].selected = true;
         this.removeCustomMarginEventListeners_();
         this.forceDisplayingMarginLines_ = true;
-        this.lastSelectedOption_ = MarginSettings.MARGINS_VALUE_DEFAULT;
       }
     },
 
     /**
      * Executes when a |customEvents.PDF_LOADED| event occurs.
      * @private
      */
     onPDFLoaded_: function() {
-      if (!previewModifiable)
+      if (!previewModifiable) {
         fadeOutOption(this.marginsOption_);
+        return;
+      }
+
+      if (this.optionToRestore_ &&
+          this.optionToRestore_ == MarginSettings.MARGINS_VALUE_CUSTOM) {
+        this.onCustomMarginsSelected_();
+        this.optionToRestore_ = null;
+      }
     }
   };
 
   return {
     MarginSettings: MarginSettings,
     PageLayout: PageLayout,
+    setLastUsedMargins: MarginSettings.prototype.setLastUsedMargins,
     setNumberFormatAndMeasurementSystem:
         MarginSettings.setNumberFormatAndMeasurementSystem,
   };
 });