OpenLP

Merge lp://qastaging/~alisonken1/openlp/strings-templates2 into lp://qastaging/openlp

  1. strings-templates2
  2. Merge into trunk
Proposed by Ken Roberts
Status: Merged
Approved by: Tim Bentley
Approved revision: 2680
Merged at revision: 2675
Proposed branch: lp://qastaging/~alisonken1/openlp/strings-templates2
Merge into: lp://qastaging/openlp
Diff against target: 923 lines (+409/-398)
5 files modified
openlp/core/lib/htmlbuilder.py (+224/-226)
openlp/core/ui/firsttimeform.py (+3/-3)
openlp/plugins/bibles/forms/editbibledialog.py (+1/-1)
openlp/plugins/songs/lib/mediaitem.py (+1/-1)
tests/functional/openlp_core_lib/test_htmlbuilder.py (+180/-167)
To merge this branch: bzr merge lp://qastaging/~alisonken1/openlp/strings-templates2
Reviewer Review Type Date Requested Status
Tim Bentley Approve
Tomas Groth Approve
Review via email: mp+296870@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2016-06-08.

Commit message

Convert htmlbuilder.py to use Template() strings for css

Description of the change

Convert htmlbuilder.py to use Template() strings for css

- Changed strings to use Template() instead of format() for css building
- Fix htmlbuilder strings tests
- Cleanup template variables and fix tests to match
- bug 1590386 - string format error in editbibledialog.py
- bug 1590475 - string format error in mediaitem.py
- Function call error in firsttimeform.py (missed label text variable)

--------------------------------
lp:~alisonken1/openlp/strings-templates2 (revision 2679)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1609/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1520/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1458/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1230/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/820/
[SUCCESS] https://ci.openlp.io/job/Branch-05a-Code_Analysis/888/
[SUCCESS] https://ci.openlp.io/job/Branch-05b-Test_Coverage/756/

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

Looks good but one comment which is general across all the code.

review: Needs Fixing
Revision history for this message
Ken Roberts (alisonken1) wrote : Posted in a previous version of this proposal

Since this is HTML/css source and not just any text variable I followed the
formatting the original HTML SRC. Has something changed since the original
was put in?
On Jun 6, 2016 9:12 AM, "Tim Bentley" <email address hidden> wrote:

> Review: Needs Fixing
>
> Looks good but one comment which is general across all the code.
>
> Diff comments:
>
> > === modified file 'openlp/core/lib/htmlbuilder.py'
> > --- openlp/core/lib/htmlbuilder.py 2016-05-17 13:21:29 +0000
> > +++ openlp/core/lib/htmlbuilder.py 2016-06-06 15:26:06 +0000
> > @@ -538,15 +538,60 @@
> > </script>
> > </head>
> > <body>
> > -<img id="bgimage" class="size" %s />
> > -<img id="image" class="size" %s />
> > -%s
> > +<img id="bgimage" class="size" ${bg_image} />
> > +<img id="image" class="size" ${image} />
> > +${html_additions}
> > <div class="lyricstable"><div id="lyricsmain" style="opacity:1"
> class="lyricscell lyricsmain"></div></div>
> > <div id="footer" class="footer"></div>
> > <div id="black" class="size"></div>
> > </body>
> > </html>
> > -"""
> > +""")
> > +
> > +LYRICS_SRC = Template("""
> > +.lyricstable {
> > + z-index: 5;
> > + position: absolute;
> > + display: table;
> > + ${stable}
> > +}
> > +.lyricscell {
> > + display: table-cell;
> > + word-wrap: break-word;
> > + -webkit-transition: opacity 0.4s ease;
> > + ${lyrics}
> > +}
> > +.lyricsmain {
> > + ${main}
> > +}
> > +""")
> > +
> > +FOOTER_SRC = Template("""
>
> Indent please.
> No real rule about indentation but Lyrics_src looks better!
>
> > +left: ${left}px;
> > +bottom: ${bottom}px;
> > +width: ${width}px;
> > +font-family: ${family};
> > +font-size: ${size}pt;
> > +color: ${color};
> > +text-align: left;
> > +white-space: ${space};
> > +""")
> > +
> > +LYRICS_FORMAT_SRC = Template("""
> > +${justify}word-wrap: break-word;
> > +text-align: ${align};
> > +vertical-align: ${valign};
> > +font-family: ${font};
> > +font-size: ${size}pt;
> > +color: ${color};
> > +line-height: ${line}%;
> > +margin: 0;
> > +padding: 0;
> > +padding-bottom: ${bottom};
> > +padding-left: ${left}px;
> > +width: ${width}px;
> > +height: ${height}px;${font_style}${font_weight}
> > +""")
> >
> >
> > def build_html(item, screen, is_live, background, image=None,
> plugins=None):
>
>
> --
>
> https://code.launchpad.net/~alisonken1/openlp/strings-templates2/+merge/296567
> You are the owner of lp:~alisonken1/openlp/strings-templates2.
>

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

No it just looks a bit cramped and could do with a visit to a beauty spar😁
On 7 Jun 2016 1:42 a.m., "Ken Roberts" <email address hidden> wrote:

> Since this is HTML/css source and not just any text variable I followed the
> formatting the original HTML SRC. Has something changed since the original
> was put in?
> On Jun 6, 2016 9:12 AM, "Tim Bentley" <email address hidden> wrote:
>
> > Review: Needs Fixing
> >
> > Looks good but one comment which is general across all the code.
> >
> > Diff comments:
> >
> > > === modified file 'openlp/core/lib/htmlbuilder.py'
> > > --- openlp/core/lib/htmlbuilder.py 2016-05-17 13:21:29 +0000
> > > +++ openlp/core/lib/htmlbuilder.py 2016-06-06 15:26:06 +0000
> > > @@ -538,15 +538,60 @@
> > > </script>
> > > </head>
> > > <body>
> > > -<img id="bgimage" class="size" %s />
> > > -<img id="image" class="size" %s />
> > > -%s
> > > +<img id="bgimage" class="size" ${bg_image} />
> > > +<img id="image" class="size" ${image} />
> > > +${html_additions}
> > > <div class="lyricstable"><div id="lyricsmain" style="opacity:1"
> > class="lyricscell lyricsmain"></div></div>
> > > <div id="footer" class="footer"></div>
> > > <div id="black" class="size"></div>
> > > </body>
> > > </html>
> > > -"""
> > > +""")
> > > +
> > > +LYRICS_SRC = Template("""
> > > +.lyricstable {
> > > + z-index: 5;
> > > + position: absolute;
> > > + display: table;
> > > + ${stable}
> > > +}
> > > +.lyricscell {
> > > + display: table-cell;
> > > + word-wrap: break-word;
> > > + -webkit-transition: opacity 0.4s ease;
> > > + ${lyrics}
> > > +}
> > > +.lyricsmain {
> > > + ${main}
> > > +}
> > > +""")
> > > +
> > > +FOOTER_SRC = Template("""
> >
> > Indent please.
> > No real rule about indentation but Lyrics_src looks better!
> >
> > > +left: ${left}px;
> > > +bottom: ${bottom}px;
> > > +width: ${width}px;
> > > +font-family: ${family};
> > > +font-size: ${size}pt;
> > > +color: ${color};
> > > +text-align: left;
> > > +white-space: ${space};
> > > +""")
> > > +
> > > +LYRICS_FORMAT_SRC = Template("""
> > > +${justify}word-wrap: break-word;
> > > +text-align: ${align};
> > > +vertical-align: ${valign};
> > > +font-family: ${font};
> > > +font-size: ${size}pt;
> > > +color: ${color};
> > > +line-height: ${line}%;
> > > +margin: 0;
> > > +padding: 0;
> > > +padding-bottom: ${bottom};
> > > +padding-left: ${left}px;
> > > +width: ${width}px;
> > > +height: ${height}px;${font_style}${font_weight}
> > > +""")
> > >
> > >
> > > def build_html(item, screen, is_live, background, image=None,
> > plugins=None):
> >
> >
> > --
> >
> >
> https://code.launchpad.net/~alisonken1/openlp/strings-templates2/+merge/296567
> > You are the owner of lp:~alisonken1/openlp/strings-templates2.
> >
>
> --
>
> https://code.launchpad.net/~alisonken1/openlp/strings-templates2/+merge/296567
> You are reviewing the proposed merge of
> lp:~alisonken1/openlp/strings-templates2 into lp:openlp.
>

Revision history for this message
Tim Bentley (trb143) : Posted in a previous version of this proposal
review: Approve
lp://qastaging/~alisonken1/openlp/strings-templates2 updated
2680. By Ken Roberts

String format oops

Revision history for this message
Ken Roberts (alisonken1) wrote :

Ignore last commit (2680) - fixed in strings_template and reverted in this branch

Revision history for this message
Tomas Groth (tomasgroth) wrote :

Looks ok to me

review: Approve
Revision history for this message
Tim Bentley (trb143) :
review: Approve

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.