openlp-core team mailing list archive

[Stewart Becker <stewart@xxxxxxxxxxxxxx>]

On Tue, 2013-09-10 at 21:06 +0000, Tim Bentley wrote:
> Review: Needs Fixing
> 
> Before going on with this change please can you email the core mailing list what the change is about and how you propose to fix it.
> The change seems to be adding spaghetti code into code for songs and this is not part of the architecture.
> Moving data around the system can only be done by a service item and not text strings.
> There is already a way to make temporary changes to songs by cloning them why is that not acceptable / extendable.
> 
> It's good to see new ideas and improvements coming to OpenLP but we need to keep to code base maintainable.  
>  

Tim,

This is my first time with python so it's unlikely to be pretty.  Please
be patient with me.  I've explained the rationale behind what I've done
below.  If you think the same can be accomplished in a way that fits
better with the existing architecture, please let me know your thoughts
on that design.

The intention is to allow users to have a verse order, separate from
that in the database, which overrides for a single instance the verse
order for a song in a service, without needing to change the database
copy - even temporarily.  Making multiple copies of the same song in the
database is not a viable "solution" to this problem.  The ability to
override a song's verse order is a feature that other presentation
software such as OpenSong already has, and has been requested by others,
e.g.
http://forums.openlp.org/discussion/1842/new-feature-selective-versing
(although 
I've been discussing my intent to do this on #openlp.

I couldn't find a natural place to put this verse order override
information that was accessible for both reading/writing to a file and
for the song edit form.  It doesn't, and shouldn't, go in the database.
Ideally I'd like to add a property to some object called
'verse_order_override' that gets set in the EditSongForm.  However I
couldn't determine which object to use: firstly the EditSongForm
operates on the database item Song, via the song_id key, rather than on
ServiceItem, thus information that is relevant only to a service item
and not to a Song needs to be passed separately.  ServiceItem itself
seems too generic to contain verse information, though I did notice
exiting verseTag logic in here.  (Aside: why is this logic here, if this
is a generic service item?  Surely only songs have verse tags?)  Also,
the verse order information might be useful in future when users have
done the workaround of copying the song and changing the verse order in
the database, to identify which of the duplicate copies it should refer
to, hence why I added this field to the ServiceItem's data_string
dictionary.  That way non-song service items won't have to care about a
property that is not relevant to them.

As for passing data round in a string, if you are referring to the
parameter data_string, it is actually a dictionary.  You say that
"Moving data around the system can only be done by a service item" but
this is not what the code does: SongMediaItem.on_remote_edit currently
takes a song_id (an int, not a service item), which it uses as a
database key.  Since the verse order override data lives outside the
database, another parameter is needed.  My choice to pass a dictionary
as a second argument is extensible if future changes want to pass in
other data.  There are other options, but this seems to me the lightest
touch, following the principle of "the simplest thing that will work",
and allows for the same function signature to be used elsewhere too such
as in CustomMediaItem.

EditSongForm goes even further: it doesn't return an object at all but
saves directly to the database.  Information that lives outside the
database in only the service item must be supplied and extracted
separately.  At this point the only option is to use the string from the
widget.  EditSongForm.load_song could indeed take the data_string
dictionary too when loading, but I don't think that's really what you
want.  NB The internal property EditSongForm.verse_order_override is a
boolean indicating the "mode" of the EditSongForm, though the variable
name could possibly be better named (suggestions welcome).

There are ways I can think of to address the above, but they involve a
significant refactoring effort, which is not something I wanted to do on
my first foray into this project, and indeed python itself.  I'd hoped
to make my changes as light-touch as possible.  As I say above, if you
have thoughts on an approach that better fits the existing design,
please detail them.

I'm not sure what the address is for the "core" development team: I know
of openlp-devel@xxxxxxxxxxxxxxxxxxxxx but even I get mail sent there,
and don't want to spam everyone with this.  Perhaps you would be kind
enough to pass it on if they wouldn't all get this mail?

Stewart


-- 
https://code.launchpad.net/~stewart-e/openlp/verse_order_override/+merge/184651
Your team OpenLP Core is subscribed to branch lp:openlp.