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