Merge lp://qastaging/~diegosarmentero/ubuntuone-control-panel/checkwrap into lp://qastaging/ubuntuone-control-panel

Proposed by Diego Sarmentero
Status: Merged
Approved by: Manuel de la Peña
Approved revision: 317
Merged at revision: 313
Proposed branch: lp://qastaging/~diegosarmentero/ubuntuone-control-panel/checkwrap
Merge into: lp://qastaging/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://qastaging/~diegosarmentero/ubuntuone-control-panel/checkwrap
Reviewer Review Type Date Requested Status
Manuel de la Peña (community) Approve
Roberto Alsina (community) Approve
Review via email: mp+102152@code.qastaging.launchpad.net

Commit message

Adding a word_wrap function to use it in those cases where the widget don't implement a wrapping functionality.

To post a comment you must log in.
Revision history for this message
Diego Sarmentero (diegosarmentero) wrote :

I force the strings to be longer (repeating the text twice) and this is how it looks now with this code:

http://ubuntuone.com/6avGquAa62TNcSQ5GB7n7G

314. By Diego Sarmentero

Fixing lint issue.

315. By Diego Sarmentero

reverting code for example.

Revision history for this message
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://en.wikipedia.org/wiki/Word_wrap#Minimum_length and implement something similar that does not force us to have a loop inside an other.

review: Needs Fixing
Revision history for this message
Roberto Alsina (ralsina) wrote :

+1 and sharing mandel's algorithm caveat

review: Approve
316. By Diego Sarmentero

Improve wrapping algorithm, fix tests.

Revision history for this message
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://en.wikipedia.org/wiki/Word_wrap#Minimum_length and implement something
> similar that does not force us to have a loop inside an other.

Done

Revision history for this message
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('gatox'). Can you please do that.

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://wiki.python.org/moin/PythonSpeed/PerformanceTips#String_Concatenation I would at least move to .join instead of +=.

review: Needs Fixing
317. By Diego Sarmentero

Improving algorithm.

Revision history for this message
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('gatox'). Can you please do that.
>

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://wiki.python.org/moin/PythonSpeed/PerformanceTips#String_Concatenation I
> would at least move to .join instead of +=.

Changed.

Revision history for this message
Manuel de la Peña (mandel) wrote :

Great!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches