Qreator

Merge lp://staging/~stefan-schwarzburg/qreator/qreator_printing into lp://staging/qreator

qreator_printing
Merge into trunk

Proposed by Schwarzburg on 2012-11-22

Status:	Merged
Merged at revision:	109
Proposed branch:	lp://staging/~stefan-schwarzburg/qreator/qreator_printing
Merge into:	lp://staging/qreator
Diff against target:	255 lines (+85/-15) (has conflicts) 2 files modified data/ui/QreatorWindow.ui (+19/-0) qreator/QreatorWindow.py (+66/-15) Text conflict in qreator/QreatorWindow.py
To merge this branch:	bzr merge lp://staging/~stefan-schwarzburg/qreator/qreator_printing
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
David Planella		2012-11-22	Approve on 2012-11-23
Review via email: mp+135636@code.staging.launchpad.net

Description of the change

This branch adds basic printing capability to qreator.

A "print" icon is added to the bottom toolbar. Clicking will open the native print dialog.

"Basic" printing because: the code does not customize the pagesize, resolution, etc... But the defaults should be sane enough.

The icon used is: printer-symbolic anthough we agree that this is not the nicest icon :-)

Revision history for this message

David Planella (dpm) wrote on 2012-11-23:

It looks good to me, thanks!

Just a couple of remarks:

* Why is 'credits' replaced with 'thecredits', was it clashing with some keyword or some other variable?

thecredits = _("translator-credits").replace('https', 'http')

* No need to change anything or resubmit the merge proposal now, but in the future, would you mind leving out changes such as whitespace and minor fixes from merge proposals? It makes the diff a lot bigger and makes it difficult to concentrate on reviewing the actual feature. I'm happy for you to commit these minor fixes to trunk directly without review, so that they can get out of the way in the diffs from merge proposals.

Thanks!

review: Needs Information

Revision history for this message

Schwarzburg (stefan-schwarzburg) wrote on 2012-11-23:

> It looks good to me, thanks!
>
> Just a couple of remarks:
>
> * Why is 'credits' replaced with 'thecredits', was it clashing with some
> keyword or some other variable?
>
> thecredits = _("translator-credits").replace('https', 'http')
>

Yes, credits was clashing. The error message from pylint is: "Redefining built-in 'credits'".
I thought (although this was not the best branch to do it), that we should get rid of ALL pylint and pep8 warnings and errors, so that everything will be easier with the test-suit.

> * No need to change anything or resubmit the merge proposal now, but in the
> future, would you mind leving out changes such as whitespace and minor fixes
> from merge proposals? It makes the diff a lot bigger and makes it difficult to
> concentrate on reviewing the actual feature. I'm happy for you to commit these
> minor fixes to trunk directly without review, so that they can get out of the
> way in the diffs from merge proposals.
>

Sure, sorry.

> Thanks!

Revision history for this message

David Planella (dpm) wrote on 2012-11-23:

Al 23/11/12 14:06, En/na Schwarzburg ha escrit:
>> It looks good to me, thanks!
>>
>> Just a couple of remarks:
>>
>> * Why is 'credits' replaced with 'thecredits', was it clashing with some
>> keyword or some other variable?
>>
>> thecredits = _("translator-credits").replace('https', 'http')
>>
>
> Yes, credits was clashing. The error message from pylint is: "Redefining built-in 'credits'".
> I thought (although this was not the best branch to do it), that we should get rid of ALL pylint and pep8 warnings and errors, so that everything will be easier with the test-suit.
>

Makes sense. Just nit-picking (I should have actually chosen that name
myself), but before merging, would you mind renaming the variable to
'translator_credits'? Thanks

>> * No need to change anything or resubmit the merge proposal now, but in the
>> future, would you mind leving out changes such as whitespace and minor fixes
>> from merge proposals? It makes the diff a lot bigger and makes it difficult to
>> concentrate on reviewing the actual feature. I'm happy for you to commit these
>> minor fixes to trunk directly without review, so that they can get out of the
>> way in the diffs from merge proposals.
>>
>
> Sure, sorry.
>

No worries :).

Cheers,
David.

review approve

review: Approve

lp://staging/~stefan-schwarzburg/qreator/qreator_printing updated on 2012-11-23