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 75 matching lines @@
   };
 
   /**
    * @constructor
    * Class describing the layout of the page.
    */
   function PageLayout(width, height, left, top, right, bottom) {
     this.contentWidth_ = width;
     this.contentHeight_ = height;
     this.margins_ = new Margins(left, top, right, bottom);
+    this.margins_.roundToLocaleUnits();
   }
 
   PageLayout.prototype = {
     /**
      * @type {number} The width of the page.
      */
     get pageWidth() {
       return this.margins_.left + this.margins_.right + this.contentWidth_;
     },
 
     /**
      * @type {number} The height of the page.
      */
     get pageHeight() {
       return this.margins_.top + this.margins_.bottom + this.contentHeight_;
     }
   };
 
   /**
    * Creates a MarginSettings object. This object encapsulates all settings and
    * logic related to the margins mode.
    * @constructor
    */
   function MarginSettings() {
     this.marginsOption_ = $('margins-option');
     this.marginList_ = $('margin-list');
     this.marginsUI_ = null;
 
     // Holds the custom margin values in points (if set).
-    this.customMargins_ = new Margins(-1, -1, -1, -1);
+    this.customMargins_ = null;
     // Holds the previous custom margin values in points.
-    this.previousCustomMargins_ = new Margins(-1, -1, -1, -1);
+    this.previousCustomMargins_ = null;
     // 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 {boolean} True if the margins UI should be diplayed when the next
+    //     |customEvents.PDF_LOADED| event occurs.
+    this.forceMarginsUIOnPDFLoad_ = 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.
     this.eventTracker_ = new EventTracker();
 
     this.addEventListeners_();
   }
 
@@ skipping 46 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;
     },
 
     /**
+     * Sets |this.customMargins_| according to |margins|.
+     * @param {{marginLeft: number, marginTop: number, marginRight: bottom,

[kmadhusu, 2011/11/09 00:57:54]

"marginRight: bottom" => "marginBottom: number"

[dpapad, 2011/11/09 03:19:00]

Done.

+     *     marginRight: number}} margins An object holding the four margin
+     *     values.
+     */
+    set customMargins(margins) {
+      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
+     * Sets the current margin selection to |lastUsedMarginType|.
+     * @param {number} lastUsedMarginType An integer value identifying a margin
      *     type according to MarginType enum in printing/print_job_constants.h.
+     * @param {Object} lastUsedCustomMargins The last used custom margins. If
+     *     custom margins have not been used before

[kmadhusu, 2011/11/09 00:57:54]

nit: 
@param {Object} lastUsedCustomMargins The last used custom margins.
|lastUsedCustomMargins| is undefined if |lastUsedMarginType| is a not a custom
type.

[dpapad, 2011/11/09 03:19:00]

As discussed in IM, this is not true anymore, lastUsedCustomMargins is
|undefined| only if custom margins have not been used since the browser was
launched, not just in the exact previous print preview session).

+     *     |margin{Top|Bottom|Left|Right}| attributes are missing.
      */
-    setLastUsedMarginsType: function(lastUsedMarginsType) {
+    setLastUsedMargins: function(lastUsedMarginType, lastUsedCustomMargins) {
+      this.forceMarginsUIOnPDFLoad_ =
+          lastUsedMarginType == MarginSettings.MARGINS_VALUE_CUSTOM;
       this.marginList_.selectedIndex =
-          this.getMarginOptionIndexByValue_(lastUsedMarginsType);
+          this.getMarginOptionIndexByValue_(lastUsedMarginType);
+      if (lastUsedCustomMargins.hasOwnProperty('marginTop')) {
+        this.customMargins_ = new Margins(-1, -1, -1 , -1);
+        this.customMargins = lastUsedCustomMargins;

[kmadhusu, 2011/11/09 00:57:54]

"this.customMargins" => "this.customMargins_"? Missing underscore..

[dpapad, 2011/11/09 03:19:00]

This is on purpose, leaving the underscore out, causes the set customMargins()
function to execute. This is not just an assignment.

+      }
     },
 
     /**
      * @return {number} The total width of the plugin in points.
      */
     get totalWidthInPoints() {
       var pageInformation = previewArea.pageLocationNormalized;
       return this.pageWidth_ / pageInformation.width;
     },
 
@@ skipping 52 matching lines @@
     },
 
     /**
      * If the custom margin values have changed then request a new preview based
      * on the newly set margins.
      * @private
      */
     requestPreviewIfNeeded_: function() {
       if (!this.areMarginSettingsValid())
         return;
-      if (this.customMargins_.isEqual(this.previousCustomMargins_))
+
+      if (this.previousCustomMargins_ &&
+          this.customMargins_.isEqual(this.previousCustomMargins_))
         return;
+
+      if (!this.previousCustomMargins_)
+        this.previousCustomMargins_ = new Margins(-1, -1, -1, -1);
       this.previousCustomMargins_.copy(this.customMargins_);

[kmadhusu, 2011/11/09 00:57:54]

Do you need to the same in line 239?

this.customMargins_.copy(lastUsedCustomMargins);

[dpapad, 2011/11/09 03:19:00]

It is not needed there, because a preview request will be issued from
print_preview.js after |setLastUsedMargins| returns. |previousCustomMargins| are
needed only when |MarginSettings.prototype.requestPreviewIfNeeded| is called.

       setDefaultValuesAndRegeneratePreview(false);
     },
 
     /**
      * Listener executed when the mouse is over the sidebar. If the custom
      * margin lines are displayed, then, it fades them out.
      * @private
      */
     onSidebarMouseOver_: function(e) {
+      $('mainview').onmouseover = this.onMainviewMouseOver_.bind(this);
+      $('sidebar').onmouseover = null;
       if (!this.forceDisplayingMarginLines_)
         this.marginsUI.hide(false);
     },
 
     /**
      * Listener executed when the mouse is over the main view. If the custom
      * margin lines are hidden, then, it fades them in.
      * @private
      */
     onMainviewMouseOver_: function() {
+      $('mainview').onmouseover = null;
+      $('sidebar').onmouseover = this.onSidebarMouseOver_.bind(this);
       this.forceDisplayingMarginLines_ = false;
       this.marginsUI.show();
     },
 
     /**
      * Adds listeners to all margin related controls.
      * @private
      */
     addEventListeners_: function() {
       this.marginList_.onchange = this.onMarginsChanged_.bind(this);
@@ skipping 36 matching lines @@
         return dragDelta * this.totalHeightInPoints;
       } else {
         return dragDelta * this.totalWidthInPoints;
       }
     },
 
     /**
      * @return {boolean} True if the margin settings are valid.
      */
     areMarginSettingsValid: function() {
-      if (this.marginsUI_ == null)
+      if (!this.isCustomMarginsSelected() || !this.marginsUI_)
         return true;
 
       var pairs = this.marginsUI.pairsAsList;
       return pairs.every(function(pair) { return pair.box_.isValid; });
     },
 
     /**
      * Calculates the maximum allowable value of the selected margin text for
      * every margin.
      * @return {array} The maximum allowable value in points in order top, left,
@@ skipping 85 matching lines @@
                              false);
       this.eventTracker_.add(document, customEvents.MARGIN_TEXTBOX_FOCUSED,
                              this.onMarginTextboxFocused_.bind(this), false);
     },
 
     /**
      * Removes the event listeners associated with the custom margins option.
      * @private
      */
     removeCustomMarginEventListeners_: function() {
+      if (!this.marginsUI_)
+        return;
       $('mainview').onmouseover = null;
       $('sidebar').onmouseover = null;
       this.eventTracker_.remove(this.marginsUI, customEvents.MARGIN_LINE_DRAG);
       this.eventTracker_.remove(document, customEvents.MARGIN_TEXTBOX_FOCUSED);
       this.marginsUI.hide(true);
     },
 
     /**
      * Updates |this.marginsUI| depending on the specified margins and the
      * position of the page within the plugin.
@@ skipping 38 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;
+      this.previousCustomMargins_ = null;
       setDefaultValuesAndRegeneratePreview(false);
     },
 
     /**
      * Executes when the custom margins option is selected.
      * @private
      */
     onCustomMarginsSelected_: function() {
-      this.addCustomMarginEventListeners_();
+      var customMarginsNotSpecified = !this.customMargins_;
+      this.updatePageData_();
 
-      this.customMargins_ = this.currentDefaultPageLayout.margins_;
-      this.customMargins_.roundToLocaleUnits();
-      this.previousCustomMargins_.copy(this.customMargins_);
-
-      if (this.previousDefaultPageLayout_ != this.currentDefaultPageLayout) {
-        this.pageWidth_ = this.currentDefaultPageLayout.pageWidth;
-        this.pageHeight_ = this.currentDefaultPageLayout.pageHeight;
+      if (customMarginsNotSpecified) {

[kmadhusu, 2011/11/09 00:57:54]

customMarginsNotSpecified variable is not required here.

Just do: if (!this.customMargins_)

[dpapad, 2011/11/09 03:19:00]

this.updatePageData_ will initialize |this.customMargins_| if not already
initialized, therefore I need this boolean, in other words at this line
!this.customMargins is always false.

+        this.previousCustomMargins_ = new Margins(-1, -1, -1, -1);
+        this.previousCustomMargins_.copy(this.customMargins_);

[kmadhusu, 2011/11/09 00:57:54]

this.customMargins_ is null here. 

Instead of lines 588 & 589, You can do
this.previeousCustomMargins_ = null;

[kmadhusu, 2011/11/09 00:57:54]

Can you create a helper function to do the following:
Copy(srcMarginsObj, destMarginsObj) {
 if (!destMarginsObj)
   destMarginsObj = new Margins(-1, -1, -1, -1);
 destMarginsObj.copy(srcMarginsObj);
}

I see the same pattern in 3 or more places. 

[dpapad, 2011/11/09 03:19:00]

|this.customMargins_| is not null here, explained in previous comment.

[dpapad, 2011/11/09 03:19:00]

Done. Created a clone() function which is much more flexible.

+        this.drawCustomMarginsUI_();
+        this.addCustomMarginEventListeners_();
+        this.marginsUI.show();
+      } else {
+        this.forceMarginsUIOnPDFLoad_ = true;
+        this.requestPreviewIfNeeded_();
       }
-
-      this.previousDefaultPageLayout_ = this.currentDefaultPageLayout;
-      this.drawCustomMarginsUI_();
-      this.marginsUI.show();
     },
 
     /**
      * Calculates the coordinates of the four margin lines. These are the
      * coordinates where the margin lines should be displayed. The coordinates
      * are expressed in terms of percentages with respect to the total width
      * and height of the plugin.
      * @return {print_preview.Rect} A rectnangle that describes the position of
      *     the four margin lines.
      * @private
@@ skipping 42 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.forceMarginsUIOnPDFLoad_) {
+        this.updatePageData_();
+        this.drawCustomMarginsUI_();
+        this.addCustomMarginEventListeners_();
+        this.marginsUI.show();
+        this.forceMarginsUIOnPDFLoad_ = false;
+      }
+    },
+
+    /**
+     * Updates |this.customMargins_|, |this.pageWidth_|, |this.pageHeight_|.
+     * @private
+     */
+    updatePageData_: function() {
+      if (!this.customMargins_) {
+        this.customMargins_ = new Margins(-1, -1, -1. -1);
+        this.customMargins_.copy(this.currentDefaultPageLayout.margins_);
+      }
+
+      this.pageWidth_ = this.currentDefaultPageLayout.pageWidth;
+      this.pageHeight_ = this.currentDefaultPageLayout.pageHeight;
     }
   };
 
   return {
     MarginSettings: MarginSettings,
     PageLayout: PageLayout,
-    setNumberFormatAndMeasurementSystem:
-        MarginSettings.setNumberFormatAndMeasurementSystem,
   };
 });