Merge lp:~ralsina/ubuntuone-control-panel/minus-one-is-the-loneliest-number into lp:ubuntuone-control-panel

[Roberto Alsina]

 - Improved the UX for the bandwidth settings (Fixes lp:847227).

* Removed the warning 'Please note that your files will not sync if you set your bandwidth to 0' which was not even true
* Enable/disable the spinboxes when you check/uncheck the matching checkbox
* Made the minimum bandwidth value 1, removing the bogus -1 and 0 (it still saves -1 to mean disabled)

 - Improved the UX for the bandwidth settings (Fixes lp:847227).

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

[Brian Curtin (brian.curtin)]

review: Approve

=== modified file 'data/qt/preferences.ui'
--- data/qt/preferences.ui	2012-05-15 13:53:50 +0000
+++ data/qt/preferences.ui	2012-05-15 18:48:18 +0000
@@ -38,18 +38,21 @@
       </item>
       <item row="0" column="1">
        <widget class="QSpinBox" name="upload_speed_spinbox">
+        <property name="enabled">
+         <bool>false</bool>
+        </property>
         <property name="alignment">
          <set>Qt::AlignRight|Qt::AlignTrailing|Qt::AlignVCenter</set>
         </property>
         <property name="minimum">
-         <number>-1</number>
+         <number>1</number>
         </property>
         <property name="maximum">
          <number>999999999</number>
         </property>
        </widget>
       </item>
-      <item row="0" column="3">
+      <item row="0" column="2">
        <spacer name="horizontalSpacer_2">
         <property name="orientation">
          <enum>Qt::Horizontal</enum>
@@ -71,11 +74,14 @@
       </item>
       <item row="1" column="1">
        <widget class="QSpinBox" name="download_speed_spinbox">
+        <property name="enabled">
+         <bool>false</bool>
+        </property>
         <property name="alignment">
          <set>Qt::AlignRight|Qt::AlignTrailing|Qt::AlignVCenter</set>
         </property>
         <property name="minimum">
-         <number>-1</number>
+         <number>1</number>
         </property>
         <property name="maximum">
          <number>999999999</number>
@@ -98,19 +104,6 @@
         </property>
        </spacer>
       </item>
-      <item row="3" column="0" colspan="3">
-       <widget class="QLabel" name="label_2">
-        <property name="text">
-         <string notr="true">Please note that your files will not sync if you set bandwidth to 0</string>
-        </property>
-        <property name="scaledContents">
-         <bool>false</bool>
-        </property>
-        <property name="wordWrap">
-         <bool>false</bool>
-        </property>
-       </widget>
-      </item>
      </layout>
     </widget>
    </item>
@@ -223,5 +216,38 @@
   </layout>
  </widget>
  <resources/>
- <connections/>
+ <connections>
+  <connection>
+   <sender>limit_uploads_checkbox</sender>
+   <signal>toggled(bool)</signal>
+   <receiver>upload_speed_spinbox</receiver>
+   <slot>setEnabled(bool)</slot>
+   <hints>
+    <hint type="sourcelabel">
+     <x>94</x>
+     <y>24</y>
+    </hint>
+    <hint type="destinationlabel">
+     <x>256</x>
+     <y>28</y>
+    </hint>
+   </hints>
+  </connection>
+  <connection>
+   <sender>limit_downloads_checkbox</sender>
+   <signal>toggled(bool)</signal>
+   <receiver>download_speed_spinbox</receiver>
+   <slot>setEnabled(bool)</slot>
+   <hints>
+    <hint type="sourcelabel">
+     <x>212</x>
+     <y>70</y>
+    </hint>
+    <hint type="destinationlabel">
+     <x>242</x>
+     <y>66</y>
+    </hint>
+   </hints>
+  </connection>
+ </connections>
 </ui>

=== modified file 'data/qt/ubuntuone.qss'
--- data/qt/ubuntuone.qss	2012-05-15 02:05:41 +0000
+++ data/qt/ubuntuone.qss	2012-05-15 18:48:18 +0000
@@ -338,6 +338,13 @@
     background-color: white;
 }
 
+QSpinBox:disabled {
+    padding: 3px;
+    background-color: transparent;
+    color: darkgrey;
+}
+
+
 QTreeWidget::item:selected {
     background: #fcece7;
     color: black;

=== modified file 'ubuntuone/controlpanel/gui/__init__.py'
--- ubuntuone/controlpanel/gui/__init__.py	2012-05-15 13:53:50 +0000
+++ ubuntuone/controlpanel/gui/__init__.py	2012-05-15 18:48:18 +0000
@@ -224,8 +224,6 @@
 SETTINGS_ALLOW_NOTIFICATIONS = _('Allow all notifications to this device')
 SETTINGS_AUTO_CONNECT = _('Connect automatically when computer starts')
 SETTINGS_BANDWIDTH = _('Bandwidth Settings')
-SETTINGS_BANDWIDTH_ZERO_WARNING = _('Please note that your files will not '
-    'sync if you set bandwidth to 0')
 SETTINGS_BUTTON_APPLY = _('Apply these settings')
 SETTINGS_BUTTON_DEFAULT = _('Default settings')
 SETTINGS_CHANGE_ERROR = _('The settings could not be changed,\n'

=== modified file 'ubuntuone/controlpanel/gui/qt/preferences.py'
--- ubuntuone/controlpanel/gui/qt/preferences.py	2012-05-15 13:53:50 +0000
+++ ubuntuone/controlpanel/gui/qt/preferences.py	2012-05-15 18:48:18 +0000
@@ -29,7 +29,6 @@
     SETTINGS_ALLOW_NOTIFICATIONS,
     SETTINGS_AUTO_CONNECT,
     SETTINGS_BANDWIDTH,
-    SETTINGS_BANDWIDTH_ZERO_WARNING,
     SETTINGS_BUTTON_APPLY,
     SETTINGS_BUTTON_DEFAULT,
     SETTINGS_FILE_SYNC,
@@ -55,22 +54,6 @@
     return CHECKED if checked else UNCHECKED
 
 
-def tweak_speed(speed):
-    """Return the proper speed in kilobytes.
-
-    If speed < 0, return -1.
-    If speed == 0, return 1 (almost nothing, the backed won't accept 0.
-    If speed > 0, return the kilobytes presentation of that.
-
-    """
-    result = -1
-    if speed > 0:
-        result = speed * KILOBYTES
-    elif speed == 0:
-        result = 1
-    return result
-
-
 class PreferencesPanel(UbuntuOneBin):
     """The Preferences Tab Panel widget"""
 
@@ -87,7 +70,6 @@
             SETTINGS_KILOBITS_PER_SECOND)
         self.ui.download_speed_spinbox.setSuffix("  " +
             SETTINGS_KILOBITS_PER_SECOND)
-        self.ui.label_2.setText(SETTINGS_BANDWIDTH_ZERO_WARNING)
         self.ui.restore_defaults_button.setText(SETTINGS_BUTTON_DEFAULT)
 
     # pylint: disable=E0202
@@ -119,40 +101,18 @@
         # signal handlers
         self.ui.download_speed_spinbox.setValue(download_speed)
         self.ui.upload_speed_spinbox.setValue(upload_speed)
+        if upload_speed > 0:
+            self.ui.limit_uploads_checkbox.setCheckState(CHECKED)
+        else:
+            self.ui.limit_uploads_checkbox.setCheckState(UNCHECKED)
+        if download_speed > 0:
+            self.ui.limit_downloads_checkbox.setCheckState(CHECKED)
+        else:
+            self.ui.limit_downloads_checkbox.setCheckState(UNCHECKED)
         self.is_processing = False
 
-    def process_speed_settings(self, new_value, checkbox):
-        """A new speed limit was set."""
-        if new_value < 0:
-            checkbox.setCheckState(UNCHECKED)
-        else:
-            checkbox.setCheckState(CHECKED)
-
-    # Invalid name "on_{down, up}load_speed_spinbox_valueChanged"
     # pylint: disable=C0103
 
-    @QtCore.pyqtSlot(int)
-    def on_download_speed_spinbox_valueChanged(self, new_value):
-        """A new download speed was set."""
-        self.process_speed_settings(new_value,
-                                    self.ui.limit_downloads_checkbox)
-
-    @QtCore.pyqtSlot(int)
-    def on_upload_speed_spinbox_valueChanged(self, new_value):
-        """A new upload speed was set."""
-        self.process_speed_settings(new_value,
-                                    self.ui.limit_uploads_checkbox)
-
-    def on_limit_downloads_checkbox_stateChanged(self, new_state):
-        """Limit download speed checkbox changed."""
-        if new_state == UNCHECKED:
-            self.ui.download_speed_spinbox.setValue(-1)
-
-    def on_limit_uploads_checkbox_stateChanged(self, new_state):
-        """Limit upload speed checkbox changed."""
-        if new_state == UNCHECKED:
-            self.ui.upload_speed_spinbox.setValue(-1)
-
     @QtCore.pyqtSlot()
     @defer.inlineCallbacks
     def on_apply_changes_button_clicked(self):
@@ -161,8 +121,12 @@
         notifications = self.ui.show_all_notifications_checkbox.checkState()
         share_autosubscribe = self.ui.share_autosubscribe_checkbox.checkState()
         udf_autosubscribe = self.ui.udf_autosubscribe_checkbox.checkState()
-        download_speed = tweak_speed(self.ui.download_speed_spinbox.value())
-        upload_speed = tweak_speed(self.ui.upload_speed_spinbox.value())
+        download_speed = self.ui.download_speed_spinbox.value() * KILOBYTES
+        if self.ui.limit_uploads_checkbox.checkState() == UNCHECKED:
+            upload_speed = -1
+        upload_speed = self.ui.upload_speed_spinbox.value() * KILOBYTES
+        if self.ui.limit_downloads_checkbox.checkState() == UNCHECKED:
+            download_speed = -1
 
         settings = {
             backend.AUTOCONNECT_KEY: autoconnect == CHECKED,

=== modified file 'ubuntuone/controlpanel/gui/qt/tests/test_preferences.py'
--- ubuntuone/controlpanel/gui/qt/tests/test_preferences.py	2012-05-15 13:53:50 +0000
+++ ubuntuone/controlpanel/gui/qt/tests/test_preferences.py	2012-05-15 18:48:18 +0000
@@ -55,8 +55,8 @@
             gui.backend.SHOW_ALL_NOTIFICATIONS_KEY: notifs == gui.CHECKED,
             gui.backend.SHARE_AUTOSUBSCRIBE_KEY: share_auto == gui.CHECKED,
             gui.backend.UDF_AUTOSUBSCRIBE_KEY: udf_auto == gui.CHECKED,
-            gui.backend.DOWNLOAD_KEY: gui.tweak_speed(download_speed),
-            gui.backend.UPLOAD_KEY: gui.tweak_speed(upload_speed),
+            gui.backend.DOWNLOAD_KEY: download_speed * gui.KILOBYTES,
+            gui.backend.UPLOAD_KEY: upload_speed * gui.KILOBYTES,
         }
         return result
 
@@ -120,18 +120,20 @@
         speed = settings[gui.backend.DOWNLOAD_KEY]
         if speed > 0:
             speed = settings[gui.backend.DOWNLOAD_KEY] // gui.KILOBYTES
-        download_speed = self.ui.ui.download_speed_spinbox.value()
-        self.assertEqual(speed, download_speed)
+            download_speed = self.ui.ui.download_speed_spinbox.value()
+            self.assertEqual(speed, download_speed)
         limit_downloads = self.ui.ui.limit_downloads_checkbox.checkState()
-        self.assertEqual(speed > 0, limit_downloads == gui.CHECKED)
+        self.assertEqual(self.ui.ui.download_speed_spinbox.isEnabled(),
+            limit_downloads == gui.CHECKED)
 
         speed = settings[gui.backend.UPLOAD_KEY]
         if speed > 0:
             speed = settings[gui.backend.UPLOAD_KEY] // gui.KILOBYTES
-        upload_speed = self.ui.ui.upload_speed_spinbox.value()
-        self.assertEqual(speed, upload_speed)
+            upload_speed = self.ui.ui.upload_speed_spinbox.value()
+            self.assertEqual(speed, upload_speed)
         limit_uploads = self.ui.ui.limit_uploads_checkbox.checkState()
-        self.assertEqual(speed > 0, limit_uploads == gui.CHECKED)
+        self.assertEqual(self.ui.ui.upload_speed_spinbox.isEnabled(),
+            limit_uploads == gui.CHECKED)
 
     def test_update_ui_from_backed_info(self):
         """The ui is correctly updated when the backend info is received."""
@@ -153,63 +155,22 @@
         self.ui.process_info(settings)
         self._test_update_ui_from_settings_info(settings)
 
-    def _test_speed_checkbox_value_changed_to_positive(self, speed_spinbox,
-                                                       limit_checkbox):
-        """When the speed is set to a positive value, enable the checkbox."""
-        limit_checkbox.setCheckState(gui.UNCHECKED)
-        speed_spinbox.setValue(33)
-
-        self.assertEqual(gui.CHECKED, limit_checkbox.checkState())
-
-    def _test_speed_checkbox_value_changed_to_negative(self, speed_spinbox,
-                                                       limit_checkbox):
-        """When the speed is set to a negative value, disable the checkbox."""
-        limit_checkbox.setCheckState(gui.UNCHECKED)
-        speed_spinbox.setValue(-1)
-
-        self.assertEqual(gui.UNCHECKED, limit_checkbox.checkState())
-
-    def _test_speed_checkbox_unchecked(self, speed_spinbox, limit_checkbox):
-        """When the speed checkbox is unchecked, reset speed to -1."""
+    def _test_speed_checkbox_toggled(self, speed_spinbox, limit_checkbox):
+        """The checkbox is checked if and only if the spinbox is enabled."""
         limit_checkbox.setCheckState(gui.CHECKED)
-        speed_spinbox.setValue(33)
+        self.assertTrue(speed_spinbox.isEnabled())
         limit_checkbox.setCheckState(gui.UNCHECKED)
-
-        self.assertEqual(-1, speed_spinbox.value())
-
-    def test_download_speed_checkbox_value_changed_to_positive(self):
-        """When the download speed is set, enable the checkbox."""
-        self._test_speed_checkbox_value_changed_to_positive(
-            self.ui.ui.download_speed_spinbox,
-            self.ui.ui.limit_downloads_checkbox)
-
-    def test_download_speed_checkbox_value_changed_to_negative(self):
-        """When the download speed is set, enable the checkbox."""
-        self._test_speed_checkbox_value_changed_to_negative(
-            self.ui.ui.download_speed_spinbox,
-            self.ui.ui.limit_downloads_checkbox)
-
-    def test_download_speed_checkbox_unchecked(self):
-        """Reset the speed to -1 when the checkbox is disabled."""
-        self._test_speed_checkbox_unchecked(
-            self.ui.ui.download_speed_spinbox,
-            self.ui.ui.limit_downloads_checkbox)
-
-    def test_upload_speed_checkbox_value_changed_to_positive(self):
-        """When the upload speed is set, enable the checkbox."""
-        self._test_speed_checkbox_value_changed_to_positive(
-            self.ui.ui.upload_speed_spinbox,
-            self.ui.ui.limit_uploads_checkbox)
-
-    def test_upload_speed_checkbox_value_changed_to_negative(self):
-        """When the upload speed is set, enable the checkbox."""
-        self._test_speed_checkbox_value_changed_to_negative(
-            self.ui.ui.upload_speed_spinbox,
-            self.ui.ui.limit_uploads_checkbox)
-
-    def test_upload_speed_checkbox_unchecked(self):
-        """Reset the speed to -1 when the checkbox is disabled."""
-        self._test_speed_checkbox_unchecked(
+        self.assertFalse(speed_spinbox.isEnabled())
+
+    def test_download_speed_checkbox_connections(self):
+        """Check connections for download_speed_checkbox."""
+        self._test_speed_checkbox_toggled(
+            self.ui.ui.download_speed_spinbox,
+            self.ui.ui.limit_downloads_checkbox)
+
+    def test_upload_speed_checkbox_connections(self):
+        """Check connections for upload_speed_checkbox."""
+        self._test_speed_checkbox_toggled(
             self.ui.ui.upload_speed_spinbox,
             self.ui.ui.limit_uploads_checkbox)
 
@@ -288,18 +249,6 @@
 
         yield self.ui.ui.restore_defaults_button.click()
 
-    def test_tweak_speed_negative(self):
-        """Tweak speed properly: if param is negative, return -1."""
-        self.assertEqual(-1, gui.tweak_speed(-5))
-
-    def test_tweak_speed_zero(self):
-        """Tweak speed properly: if param is zero, return 1."""
-        self.assertEqual(1, gui.tweak_speed(0))
-
-    def test_tweak_speed_positive(self):
-        """Tweak speed properly: if param is positive, return kilobytes."""
-        self.assertEqual(123456 * gui.KILOBYTES, gui.tweak_speed(123456))
-
     def test_resize_event(self):
         """Check that the QCheckBox widgets receive the proper texts."""
         wrap_calls = {}