Merge lp:~diegosarmentero/ubuntuone-control-panel/checkwrap into lp:ubuntuone-control-panel
- checkwrap
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Manuel de la Peña |
Approved revision: | 317 |
Merged at revision: | 313 |
Proposed branch: | lp:~diegosarmentero/ubuntuone-control-panel/checkwrap |
Merge into: | lp:ubuntuone-control-panel |
Diff against target: |
229 lines (+133/-14) 4 files modified
ubuntuone/controlpanel/gui/qt/__init__.py (+21/-3) ubuntuone/controlpanel/gui/qt/preferences.py (+22/-10) ubuntuone/controlpanel/gui/qt/tests/test_common.py (+60/-1) ubuntuone/controlpanel/gui/qt/tests/test_preferences.py (+30/-0) |
To merge this branch: | bzr merge lp:~diegosarmentero/ubuntuone-control-panel/checkwrap |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Manuel de la Peña (community) | Approve | ||
Roberto Alsina (community) | Approve | ||
Review via email: mp+102152@code.launchpad.net |
Commit message
Adding a word_wrap function to use it in those cases where the widget don't implement a wrapping functionality.
Description of the change
Diego Sarmentero (diegosarmentero) wrote : | # |
- 314. By Diego Sarmentero
-
Fixing lint issue.
- 315. By Diego Sarmentero
-
reverting code for example.
Manuel de la Peña (mandel) wrote : | # |
I really thing we can get a much better algorithm here to calculate the wrapping a good example to do so is to read http://
Roberto Alsina (ralsina) wrote : | # |
+1 and sharing mandel's algorithm caveat
- 316. By Diego Sarmentero
-
Improve wrapping algorithm, fix tests.
Diego Sarmentero (diegosarmentero) wrote : | # |
> I really thing we can get a much better algorithm here to calculate the
> wrapping a good example to do so is to read
> http://
> similar that does not force us to have a loop inside an other.
Done
Manuel de la Peña (mandel) wrote : | # |
Two comments,
1. I have been told (I' not sure how official is this) that we should start making some move towards using python 3, due to that we should stop using the old string formating 'Hola %s' % 'gatox' to the new one 'Hola {0}'.format(
2. I'm not sure about the removal of the \n in the text.. In translations they do not care about the length of the string but they do care about the paragraph separations. What I mean is:
'This is a very long text, lets say I use more than one paragraph to separater ideas.\n\nThis is my new paragraph focusing on a diff idea.'
In these type of cases we are breaking the paragraph separation that is used by the translator with the intent to express to diff ideas. What I would do is not to remove \n and to check at in every word if we have a \n at the end (word[-1] is faster than endswith.) if that is the case we should set the size to be the full line size.
3. Since this is a very generic method (can be used in any possible widget) and there is no limit in the length of the strings to be wrapped I would assume the worst case scenario which is the case in which strings are very long. If that is the case using += to create the final text is probably not the best thing to do. Following the recommendations from http://
- 317. By Diego Sarmentero
-
Improving algorithm.
Diego Sarmentero (diegosarmentero) wrote : | # |
> Two comments,
>
> 1. I have been told (I' not sure how official is this) that we should start
> making some move towards using python 3, due to that we should stop using the
> old string formating 'Hola %s' % 'gatox' to the new one 'Hola
> {0}'.format(
>
Stay as it is because we didn't decide how to use yet.
> 2. I'm not sure about the removal of the \n in the text.. In translations they
> do not care about the length of the string but they do care about the
> paragraph separations. What I mean is:
>
> 'This is a very long text, lets say I use more than one paragraph to separater
> ideas.\n\nThis is my new paragraph focusing on a diff idea.'
>
> In these type of cases we are breaking the paragraph separation that is used
> by the translator with the intent to express to diff ideas. What I would do is
> not to remove \n and to check at in every word if we have a \n at the end
> (word[-1] is faster than endswith.) if that is the case we should set the size
> to be the full line size.
>
Fixed, the '\n' is not needed because we are always providing the original string for the wrapping method.
> 3. Since this is a very generic method (can be used in any possible widget)
> and there is no limit in the length of the strings to be wrapped I would
> assume the worst case scenario which is the case in which strings are very
> long. If that is the case using += to create the final text is probably not
> the best thing to do. Following the recommendations from
> http://
> would at least move to .join instead of +=.
Changed.
Preview Diff
1 | === modified file 'ubuntuone/controlpanel/gui/qt/__init__.py' | |||
2 | --- ubuntuone/controlpanel/gui/qt/__init__.py 2012-03-27 12:52:17 +0000 | |||
3 | +++ ubuntuone/controlpanel/gui/qt/__init__.py 2012-04-17 18:12:18 +0000 | |||
4 | @@ -1,8 +1,6 @@ | |||
5 | 1 | # -*- coding: utf-8 -*- | 1 | # -*- coding: utf-8 -*- |
6 | 2 | |||
7 | 3 | # Authors: Natalia B Bidart <natalia.bidart@canonical.com> | ||
8 | 4 | # | 2 | # |
10 | 5 | # Copyright 2010 Canonical Ltd. | 3 | # Copyright 2010-2012 Canonical Ltd. |
11 | 6 | # | 4 | # |
12 | 7 | # This program is free software: you can redistribute it and/or modify it | 5 | # This program is free software: you can redistribute it and/or modify it |
13 | 8 | # under the terms of the GNU General Public License version 3, as published | 6 | # under the terms of the GNU General Public License version 3, as published |
14 | @@ -33,6 +31,26 @@ | |||
15 | 33 | ) | 31 | ) |
16 | 34 | 32 | ||
17 | 35 | 33 | ||
18 | 34 | def force_wordwrap(widget, size, text): | ||
19 | 35 | """Set the text to the widget after word wrapping it.""" | ||
20 | 36 | font_metrics = QtGui.QFontMetrics(widget.font()) | ||
21 | 37 | text_width = font_metrics.width(text) | ||
22 | 38 | if size > 0 and text_width > size: | ||
23 | 39 | words = text.split(' ') | ||
24 | 40 | text = '' | ||
25 | 41 | size_left = size | ||
26 | 42 | for word in words: | ||
27 | 43 | word_size = font_metrics.width(word) | ||
28 | 44 | if (size_left - word_size) > 0 or word_size > size: | ||
29 | 45 | text = ''.join((text, '%s ' % word)) | ||
30 | 46 | size_left -= word_size | ||
31 | 47 | else: | ||
32 | 48 | text = ''.join((text, '\n%s ' % word)) | ||
33 | 49 | size_left = size - word_size | ||
34 | 50 | text = text.rstrip() | ||
35 | 51 | widget.setText(text) | ||
36 | 52 | |||
37 | 53 | |||
38 | 36 | def uri_hook(uri): | 54 | def uri_hook(uri): |
39 | 37 | """Open an URI using the default browser/file manager.""" | 55 | """Open an URI using the default browser/file manager.""" |
40 | 38 | if uri.startswith(FILE_URI_PREFIX): | 56 | if uri.startswith(FILE_URI_PREFIX): |
41 | 39 | 57 | ||
42 | === modified file 'ubuntuone/controlpanel/gui/qt/preferences.py' | |||
43 | --- ubuntuone/controlpanel/gui/qt/preferences.py 2012-03-28 12:06:28 +0000 | |||
44 | +++ ubuntuone/controlpanel/gui/qt/preferences.py 2012-04-17 18:12:18 +0000 | |||
45 | @@ -1,8 +1,6 @@ | |||
46 | 1 | # -*- coding: utf-8 -*- | 1 | # -*- coding: utf-8 -*- |
47 | 2 | |||
48 | 3 | # Authors: Alejandro J. Cura <alecu@canonical.com> | ||
49 | 4 | # | 2 | # |
51 | 5 | # Copyright 2011 Canonical Ltd. | 3 | # Copyright 2011-2012 Canonical Ltd. |
52 | 6 | # | 4 | # |
53 | 7 | # This program is free software: you can redistribute it and/or modify it | 5 | # This program is free software: you can redistribute it and/or modify it |
54 | 8 | # under the terms of the GNU General Public License version 3, as published | 6 | # under the terms of the GNU General Public License version 3, as published |
55 | @@ -41,6 +39,7 @@ | |||
56 | 41 | SETTINGS_SYNC_ALL_FOLDERS, | 39 | SETTINGS_SYNC_ALL_FOLDERS, |
57 | 42 | SETTINGS_SYNC_ALL_SHARES, | 40 | SETTINGS_SYNC_ALL_SHARES, |
58 | 43 | ) | 41 | ) |
59 | 42 | from ubuntuone.controlpanel.gui.qt import force_wordwrap | ||
60 | 44 | from ubuntuone.controlpanel.gui.qt.ubuntuonebin import UbuntuOneBin | 43 | from ubuntuone.controlpanel.gui.qt.ubuntuonebin import UbuntuOneBin |
61 | 45 | from ubuntuone.controlpanel.gui.qt.ui import preferences_ui | 44 | from ubuntuone.controlpanel.gui.qt.ui import preferences_ui |
62 | 46 | 45 | ||
63 | @@ -82,19 +81,12 @@ | |||
64 | 82 | """Do some extra setupping for the UI.""" | 81 | """Do some extra setupping for the UI.""" |
65 | 83 | super(PreferencesPanel, self)._setup() | 82 | super(PreferencesPanel, self)._setup() |
66 | 84 | self.ui.apply_changes_button.setText(SETTINGS_BUTTON_APPLY) | 83 | self.ui.apply_changes_button.setText(SETTINGS_BUTTON_APPLY) |
67 | 85 | self.ui.autoconnect_checkbox.setText(SETTINGS_AUTO_CONNECT) | ||
68 | 86 | self.ui.bandwidth_settings.setTitle(SETTINGS_BANDWIDTH) | 84 | self.ui.bandwidth_settings.setTitle(SETTINGS_BANDWIDTH) |
69 | 87 | self.ui.file_sync_settings.setTitle(SETTINGS_FILE_SYNC) | 85 | self.ui.file_sync_settings.setTitle(SETTINGS_FILE_SYNC) |
70 | 88 | self.ui.kbps_label_1.setText(SETTINGS_KILOBITS_PER_SECOND) | 86 | self.ui.kbps_label_1.setText(SETTINGS_KILOBITS_PER_SECOND) |
71 | 89 | self.ui.kbps_label_2.setText(SETTINGS_KILOBITS_PER_SECOND) | 87 | self.ui.kbps_label_2.setText(SETTINGS_KILOBITS_PER_SECOND) |
72 | 90 | self.ui.label_2.setText(SETTINGS_BANDWIDTH_ZERO_WARNING) | 88 | self.ui.label_2.setText(SETTINGS_BANDWIDTH_ZERO_WARNING) |
73 | 91 | self.ui.limit_uploads_checkbox.setText(SETTINGS_LIMIT_UPLOAD) | ||
74 | 92 | self.ui.limit_downloads_checkbox.setText(SETTINGS_LIMIT_DOWNLOAD) | ||
75 | 93 | self.ui.restore_defaults_button.setText(SETTINGS_BUTTON_DEFAULT) | 89 | self.ui.restore_defaults_button.setText(SETTINGS_BUTTON_DEFAULT) |
76 | 94 | self.ui.share_autosubscribe_checkbox.setText(SETTINGS_SYNC_ALL_FOLDERS) | ||
77 | 95 | self.ui.show_all_notifications_checkbox.setText( | ||
78 | 96 | SETTINGS_ALLOW_NOTIFICATIONS) | ||
79 | 97 | self.ui.udf_autosubscribe_checkbox.setText(SETTINGS_SYNC_ALL_SHARES) | ||
80 | 98 | 90 | ||
81 | 99 | # pylint: disable=E0202 | 91 | # pylint: disable=E0202 |
82 | 100 | @defer.inlineCallbacks | 92 | @defer.inlineCallbacks |
83 | @@ -192,3 +184,23 @@ | |||
84 | 192 | settings = yield self.backend.file_sync_settings_info() | 184 | settings = yield self.backend.file_sync_settings_info() |
85 | 193 | 185 | ||
86 | 194 | self.process_info(settings) | 186 | self.process_info(settings) |
87 | 187 | |||
88 | 188 | def resizeEvent(self, event): | ||
89 | 189 | super(PreferencesPanel, self).resizeEvent(event) | ||
90 | 190 | size_bandwidth = (self.ui.bandwidth_settings.width() - | ||
91 | 191 | self.ui.upload_speed_spinbox.width() - | ||
92 | 192 | self.ui.kbps_label_1.width()) | ||
93 | 193 | padding = 160 # Padding to allow shrinking | ||
94 | 194 | size_sync = (self.ui.file_sync_settings.width() - padding) | ||
95 | 195 | force_wordwrap(self.ui.limit_uploads_checkbox, size_bandwidth, | ||
96 | 196 | SETTINGS_LIMIT_UPLOAD) | ||
97 | 197 | force_wordwrap(self.ui.limit_downloads_checkbox, size_bandwidth, | ||
98 | 198 | SETTINGS_LIMIT_DOWNLOAD) | ||
99 | 199 | force_wordwrap(self.ui.autoconnect_checkbox, size_sync, | ||
100 | 200 | SETTINGS_AUTO_CONNECT) | ||
101 | 201 | force_wordwrap(self.ui.udf_autosubscribe_checkbox, size_sync, | ||
102 | 202 | SETTINGS_SYNC_ALL_SHARES) | ||
103 | 203 | force_wordwrap(self.ui.share_autosubscribe_checkbox, size_sync, | ||
104 | 204 | SETTINGS_SYNC_ALL_FOLDERS) | ||
105 | 205 | force_wordwrap(self.ui.show_all_notifications_checkbox, size_sync, | ||
106 | 206 | SETTINGS_ALLOW_NOTIFICATIONS) | ||
107 | 195 | 207 | ||
108 | === modified file 'ubuntuone/controlpanel/gui/qt/tests/test_common.py' | |||
109 | --- ubuntuone/controlpanel/gui/qt/tests/test_common.py 2012-03-26 21:00:47 +0000 | |||
110 | +++ ubuntuone/controlpanel/gui/qt/tests/test_common.py 2012-04-17 18:12:18 +0000 | |||
111 | @@ -28,13 +28,72 @@ | |||
112 | 28 | GENERAL_ERROR_TITLE, | 28 | GENERAL_ERROR_TITLE, |
113 | 29 | GENERAL_ERROR_MSG, | 29 | GENERAL_ERROR_MSG, |
114 | 30 | ) | 30 | ) |
116 | 31 | from ubuntuone.controlpanel.gui.qt import uri_hook, handle_errors | 31 | from ubuntuone.controlpanel.gui.qt import ( |
117 | 32 | force_wordwrap, | ||
118 | 33 | handle_errors, | ||
119 | 34 | uri_hook, | ||
120 | 35 | ) | ||
121 | 32 | from ubuntuone.controlpanel.gui.qt.tests import ( | 36 | from ubuntuone.controlpanel.gui.qt.tests import ( |
122 | 33 | BaseTestCase, | 37 | BaseTestCase, |
123 | 34 | FakedDialog, | 38 | FakedDialog, |
124 | 35 | ) | 39 | ) |
125 | 36 | 40 | ||
126 | 37 | 41 | ||
127 | 42 | class ForceWordWrapTestCase(BaseTestCase): | ||
128 | 43 | """The test suite for the force_wordwrap.""" | ||
129 | 44 | |||
130 | 45 | @defer.inlineCallbacks | ||
131 | 46 | def setUp(self): | ||
132 | 47 | yield super(ForceWordWrapTestCase, self).setUp() | ||
133 | 48 | self.check = QtGui.QCheckBox() | ||
134 | 49 | |||
135 | 50 | def test_small_text_no_wordwrap(self): | ||
136 | 51 | """Set a small text, the widget shouldn't wordwrap.""" | ||
137 | 52 | text = u'small text' | ||
138 | 53 | force_wordwrap(self.check, 1000, text) | ||
139 | 54 | self.assertEqual(unicode(self.check.text()), text) | ||
140 | 55 | |||
141 | 56 | def test_size_invalid(self): | ||
142 | 57 | """The text shouldn't change if the size is not valid.""" | ||
143 | 58 | text = u'small text' * 20 | ||
144 | 59 | force_wordwrap(self.check, 0, text) | ||
145 | 60 | self.assertEqual(unicode(self.check.text()), text) | ||
146 | 61 | |||
147 | 62 | def test_size_too_small(self): | ||
148 | 63 | """The text shouldn't change if the size is too small.""" | ||
149 | 64 | text = u'small text' * 20 | ||
150 | 65 | force_wordwrap(self.check, 10, text) | ||
151 | 66 | self.assertEqual(unicode(self.check.text()), text) | ||
152 | 67 | |||
153 | 68 | def test_check_wrapping(self): | ||
154 | 69 | """The text should be splitted in several lines.""" | ||
155 | 70 | text = u'small text ' * 20 | ||
156 | 71 | font_metrics = QtGui.QFontMetrics(self.check.font()) | ||
157 | 72 | size = font_metrics.width('small text') | ||
158 | 73 | force_wordwrap(self.check, size, text) | ||
159 | 74 | expected = u'small text \n' * 20 | ||
160 | 75 | expected = expected[:-1] | ||
161 | 76 | self.assertEqual(unicode(self.check.text()), expected.rstrip()) | ||
162 | 77 | |||
163 | 78 | def test_check_wrapping_in_the_middle_of_word(self): | ||
164 | 79 | """The text should be splitted in several lines.""" | ||
165 | 80 | text = u'small text ' * 20 | ||
166 | 81 | font_metrics = QtGui.QFontMetrics(self.check.font()) | ||
167 | 82 | size = font_metrics.width('small text') | ||
168 | 83 | force_wordwrap(self.check, size, text) | ||
169 | 84 | expected = u'small text \n' * 20 | ||
170 | 85 | expected = expected[:-1] | ||
171 | 86 | self.assertEqual(unicode(self.check.text()), expected.rstrip()) | ||
172 | 87 | |||
173 | 88 | def test_no_word_wrap_valid(self): | ||
174 | 89 | """If a not valid wrap place is found, the word wrap is not made.""" | ||
175 | 90 | text = u'smalltext ' * 20 | ||
176 | 91 | font_metrics = QtGui.QFontMetrics(self.check.font()) | ||
177 | 92 | size = font_metrics.averageCharWidth() * 5 | ||
178 | 93 | force_wordwrap(self.check, size, text) | ||
179 | 94 | self.assertEqual(unicode(self.check.text()), text.rstrip()) | ||
180 | 95 | |||
181 | 96 | |||
182 | 38 | class UriHookTestCase(BaseTestCase): | 97 | class UriHookTestCase(BaseTestCase): |
183 | 39 | """The test suite for the uri_hook.""" | 98 | """The test suite for the uri_hook.""" |
184 | 40 | 99 | ||
185 | 41 | 100 | ||
186 | === modified file 'ubuntuone/controlpanel/gui/qt/tests/test_preferences.py' | |||
187 | --- ubuntuone/controlpanel/gui/qt/tests/test_preferences.py 2012-03-28 12:06:28 +0000 | |||
188 | +++ ubuntuone/controlpanel/gui/qt/tests/test_preferences.py 2012-04-17 18:12:18 +0000 | |||
189 | @@ -20,6 +20,8 @@ | |||
190 | 20 | 20 | ||
191 | 21 | from __future__ import division | 21 | from __future__ import division |
192 | 22 | 22 | ||
193 | 23 | import operator | ||
194 | 24 | |||
195 | 23 | from twisted.internet import defer | 25 | from twisted.internet import defer |
196 | 24 | 26 | ||
197 | 25 | from ubuntuone.controlpanel.gui.qt import preferences as gui | 27 | from ubuntuone.controlpanel.gui.qt import preferences as gui |
198 | @@ -297,3 +299,31 @@ | |||
199 | 297 | def test_tweak_speed_positive(self): | 299 | def test_tweak_speed_positive(self): |
200 | 298 | """Tweak speed properly: if param is positive, return kilobytes.""" | 300 | """Tweak speed properly: if param is positive, return kilobytes.""" |
201 | 299 | self.assertEqual(123456 * gui.KILOBYTES, gui.tweak_speed(123456)) | 301 | self.assertEqual(123456 * gui.KILOBYTES, gui.tweak_speed(123456)) |
202 | 302 | |||
203 | 303 | def test_resize_event(self): | ||
204 | 304 | """Check that the QCheckBox widgets receive the proper texts.""" | ||
205 | 305 | wrap_calls = {} | ||
206 | 306 | self.patch(gui, "force_wordwrap", | ||
207 | 307 | lambda check, size, text: operator.setitem( | ||
208 | 308 | wrap_calls, check, (size, text))) | ||
209 | 309 | self.ui.setFixedWidth(1000) | ||
210 | 310 | size_bandwidth = (self.ui.ui.bandwidth_settings.width() - | ||
211 | 311 | self.ui.ui.upload_speed_spinbox.width() - | ||
212 | 312 | self.ui.ui.kbps_label_1.width()) | ||
213 | 313 | padding = 160 # Left and Right Padding | ||
214 | 314 | size_sync = (self.ui.ui.file_sync_settings.width() - padding) | ||
215 | 315 | self.ui.show() | ||
216 | 316 | self.addCleanup(self.ui.hide) | ||
217 | 317 | self.assertEqual(wrap_calls[self.ui.ui.limit_uploads_checkbox], | ||
218 | 318 | (size_bandwidth, gui.SETTINGS_LIMIT_UPLOAD)) | ||
219 | 319 | self.assertEqual(wrap_calls[self.ui.ui.limit_downloads_checkbox], | ||
220 | 320 | (size_bandwidth, gui.SETTINGS_LIMIT_DOWNLOAD)) | ||
221 | 321 | self.assertEqual(wrap_calls[self.ui.ui.autoconnect_checkbox], | ||
222 | 322 | (size_sync, gui.SETTINGS_AUTO_CONNECT)) | ||
223 | 323 | self.assertEqual(wrap_calls[self.ui.ui.udf_autosubscribe_checkbox], | ||
224 | 324 | (size_sync, gui.SETTINGS_SYNC_ALL_SHARES)) | ||
225 | 325 | self.assertEqual(wrap_calls[self.ui.ui.share_autosubscribe_checkbox], | ||
226 | 326 | (size_sync, gui.SETTINGS_SYNC_ALL_FOLDERS)) | ||
227 | 327 | self.assertEqual( | ||
228 | 328 | wrap_calls[self.ui.ui.show_all_notifications_checkbox], | ||
229 | 329 | (size_sync, gui.SETTINGS_ALLOW_NOTIFICATIONS)) |
I force the strings to be longer (repeating the text twice) and this is how it looks now with this code:
http:// ubuntuone. com/6avGquAa62T NcSQ5GB7n7G