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.
_______________________________________________
Mailing list: https://launchpad.net/~openlp-core
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openlp-core
More help : https://help.launchpad.net/ListHelp
