openlp-core team mailing list archive

[Tim Bentley <tim.bentley@xxxxxxxxx>]

Review: Needs Fixing

At first glance I like the idea but the implementation is slightly wrong.

The architecture of OpenLP defines that plugins and core code do not mix and if they need to the ServiceItem is the beast to do this.

Looking at you change the logic looks good but there are two ways this could go.

1) Move the settings from Songs Tab to advanced tab and this makes the code core only.

2) Use Songs and add the value to the ServiceItem so it could be a per song option.  This would not need settings but a combo on the song Media Manager.  See Bibles for an example for formatting.

Raoul may have an view on this.

Also you will need a test to get it merge.

Having said all the above this is a nice feature. 

Diff comments:

> === modified file 'openlp/core/common/__init__.py'
> --- openlp/core/common/__init__.py	2014-08-27 23:18:06 +0000
> +++ openlp/core/common/__init__.py	2014-10-11 11:33:59 +0000
> @@ -120,6 +120,16 @@
>      Next = 3
>  
>  
> +class SlideFooter(object):
> +    """
> +    Provides an enumeration for when to show the footer text during the songs
> +    """
> +    All = 1
> +    First = 2
> +    Last = 3
> +    FirstAndLast = 4
> +
> +
>  def de_hump(name):
>      """
>      Change any Camel Case string to python string
> 
> === modified file 'openlp/core/ui/maindisplay.py'
> --- openlp/core/ui/maindisplay.py	2014-08-27 23:18:06 +0000
> +++ openlp/core/ui/maindisplay.py	2014-10-11 11:33:59 +0000
> @@ -326,7 +326,7 @@
>          # clear the cache
>          self.override = {}
>  
> -    def preview(self):
> +    def preview(self, hide_slide):
>          """
>          Generates a preview of the image displayed.
>          """
> @@ -337,6 +337,12 @@
>              # Wait for the fade to finish before geting the preview.
>              # Important otherwise preview will have incorrect text if at all!
>              if self.service_item.theme_data and self.service_item.theme_data.display_slide_transition:
> +                if hide_slide:
> +                    if self.service_item.foot_text:
> +                        self.footer("")
> +                else:
> +                    if self.service_item.foot_text:
> +                        self.footer(self.service_item.foot_text)
>                  while not self.frame.evaluateJavaScript('show_text_completed()'):
>                      self.application.process_events()
>          # Wait for the webview to update before getting the preview.
> 
> === modified file 'openlp/core/ui/slidecontroller.py'
> --- openlp/core/ui/slidecontroller.py	2014-07-15 18:52:59 +0000
> +++ openlp/core/ui/slidecontroller.py	2014-10-11 11:33:59 +0000
> @@ -36,7 +36,7 @@
>  
>  from PyQt4 import QtCore, QtGui
>  
> -from openlp.core.common import Registry, RegistryProperties, Settings, SlideLimits, UiStrings, translate, \
> +from openlp.core.common import Registry, RegistryProperties, Settings, SlideLimits, UiStrings, translate, SlideFooter, \
>      RegistryMixin, OpenLPMixin
>  from openlp.core.lib import OpenLPToolbar, ItemCapabilities, ServiceItem, ImageSource, ServiceItemAction, \
>      ScreenList, build_icon, build_html
> @@ -1069,10 +1069,37 @@
>              QtCore.QTimer.singleShot(0.5, self.grab_maindisplay)
>              QtCore.QTimer.singleShot(2.5, self.grab_maindisplay)
>          else:
> -            self.slide_image = self.display.preview()
> +            hide_footer = self.check_to_hide_footer((self.preview_widget.current_slide_number() + 1), self.preview_widget.slide_count())
> +            self.slide_image = self.display.preview(hide_footer)
>              self.slide_preview.setPixmap(self.slide_image)
>          self.slide_count += 1
>  
> +    def check_to_hide_footer(self, current_slide, slide_count):
> +        """
> +        Determines if the current slide should hide the text in the footer
> +        """
> +        hide_footer = False
> +        slide_footer = Settings().value(self.main_window.songs_settings_section + '/show footer')
> +        if current_slide == 1:
> +            # If it's on the first slide only hide the footer when the 'Show footer on
> +            # last slide' setting is on
> +            if slide_footer == SlideFooter.Last:
> +                hide_footer = True
> +        elif current_slide == slide_count:
> +            # If it's on the last slide only hide the footer when the 'Show footer on
> +            # first slide' setting is on
> +            if slide_footer == SlideFooter.First:
> +                hide_footer = True
> +        else:
> +            # If it's on any of the middle slides only hide the footer if the 'Show footer on all
> +            # slides' setting is not on (ie. when setting is either First, Last or First + Last)
> +            if (slide_footer != SlideFooter.All) & \
> +                    ((slide_footer == SlideFooter.First) |
> +                    (slide_footer == SlideFooter.Last) |
> +                    (slide_footer == SlideFooter.FirstAndLast)):
> +                hide_footer = True
> +        return hide_footer
> +
>      def grab_maindisplay(self):
>          """
>          Creates an image of the current screen and updates the preview frame.
> 
> === modified file 'openlp/plugins/songs/lib/songstab.py'
> --- openlp/plugins/songs/lib/songstab.py	2014-07-09 12:33:26 +0000
> +++ openlp/plugins/songs/lib/songstab.py	2014-10-11 11:33:59 +0000
> @@ -29,7 +29,7 @@
>  
>  from PyQt4 import QtCore, QtGui
>  
> -from openlp.core.common import Settings, translate
> +from openlp.core.common import Settings, translate, SlideFooter
>  from openlp.core.lib import SettingsTab
>  from openlp.plugins.songs.lib.ui import SongStrings
>  
> @@ -67,6 +67,24 @@
>          self.display_copyright_check_box.setObjectName('copyright_check_box')
>          self.mode_layout.addWidget(self.display_copyright_check_box)
>          self.left_layout.addWidget(self.mode_group_box)
> +        # Footer/copyright
> +        self.footer_group_box = QtGui.QGroupBox(self.left_column)
> +        self.footer_group_box.setObjectName('footer_group_box')
> +        self.footer_layout = QtGui.QVBoxLayout(self.footer_group_box)
> +        self.footer_layout.setObjectName('slide_layout')
> +        self.all_slides_radio_button = QtGui.QRadioButton(self.footer_group_box)
> +        self.all_slides_radio_button.setObjectName('all_slides_radio_button')
> +        self.footer_layout.addWidget(self.all_slides_radio_button)
> +        self.first_slide_radio_button = QtGui.QRadioButton(self.footer_group_box)
> +        self.first_slide_radio_button.setObjectName('first_slide_radio_button')
> +        self.footer_layout.addWidget(self.first_slide_radio_button)
> +        self.last_slide_radio_button = QtGui.QRadioButton(self.footer_group_box)
> +        self.last_slide_radio_button.setObjectName('last_slide_radio_button')
> +        self.footer_layout.addWidget(self.last_slide_radio_button)
> +        self.first_last_slide_radio_button = QtGui.QRadioButton(self.footer_group_box)
> +        self.first_last_slide_radio_button.setObjectName('first_last_slide_radio_button')
> +        self.footer_layout.addWidget(self.first_last_slide_radio_button)
> +        self.left_layout.addWidget(self.footer_group_box)
>          self.left_layout.addStretch()
>          self.right_layout.addStretch()
>          self.search_as_type_check_box.stateChanged.connect(self.on_search_as_type_check_box_changed)
> @@ -75,6 +93,10 @@
>          self.add_from_service_check_box.stateChanged.connect(self.on_add_from_service_check_box_changed)
>          self.display_songbook_check_box.stateChanged.connect(self.on_songbook_check_box_changed)
>          self.display_copyright_check_box.stateChanged.connect(self.on_copyright_check_box_changed)
> +        self.all_slides_radio_button.clicked.connect(self.on_all_slides_button_clicked)

Could this be a combo box?

> +        self.first_slide_radio_button.clicked.connect(self.on_first_slide_button_clicked)
> +        self.last_slide_radio_button.clicked.connect(self.on_last_slide_button_clicked)
> +        self.first_last_slide_radio_button.clicked.connect(self.on_first_and_last_slides_button_clicked)
>  
>      def retranslateUi(self):
>          self.mode_group_box.setTitle(translate('SongsPlugin.SongsTab', 'Songs Mode'))
> @@ -88,6 +110,17 @@
>          self.display_copyright_check_box.setText(translate('SongsPlugin.SongsTab',
>                                                             'Display "%s" symbol before copyright info' %
>                                                             SongStrings.CopyrightSymbol))
> +        # Footer
> +        self.footer_group_box.setTitle(translate('SongsPlugin.SongsTab',

Can be one line as we support 120 column lines. There are others as well.

> +                                                 'Show Footer'))
> +        self.all_slides_radio_button.setText(translate('SongsPlugin.SongsTab',
> +                                                       'All slides'))
> +        self.first_slide_radio_button.setText(translate('SongsPlugin.SongsTab',
> +                                                        'First slide only'))
> +        self.last_slide_radio_button.setText(translate('SongsPlugin.SongsTab',
> +                                                       'Last slide only'))
> +        self.first_last_slide_radio_button.setText(translate('SongsPlugin.SongsTab',
> +                                                             'First and last slide only'))
>  
>      def on_search_as_type_check_box_changed(self, check_state):
>          self.song_search = (check_state == QtCore.Qt.Checked)
> @@ -107,6 +140,30 @@
>      def on_copyright_check_box_changed(self, check_state):
>          self.display_copyright_symbol = (check_state == QtCore.Qt.Checked)
>  
> +    def on_all_slides_button_clicked(self):
> +        """
> +        Show footer on all song slides
> +        """
> +        self.slide_footer = SlideFooter.All
> +
> +    def on_first_slide_button_clicked(self):
> +        """
> +        Show footer on first song slide only
> +        """
> +        self.slide_footer = SlideFooter.First
> +
> +    def on_last_slide_button_clicked(self):
> +        """
> +        Show footer on last song slide only
> +        """
> +        self.slide_footer = SlideFooter.Last
> +
> +    def on_first_and_last_slides_button_clicked(self):
> +        """
> +        Show footer on last song slide only
> +        """
> +        self.slide_footer = SlideFooter.FirstAndLast
> +
>      def load(self):
>          settings = Settings()
>          settings.beginGroup(self.settings_section)
> @@ -116,12 +173,21 @@
>          self.update_load = settings.value('add song from service')
>          self.display_songbook = settings.value('display songbook')
>          self.display_copyright_symbol = settings.value('display copyright symbol')
> +        self.show_footer = settings.value('show footer')
>          self.search_as_type_check_box.setChecked(self.song_search)
>          self.tool_bar_active_check_box.setChecked(self.tool_bar)
>          self.update_on_edit_check_box.setChecked(self.update_edit)
>          self.add_from_service_check_box.setChecked(self.update_load)
>          self.display_songbook_check_box.setChecked(self.display_songbook)
>          self.display_copyright_check_box.setChecked(self.display_copyright_symbol)
> +        if self.show_footer == SlideFooter.FirstAndLast:
> +            self.first_last_slide_radio_button.setChecked(True)
> +        elif self.show_footer == SlideFooter.First:
> +            self.first_slide_radio_button.setChecked(True)
> +        elif self.show_footer == SlideFooter.Last:
> +            self.last_slide_radio_button.setChecked(True)
> +        else:
> +            self.all_slides_radio_button.setChecked(True)
>          settings.endGroup()
>  
>      def save(self):
> @@ -133,6 +199,7 @@
>          settings.setValue('add song from service', self.update_load)
>          settings.setValue('display songbook', self.display_songbook)
>          settings.setValue('display copyright symbol', self.display_copyright_symbol)
> +        settings.setValue('show footer', self.slide_footer)
>          settings.endGroup()
>          if self.tab_visited:
>              self.settings_form.register_post_process('songs_config_updated')
> 
> === modified file 'openlp/plugins/songs/songsplugin.py'
> --- openlp/plugins/songs/songsplugin.py	2014-07-09 12:33:26 +0000
> +++ openlp/plugins/songs/songsplugin.py	2014-10-11 11:33:59 +0000
> @@ -38,7 +38,7 @@
>  
>  from PyQt4 import QtCore, QtGui
>  
> -from openlp.core.common import UiStrings, Registry, translate
> +from openlp.core.common import UiStrings, Registry, translate, SlideFooter
>  from openlp.core.lib import Plugin, StringContent, build_icon
>  from openlp.core.lib.db import Manager
>  from openlp.core.lib.ui import create_action
> @@ -69,7 +69,8 @@
>      'songs/last directory export': '',
>      'songs/songselect username': '',
>      'songs/songselect password': '',
> -    'songs/songselect searches': ''
> +    'songs/songselect searches': '',
> +    'songs/show footer': SlideFooter.All
>  }
>  
>  
> 


-- 
https://code.launchpad.net/~martin-woodhart/openlp/show-hide-song-footer/+merge/238050
Your team OpenLP Core is subscribed to branch lp:openlp.