Printer dropdown cleanup

chrome/browser/resources/print_preview/print_preview.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.
 
 // require: cr/ui/print_preview_cloud.js
 
 var localStrings = new LocalStrings();
 
 // If useCloudPrint is true we attempt to connect to cloud print
 // and populate the list of printers with cloud print printers.
 var useCloudPrint = false;
 
 // Store the last selected printer index.
 var lastSelectedPrinterIndex = 0;
 
 // Used to disable some printing options when the preview is not modifiable.
 var previewModifiable = false;
 
+// True if a request to generate a preview has been sent to the browser in
+// this session.
+var previewRequested = false;

[dpapad, 2011/07/19 18:44:55]

Instead of adding another state variable, you could use the existing
|lastPreviewRequestID| to determine if a preview has been requested (it will be
greater than -1). I would prefer having a function 
function hasRequestedPreview() { return lastPreviewRequestID > -1; }
to keep the number of state variables to a minimum.

[Albert Bodenhamer, 2011/07/19 21:53:19]

Done.

+
 // Destination list special value constants.
 const ADD_CLOUD_PRINTER = 'addCloudPrinter';
 const MANAGE_CLOUD_PRINTERS = 'manageCloudPrinters';
 const MANAGE_LOCAL_PRINTERS = 'manageLocalPrinters';
 const MORE_PRINTERS = 'morePrinters';
 const SIGN_IN = 'signIn';
 const PRINT_TO_PDF = 'Print To PDF';
 
 // State of the print preview settings.
 var printSettings = new PrintSettings();
@@ skipping 20 matching lines @@
 var copiesSettings;
 
 // Object holding all the layout related settings.
 var layoutSettings;
 
 // True if the user has click 'Advanced...' in order to open the system print
 // dialog.
 var showingSystemDialog = false;
 
 // The range of options in the printer dropdown controlled by cloud print.
-var firstCloudPrintOptionPos = 0
+var firstCloudPrintOptionPos = 0;
 var lastCloudPrintOptionPos = firstCloudPrintOptionPos;
 
+// A dictionary of cloud printers that have been added to the printer
+// dropdown.
+var addedCloudPrinters = {};

[dpapad, 2011/07/19 18:44:55]

Could these be moved within print_preview_cloud.js? Also it seems that there is
a lot of cloud print related logic in this file.
  In general there has been an effort to encapsulate related options to their
own separate class, please take a look at bug 88098 and the CLs that refer to
that bug. I would consider creating a class like CloudPrintSettings and put
everything that is cloud print related in there.
  I am not saying this has to be done in this CL necessarily, but if you think
you can follow the same pattern, please put this in your task queue.

[Albert Bodenhamer, 2011/07/19 21:53:19]

The boundary has definitely gotten muddy as things have come together.  My
original intent was that code that interacted directly with the print preview UI
would live in print_preview.js while logic related to GCP would live in
print_preview_cloud.js.

Thanks for being flexible on the refactoring.  I've added a todo to keep track
of it.

On 2011/07/19 18:44:55, dpapad wrote:

[dpapad, 2011/07/19 22:22:58]

 The UI logic has migrated (to some degree) outside print_preview.js, which
should act more like a controller (in the MVC pattern), the UI should receive
generic events and update itself. 
 Thanks for taking a note on this, please refer your CL to the same issue
(88098), when you get to it.

+
+// The maximum number of cloud printers to allow in the dropdown.
+const maxCloudPrinters = 10;
+
+
 /**
  * Window onload handler, sets up the page and starts print preview by getting
  * the printer list.
  */
 function onLoad() {
   cr.enablePlatformSpecificCSSRules();
 
   $('cancel-button').addEventListener('click', handleCancelButtonClick);
 
   if (!checkCompatiblePluginExists()) {
@@ skipping 114 matching lines @@
   if (cloudprint.isCloudPrint(printerList.options[selectedIndex])) {
     updateWithCloudPrinterCapabilities();
     skip_refresh = true;
   } else if (selectedValue == SIGN_IN ||
              selectedValue == MANAGE_CLOUD_PRINTERS ||
              selectedValue == MANAGE_LOCAL_PRINTERS ||
              selectedValue == ADD_CLOUD_PRINTER) {
     printerList.selectedIndex = lastSelectedPrinterIndex;
     chrome.send(selectedValue);
     skip_refresh = true;
+  } else if (selectedValue == MORE_PRINTERS) {
+    onSystemDialogLinkClicked();
+    skip_refresh = true;
   } else if (selectedValue == PRINT_TO_PDF) {
     updateWithPrinterCapabilities({
         'disableColorOption': true,
         'setColorAsDefault': true,
         'setDuplexAsDefault': false,
         'disableCopiesOption': true});
   } else {
     // This message will call back to 'updateWithPrinterCapabilities'
     // function.
     chrome.send('getPrinterCapabilities', [selectedValue]);
@@ skipping 215 matching lines @@
  */
 function requestPrintPreview() {
   hasPendingPreviewRequest = true;
   removeEventListeners();
   printSettings.save();
   if (!isTabHidden)
     showLoadingAnimation();
 
   var settings = getSettings();
   settings.requestID = generatePreviewRequestID();
+  previewRequested = true;
   chrome.send('getPreview', [JSON.stringify(settings)]);
 }
 
 /**
  * Called from PrintPreviewUI::OnFileSelectionCancelled to notify the print
  * preview tab regarding the file selection cancel event.
  */
 function fileSelectionCancelled() {
   // TODO(thestig) re-enable controls here.
 }
 
 /**
  * Set the default printer. If there is one, generate a print preview.
  * @param {string} printer Name of the default printer. Empty if none.
  * @param {string} cloudPrintData Cloud print related data to restore if
  *                 the default printer is a cloud printer.
  */
 function setDefaultPrinter(printer_name, cloudPrintData) {
-  // Add a placeholder value so the printer list looks valid.
-  addDestinationListOption('', '', true, true, true);
   if (printer_name) {
     defaultOrLastUsedPrinterName = printer_name;
     if (cloudPrintData) {
       cloudprint.setDefaultPrinter(printer_name,
                                    cloudPrintData,
                                    addCloudPrinters,
                                    doUpdateCloudPrinterCapabilities);
     } else {
-      $('printer-list')[0].value = defaultOrLastUsedPrinterName;
+      addDestinationListOption('',
+                               defaultOrLastUsedPrinterName,
+                               true,
+                               true,
+                               true);
       updateControlsWithSelectedPrinterCapabilities();
     }
   }
   chrome.send('getPrinters');
 }
 
 /**
  * Fill the printer list drop down.
  * Called from PrintPreviewHandler::SendPrinterList().
  * @param {Array} printers Array of printer info objects.
  */
 function setPrinters(printers) {
-  var printerList = $('printer-list');
-  // If there exists a dummy printer value, then setDefaultPrinter() already
-  // requested a preview, so no need to do it again.
-  var needPreview = (printerList[0].value == '');
   for (var i = 0; i < printers.length; ++i) {
     var isDefault = (printers[i].deviceName == defaultOrLastUsedPrinterName);
     addDestinationListOption(printers[i].printerName, printers[i].deviceName,
                              isDefault, false, false);
   }
 
-  if (!cloudprint.isCloudPrint(printerList[0]))
-    // Remove the dummy printer added in setDefaultPrinter().
-    printerList.remove(0);
-
   if (printers.length != 0)
     addDestinationListOption('', '', false, true, true);
 
   // Adding option for saving PDF to disk.
   addDestinationListOption(localStrings.getString('printToPDF'),
                            PRINT_TO_PDF,
                            defaultOrLastUsedPrinterName == PRINT_TO_PDF,
                            false,
                            false);
   addDestinationListOption('', '', false, true, true);
 
   // Add options to manage printers.
   if (!cr.isChromeOS) {
     addDestinationListOption(localStrings.getString('manageLocalPrinters'),
         MANAGE_LOCAL_PRINTERS, false, false, false);
   }
   if (useCloudPrint) {
     cloudprint.fetchPrinters(addCloudPrinters, false);
+    cloudprint.fetchPrinters(addCloudPrinters, true);

[dpapad, 2011/07/19 18:44:55]

Why fetching printers twice? Is this for getting a short list quickly and then
fetch the full list?

[Albert Bodenhamer, 2011/07/19 21:53:19]

Yep.  We fetch the recents and the full list separately and then merge them to
fill out the list.  I've added comments to make it more clear.

On 2011/07/19 18:44:55, dpapad wrote:

     addDestinationListOption(localStrings.getString('manageCloudPrinters'),
         MANAGE_CLOUD_PRINTERS, false, false, false);
   }
 
+  var printerList = $('printer-list');

[dpapad, 2011/07/19 18:44:55]

Since $('printer-list') is only used once here, no need to cache it to a local
var.

[Albert Bodenhamer, 2011/07/19 21:53:19]

Done.

   printerList.disabled = false;
 
-  if (needPreview)
+  if (!previewRequested)
     updateControlsWithSelectedPrinterCapabilities();
 }
 
 /**
  * Creates an option that can be added to the printer destination list.
  * @param {String} optionText specifies the option text content.
  * @param {String} optionValue specifies the option value.
  * @param {boolean} isDefault is true if the option needs to be selected.
  * @param {boolean} isDisabled is true if the option needs to be disabled.
  * @param {boolean} isSeparator is true if the option is a visual separator and
@@ skipping 53 matching lines @@
                                            isDefault,
                                            isDisabled,
                                            isSeparator);
   var printerList = $('printer-list');
   var before = printerList[position];
   printerList.add(option, before);
   return option;
 }
 
 /**
- * Removes the list of cloud printers from the printer pull down.
+ * Test if a particular cloud printer has already been added to the
+ * printer dropdown.
+ * @param {String} id - A unique value to track this printer.

[dpapad, 2011/07/19 18:44:55]

Nit: No need for "-", just use a space between parameter name and comment, here
and elsewhere.

[Albert Bodenhamer, 2011/07/19 21:53:19]

Done.

+ * @return {boolean} Returns true if this id has previously
+ *                   been passed to trackCloudPrinterAdded.

[dpapad, 2011/07/19 18:44:55]

Nit: Use 4 space indent as usual, here and elsewhere.

[Albert Bodenhamer, 2011/07/19 21:53:19]

Done.

  */
-function clearCloudPrinters() {
-  var printerList = $('printer-list');
-  while (firstCloudPrintOptionPos < lastCloudPrintOptionPos) {
-    lastCloudPrintOptionPos--;
-    printerList.remove(lastCloudPrintOptionPos);
+function cloudPrinterAlreadyAdded(id) {
+  return (addedCloudPrinters[id]);
+}
+
+/**
+ * Record that a cloud printer will added to the printer dropdown.
+ * @param {String} id - A unique value to track this printer.
+ * @return {boolean} Returns false if there adding this printer
+ *                   would exceed maxCloudPrinters.
+ */
+function trackCloudPrinterAdded(id) {
+  if (Object.keys(addedCloudPrinters).length < maxCloudPrinters) {
+    addedCloudPrinters[id] = true;
+    return true;
+  } else {
+    return false;
   }
 }
 
+
 /**
  * Add cloud printers to the list drop down.
  * Called from the cloudprint object on receipt of printer information from the
  * cloud print server.
  * @param {Array} printers Array of printer info objects.
  * @return {Object} The currently selected printer.
  */
-function addCloudPrinters(printers, showMorePrintersOption) {
-  // TODO(abodenha@chromium.org) Avoid removing printers from the list.
-  // Instead search for duplicates and don't add printers that already exist
-  // in the list.
-  clearCloudPrinters();
-  addDestinationListOptionAtPosition(
-      lastCloudPrintOptionPos++,
-      localStrings.getString('cloudPrinters'),
-      '',
-      false,
-      true,
-      false);
+function addCloudPrinters(printers) {
+  var isFirstPass = false;
+  var showMorePrintersOption = false;
+
+  if (firstCloudPrintOptionPos == lastCloudPrintOptionPos) {
+    isFirstPass = true;
+    var option = addDestinationListOptionAtPosition(
+        lastCloudPrintOptionPos++,
+        localStrings.getString('cloudPrinters'),
+        'Label',
+        false,
+        true,
+        false);
+    cloudprint.setCloudPrint(option, null, null);
+  }
   if (printers != null) {
     if (printers.length == 0) {
       addDestinationListOptionAtPosition(lastCloudPrintOptionPos++,
           localStrings.getString('addCloudPrinter'),
           ADD_CLOUD_PRINTER,
           false,
           false,
           false);
     } else {
       for (printer in printers) {

[dpapad, 2011/07/19 18:44:55]

Do not use for in statement with arrays, since it does not behave as expected,
see JS styleguide
http://www.corp.google.com/eng/doc/javascriptguide.xml?showone=for-in_loop#fo....

[Albert Bodenhamer, 2011/07/19 21:53:19]

Done.

-        var option = addDestinationListOptionAtPosition(
-            lastCloudPrintOptionPos++,
-            printers[printer]['name'],
-            printers[printer]['id'],
-            printers[printer]['name'] == defaultOrLastUsedPrinterName,
-            false,
-            false);
-        cloudprint.setCloudPrint(option,
-                                 printers[printer]['name'],
-                                 printers[printer]['id']);
+        if (!cloudPrinterAlreadyAdded(printers[printer]['id'])) {
+          var option = addDestinationListOptionAtPosition(
+              lastCloudPrintOptionPos++,
+              printers[printer]['name'],
+              printers[printer]['id'],
+              printers[printer]['name'] == defaultOrLastUsedPrinterName,
+              false,
+              false);
+          cloudprint.setCloudPrint(option,
+                                   printers[printer]['name'],
+                                   printers[printer]['id']);
+          if (!trackCloudPrinterAdded(printers[printer]['id'])) {
+            showMorePrintersOption = true;
+            break;
+          }
+        }
       }
       if (showMorePrintersOption) {
         addDestinationListOptionAtPosition(lastCloudPrintOptionPos++,
                                            '',
                                            '',
                                            false,
                                            true,
                                            true);
         addDestinationListOptionAtPosition(lastCloudPrintOptionPos++,
             localStrings.getString('morePrinters'),
             MORE_PRINTERS, false, false, false);
       }
     }
   } else {
-    addDestinationListOptionAtPosition(lastCloudPrintOptionPos++,
-                                       localStrings.getString('signIn'),
-                                       SIGN_IN,
+    if (!cloudPrinterAlreadyAdded(SIGN_IN)) {
+      addDestinationListOptionAtPosition(lastCloudPrintOptionPos++,
+                                         localStrings.getString('signIn'),
+                                         SIGN_IN,
+                                         false,
+                                         false,
+                                         false);
+      trackCloudPrinterAdded(SIGN_IN);
+    }
+  }
+  if (isFirstPass) {
+    addDestinationListOptionAtPosition(lastCloudPrintOptionPos,
+                                       '',
+                                       '',
                                        false,
+                                       true,
+                                       true);
+    addDestinationListOptionAtPosition(lastCloudPrintOptionPos + 1,
+                                       localStrings.getString('localPrinters'),
+                                       '',
                                        false,
+                                       true,
                                        false);
   }
-  addDestinationListOptionAtPosition(lastCloudPrintOptionPos++,
-                                     '',
-                                     '',
-                                     false,
-                                     true,
-                                     true);
-  addDestinationListOptionAtPosition(lastCloudPrintOptionPos++,
-                                     localStrings.getString('localPrinters'),
-                                     '',
-                                     false,
-                                     true,
-                                     false);
   var printerList = $('printer-list')
   var selectedPrinter = printerList.selectedIndex;
   if (selectedPrinter < 0)
     return null;
   return printerList.options[selectedPrinter];
 }
 
 /**
  * Sets the color mode for the PDF plugin.
  * Called from PrintPreviewHandler::ProcessColorSetting().
@@ skipping 272 matching lines @@
 }
 
 /**
  * Takes a snapshot of the print settings.
  */
 PrintSettings.prototype.save = function() {
   this.deviceName = getSelectedPrinterName();
   this.isLandscape = layoutSettings.isLandscape();
 }