Edit Service Item

= Editing items in services without updating the database = Sometimes it is useful to edit songs or other service items in a service without affecting the database. A simple case of this is when we want to omit some verses, or change their order for a single service. Another case I have experienced is for the worship leader to change “we/us/our” to “I/me/my”, in order to obtain a more personal ethos from a song with a corporate one (or vice-versa). Currently, OpenLP will always edit the song in the database whether this is activated from the Media Manager, Preview or Service Manager. There are workarounds such as duplicating the song, or editing it, adding to the service, then reverting it. However these are prone to error if steps are omitted or reverted, and do not truly allow editing of a service item separately from the database. Other lyric projection packages provide means to override some of the song's properties for a specific instance, e.g OpenSong's “custom verse order” edit box.

Initial approach
Initially I made a preparation “verse_order_override” that would add a second verse order box to the EditSongForm. However the UI changes were not liked and the means of transferring the extra data were felt not to be in accordance with the design philosophy. Furthermore, this only addresses when the verse order should change, and so does not scale to other aspects a song or to other types of service item.

Feedback
While the above approach was largely rejected, on #openlp the project manager superfly declared it should be the case that when you edit a song in a service you edit only that instance, not the database. While this was not universally agreed as some felt it might lead to confusion,. ..

A new approach
. . . a suggestion from ArjanS on #openlp that was well received was to have the SongEditForm display a checkbox when editing which would say “Also save to database” or similar. If checked then both the service and the database would be updated; if unchecked then only the service would be affected.

''The challenge is the song edit form is designed to work from the database and the service only contains a cut down version of the song! The song is stored in the service item in xml for export and import into a different song database but this is not compatible with song editing it needs to be saved into the database first (hence temporary songs for songs which are loaded from services but not wanted to be kept). TRB143''

Since a longer term goal is to refactor the code base to split the UI from the implementation code, would it make sense to first change this so that the form works on a Song item, and leave the database/service interaction to the manager?

For discussion
While Stewart Becker is willing to do the legwork of coding this, it needs input from those more familiar with the OpenLP codebase and design philosophy before implementing it. The following are some of the questions that need to be answered and some possible resolutions. Please add others, and especially expound on the pros and cons of each approach.

Editing from the Media Manager
When editing a song from the Media Manager, there is no service item that is being edited, only the database entry. How should the checkbox behave in this case?
 * It is simply ignored
 * Pros: fairly easy to do.
 * Cons: possibly (probably?) very confusing for users
 * The database is only actually updated if it is checked, as for editing from the Service Manager
 * Pros: easy to do.
 * Cons: possibly (probably?) confusing for users
 * It is invisible, i.e. the form looks the same as it does at present
 * Pros: mimics existing behaviour. Gives a visual difference between different editing modes.
 * Cons: Can't think of any
 * It is visible but disabled, locked to “on”
 * Pros: explicit what is going on.
 * Cons: Is this confusing?

Chosen approach: Hide it TRB143 and sibecker

Default behaviour
What should the value of the checkbox be each time the EditSongForm is displayed?.
 * Checked:
 * Pros: very easy to do, mimics current behaviour closely
 * Cons: Users updating several service items have to uncheck each time.
 * Unchecked
 * Pros: very easy to do
 * Cons: diverges from current behaviour.  Users wanting to update service and database on several items have to check each time
 * The same as whatever it was last time, saved in OpenLP config between sessions
 * Pros: users have more control
 * Cons: More work. May diverge from current behaviour.  Still requires an initial decision on parameter the first ever time this feautre is loaded.

Chosen approach: not yet decided

Managing separate different instances
When the song in the service is saved, how will the EditSongForm manage saving to two different instances of the song?
 * It takes in two variables where is currently takes in only one (song_id) for the two items. If one is None then it implies there is no temporary / permanent database entry.  They can't both be None.
 * Pros: Close to current API
 * Cons: Similar approach previously rejected
 * It operates on a single Song object, and the decision of where to save this and how many times is handled further up the chain
 * Pros: Easier to write test as it helps separate UI from implementation (a long term goal)
 * Cons: Diverges from how the rest of OpenLP works?
 * Make the decision before the song is edited. Question of editing permenent/temporary copy is asked when edit is requested and at that point a temporary copy made and used for the service for that instance ofthe song
 * Pros: The form is built around the database - other approaches would need a big re-write to change this.
 * Cons: Breaks link back to permanent copy so subsequent edits can't be written back to permanent copy. Diverges from current user experience, even if under-the-hood is similar.  Rewrite needed for long-term goal of UI/implementation separation anyway.

Chosen approach: not yet decided

Storing both instances in the service item
Currently, the service item holds a single edit_id to refer into the database. A temporary copy of a song may be entered into the database to hold changes, as is currently done when a service file is loaded containing a song not in the database. How should the service item refer to these two items
 * A new variable, e.g. temp_id, is added referring to the temporary copy, and edit_id refers to the permanent entry in the database
 * Pros: Does the job fairly simply
 * Cons: Need to go through existing code carefully to be sure where edit_id should become temp_id
 * A new variable, e.g. perm_id, is added referring to the permanent database entry, and edit_id refers to the temporary copy
 * Pros: Does the job fairly simply
 * Cons: Need to go through existing code carefully to be sure where edit_id should become perm_id
 * The service item holds a Song object in addition to the edit_id, which refers to the permanent copy.
 * Pros: Can't think of any
 * Cons: Big divergence from current behaviour

Chosen approach: not yet decided

Making temporary copies
OpenLP supports putting a temporary copy of a song into the database. This is done, for instance,. If songs in services can be updated separately to database entries, these will need to be used more, but how much more?
 * A temporary copy is always added when the song is added to the service (by user or as part of loading a file)
 * Pros: easier to implement
 * Cons: Do we need two copies when user wants to use the database entry without changes
 * The song referred to by the service item is only a temporary copy when it actually differs from the database copy, and this is detected whenever the song is added or updated in the service manager. (It could be as simple as testing the state of the checkbox, rather than a “proper” comparison)
 * Pros: May save memory if unused temporary songs get deleted whenever temporary copy reverts to the same as permanent
 * Cons: More work. Dealing with the case where a song has been entered in a service twice is very hard

Chosen approach: not yet decided

Other item types
The above only addresses how this change will affect songs, but they are not the only items that can be edited from a service. Currently the only other item type that can be edited this way are custom items, but how should they be affected?
 * Nothing should change, this should affect on songs only
 * Pros: Less work
 * Cons: Makes user experience of editing different types of service item diverge
 * Custom items should work in a similar way, so that they all have a common means of editing them.
 * Pros: Makes user experience the same across all items editable from service manager
 * Cons: More work
 * All items should work in the same way under the surface, whether the UI enables editing them from the service or not, as that could change in the future.
 * Pros: Unifies user experience, and code interface also
 * Cons: A lot of work. No immediate tangible benefit in many item types

Chosen approach: not yet decided

Other things for consideration
The above list of things to consider may well be incomplete. What else needs to be brought under discussion or specified prior to coding the feature

--Stewart Becker 19:47, 15 September 2013 (SAST)