Merge lp://qastaging/~sam92/openlp/bug-1695620 into lp://qastaging/openlp

Proposed by Samuel Mehrbrodt
Status: Merged
Merged at revision: 2854
Proposed branch: lp://qastaging/~sam92/openlp/bug-1695620
Merge into: lp://qastaging/openlp
Diff against target: 724 lines (+284/-155)
6 files modified
openlp/core/lib/serviceitem.py (+5/-3)
openlp/core/ui/printserviceform.py (+5/-5)
openlp/plugins/songs/lib/mediaitem.py (+40/-36)
openlp/plugins/songs/lib/songstab.py (+72/-37)
openlp/plugins/songs/songsplugin.py (+55/-5)
tests/functional/openlp_plugins/songs/test_mediaitem.py (+107/-69)
To merge this branch: bzr merge lp://qastaging/~sam92/openlp/bug-1695620
Reviewer Review Type Date Requested Status
Tim Bentley Pending
Raoul Snyman Pending
Tomas Groth Pending
Review via email: mp+365702@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2019-02-21.

Commit message

Make footer configurable

Description of the change

Make footer configurable

- Make use of Mako for footer generation, configurable in song settings
- removed now obsolete and via template better configurable options to display "songbook", "written by" and "copyright" information in footer
- added explanation box for so far used settings as Mako placeholders
- added songs configuration setting for template including reset button
- added default template replacing currently existing configuration as best as possible (should be backwards compatible or at least be adaptable to correspond to former settings)
- write and adapt tests for new and removed functionality
- Added some more available fields in the footer: Alternate Title, CCLI Number, Topics, Authors (all music, all words)

To post a comment you must log in.
Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

You need to run CI on this to show the success of tests.
Not sure why but this request did a full download when I did a check out. Also cannot run due to chord issues.
Cannot run this but there seem to be lots of line breaks in the footer display. As the footer area is small this will not be displayed and lead to forum comments.
You need some validation do defend against too long footers!

review: Needs Fixing
Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

We're moving away from Python generated HTML, and using a JS system based on Reveal.js. Come chat with tgc and myself in IRC.

review: Disapprove
Revision history for this message
Samuel Mehrbrodt (sam92) wrote : Posted in a previous version of this proposal

> Cannot run this but there seem to be lots of line breaks in the footer display. As the footer area is small this will not be displayed and lead to forum comments.

The footer has the same height as before, there should be no difference by default.

So I replaced Pystache with Mako. Should be safe to merge now I think.
As the new renderer is in a very early state atm, I can't see how I can adopt this for the new renderer. Maybe we can merge this now and adopt when the new renderer is ready?

Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

Blank lines!
Will review tomorrow as I have bible study!

review: Needs Fixing
Revision history for this message
Samuel Mehrbrodt (sam92) wrote : Posted in a previous version of this proposal

So is there a chance this can be reviewed now?

> Blank lines!
What do you mean with that?

Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

Linux tests passed!

Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

Linting passed!

Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

macOS tests passed!

Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

Linux tests passed!

Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

Linting passed!

Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

macOS tests passed!

Revision history for this message
Tomas Groth (tomasgroth) wrote : Posted in a previous version of this proposal

Looks good to me!

review: Approve
Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

Template should be in its own file so can be tested and not duplicated.
Not happy with the if conditions being removed these have been added as we sometimes get broken databases in the wild and this needs to be guarded against.

review: Needs Fixing
Revision history for this message
Samuel Mehrbrodt (sam92) wrote : Posted in a previous version of this proposal

@Tim: As discussed on IRC, could you please review again?
I had to remove the conditions so that the variables are always available to the template, even if they are empty. They were there to make sure that no empty lines were added to the footer. But now this checking is done in the template.

Also wrt the template in its own file, after discussion we came to agreement that it's not worth the hassle (packaging etc). So we keep it just as a config string.

Revision history for this message
Tim Bentley (trb143) : Posted in a previous version of this proposal
review: Approve
Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

So, I like the principle, but I really don't think that using Mako is necessary. Do you think it's plausible to just use string formatting or string templating?

review: Needs Information
Revision history for this message
Samuel Mehrbrodt (sam92) wrote : Posted in a previous version of this proposal

> Do you think it's plausible to just use string formatting or string templating?

Hm while that would allow adding a bit of markup around the strings, you can't add any control structures.

With mako you can do much more, e.g. concatenate multiple authors into one line (as we do using join), display them each on it own line.

The use case I'm especially interested in: Pick just one songbook to be displayed. Using Mako I can just write Python code to only show the songbook we are actually using in the service.

I'm sure there are more valid use cases for this.

Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

You can do the same things in Python. The thing is, we no longer use Mako anywhere else. It's such a small usage, I'd really like it if we could remove the dependency.

I know we're trying to make a customisable footer, but is there any way we can do it without Mako and without introducing another dependency?

Look, if there's no other way around it, then I'll approve this, I'm just trying to reduce/minimize dependencies where possible.

Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

I'm getting conflicts when merging with trunk, can you just re-merge trunk and fix the conflicts please?

review: Needs Fixing
Revision history for this message
Samuel Mehrbrodt (sam92) wrote :

Merged trunk.

I couldn't find a better way to do this without Mako or another template engine.

Revision history for this message
Raoul Snyman (raoul-snyman) wrote :

Linux tests passed!

Revision history for this message
Raoul Snyman (raoul-snyman) wrote :

Linting passed!

Revision history for this message
Raoul Snyman (raoul-snyman) wrote :

macOS tests passed!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.