Merge lp:~ohad-kammar/qbzr/configurable-colours into lp:~qbzr-dev/qbzr/trunk

Proposed by Ohad Kammar
Status: Merged
Merged at revision: not available
Proposed branch: lp:~ohad-kammar/qbzr/configurable-colours
Merge into: lp:~qbzr-dev/qbzr/trunk
To merge this branch: bzr merge lp:~ohad-kammar/qbzr/configurable-colours
Reviewer Review Type Date Requested Status
Gary van der Merwe Approve
Review via email: mp+1278@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Ohad Kammar (ohad-kammar) wrote :

This merge depends on the following merge (and indeed branched from it):
https://code.edge.launchpad.net/~ohad-kammar/qbzr/config-bugfix/+merge/1277

This is a fix to bug #280041:
https://bugs.edge.launchpad.net/qbzr/+bug/280041

Problems with this fix:
1. I am not familiar with this project's code, so you may find the fix's implementation inconsistent with the usual programming practices of the team.
2. I didn't know how to handle errors correctly, so I implemented the mechanism for detecting them, but it doesn't do anything apart from ignoring an erroneous input and using the default instead.

Revision history for this message
Alexander Belchenko (bialix) wrote :

Thanks for your attempt. The idea is mostly correct, but implementation needs some changes.
1) function to get color values from config should be placed to util.py, preferable as QBzrConfig method.
2) ignoring error is fine, all we can do is to write corresponding message to .bzr.log using mutter() function from bzrlib.trace module
3) don't use apply() function, it's obsolete. Use QtGui.QColor(*colour_components) instead.
4) replacement_text_background should be placed at the top of the module, around colors dict definition.
5) replacement_text_background is really bad name and it's not very precise. Actually it's about interline changes highlighting.

Also we have forum/ML at google: http://groups.google.com/group/qbzr
If you wish we can discuss your code there.

515. By Ohad Kammar

 Moved the utility function colour_value_to_RGB() into QBzrConfig, and changed its name to getColour().
While doing so, added more elaborate error messages to the raise ErrorValue statements.

516. By Ohad Kammar

Added a missing space to error message.

517. By Ohad Kammar

 Added error handling to reading colours.

518. By Ohad Kammar

 Replaced the `apply' invokation with *argument.

519. By Ohad Kammar

 Moved `the replacement_text_background' default value definition upwards, near the definition for the colors dictionary.

520. By Ohad Kammar

 Replaced the horrible name `replacement_text_background' with the more precise `interline_changes_background'.

Revision history for this message
Ohad Kammar (ohad-kammar) wrote :

Right. Good points.

I incorporated as follows:
point 1 implemented in rev. #516.
point 2 implemented in rev. #517.
point 3 implemented in rev. #518 (ignore the spelling mistake :D)
point 4 implemented in rev. #519.
point 5 implemented in rev. #520.

Cheers!
Ohad.

Revision history for this message
Lukáš Lalinský (luks) wrote :

I know you are going to hate me :) but please change colours to colors, both in the configuration group and the code. US English is more or less standard in open source programs.

Revision history for this message
Ohad Kammar (ohad-kammar) wrote :

8-0 Are you serious?

521. By Ohad Kammar

 Code migration over the Atlantic.

Revision history for this message
Ohad Kammar (ohad-kammar) wrote :

Done. Code is now colourless.

Revision history for this message
Gary van der Merwe (garyvdm) wrote :

It seems this was forgot :-(

I've had a look at the code - I'm happy with it, and Alexander and Lukáš comments have been addressed. So I have merge it.

review: Approve
Revision history for this message
Gary van der Merwe (garyvdm) wrote :

Ohad: Please will you do one more thing: Please document the config options you have added in README.txt so that it is easy to find them for users.

Subscribers

People subscribed via source and target branches

to status/vote changes: