openlp-core team mailing list archive

[Andreas Preikschat <andreaspreikschat@xxxxxxxxxx>]

Review: Needs Fixing
I think it's better when you do not mix two different things in one branch/proposal. 

1) Song Usage changes
The challenging part is missing here. It has to be distinguished if a song was printed and/or displayed. If we do not distinguish we actually do not help the user. Also the song should only be added to the records when the lyrics are printed.

2) Image changes
When I try to replace the image background I get this traceback:

Traceback (most recent call last):
  File "/home/andreas/Projekte/openlp/features4/openlp/plugins/images/lib/mediaitem.py", line 217, in onReplaceClick
    filename):
  File "/home/andreas/Projekte/openlp/features4/openlp/core/ui/maindisplay.py", line 235, in directImage
    self.imageManager.add_image(name, path)
TypeError: add_image() takes exactly 5 arguments (3 given)

I looked at the html structure and I am asking myself if this is the correct way. When I display an image, then we actually have the background image and the image we want to display above. Both images can have a different boarder colour. Image the following case: You display a song with a image based theme. Then you replace the background image. Which boarder colour should we use?

Do you see what I mean?

I think it should be done as follows:
1) Do not add boarder to the images (at all, instead set a background colour).
2) Remove the second image from the html.
3) Remove the (boarder colour from the) image tab

Of course this means, that images are no longer not "themeable" items. Well, if you are precious adding the image settings tab is also a way of making images item themeable.

For:
1) Possible performance improvement when displaying images (as the "background" image does not need to be loaded).
2) General performance improvement when dealing with images whose ratio is not equal to the screen's ratio (as we do not have to add the boarder).

Against:
1) hm... quite a lot to change.
2) Side effects; I do not know if there is something which has to be there (but I am suggesting to remove it).

But actually we could split this up in to parts (and actually I ONLY consider the first part a must in regard to the feature you are implementing):
1) Fix things in the branch (remove the image tab and use the theme setting)
2) Remove the second image and remove the need to add boarder to the images.

Feel free to share your thoughts...
-- 
https://code.launchpad.net/~trb143/openlp/features4/+merge/72352
Your team OpenLP Core is subscribed to branch lp:openlp.