openlp-core team mailing list archive

[Tomas Groth <tomasgroth@xxxxxxxx>]

Review: Needs Fixing

Hi MBernis,

Thanks for contributing to OpenLP! I think this is your first merge request so I'll just start out by saying that your code (not you!) most likely will meet criticism and you will need to add/rewrite some of it before it can be merged. This can be a long process when adding new features, but I hope you will get through it! :)

As I mentioned in my reply to your email on the mailinglist you should add the "web" code needed for rendering the chords to make the chords actually work with this branch.
How does the chords look when you try to print a service while showing the content? I think it would be a good idea to be able to toggle showing chords or not in printouts.

Some users on the forum has "hacked" chords into OpenLP using formatting tags, don't know if you've already seen it, but if not have a look: http://forums.openlp.org/discussion/2544/chords-in-openlp

And last but not least: Tests. Nothing new gets added to OpenLP without some tests that proves that the code works as expected, so you'll have to write some tests. Take a look at the existing tests to see how it can be done.
-- 
https://code.launchpad.net/~mbernis/openlp/chordpro_format/+merge/271233
Your team OpenLP Core is requested to review the proposed merge of lp:~mbernis/openlp/chordpro_format into lp:openlp.