openlp-core team mailing list archive

[Phill <phill.ridout@xxxxxxxxx>]

Review: Needs Fixing

See line 189 for my main comment.

Others are just me being nit-picky!

Diff comments:

> 
> === modified file 'openlp/plugins/presentations/lib/impresscontroller.py'
> --- openlp/plugins/presentations/lib/impresscontroller.py	2019-05-22 06:47:00 +0000
> +++ openlp/plugins/presentations/lib/impresscontroller.py	2019-05-29 19:24:56 +0000
> @@ -166,6 +187,8 @@
>          Called at system exit to clean up any running presentations.
>          """
>          log.debug('Kill OpenOffice')
> +        if self.presenter_screen_disabled_by_openlp:
> +            self._toggle_presentation_screen(True)

is this correct with the leading underscore? or should it be "self.toggle_presentation_screen(True)"

>          while self.docs:
>              self.docs[0].close_presentation()
>          desktop = None
> @@ -195,6 +218,54 @@
>              except Exception:
>                  log.warning('Failed to terminate OpenOffice')
>  
> +    def toggle_presentation_screen(self, target_value):

Would `set_visible` be more descriptive for `target_value`?
Can you document it? I.e. ":param bool {set_visible|target_value): .... "

> +        """
> +        Enable or disable the Presentation Screen/Console
> +        """
> +        # Create Instance of ConfigurationProvider
> +        if not self.conf_provider:
> +            if is_win():
> +                self.conf_provider = self.manager.createInstance('com.sun.star.configuration.ConfigurationProvider')
> +            else:
> +                self.conf_provider = self.manager.createInstanceWithContext(
> +                    'com.sun.star.configuration.ConfigurationProvider', uno.getComponentContext())
> +        # Setup lookup properties to get Impress settings
> +        properties = []
> +        properties.append(self.create_property('nodepath', 'org.openoffice.Office.Impress'))
> +        properties = tuple(properties)
> +        try:
> +            # Get an updateable configuration view
> +            impress_conf_props = self.conf_provider.createInstanceWithArguments(
> +                'com.sun.star.configuration.ConfigurationUpdateAccess', properties)
> +            # Get the specific setting for presentation screen
> +            presenter_screen_enabled = impress_conf_props.getHierarchicalPropertyValue(
> +                'Misc/Start/EnablePresenterScreen')
> +            # If the presentation screen is enabled we disable it
> +            if presenter_screen_enabled != target_value:
> +                impress_conf_props.setHierarchicalPropertyValue('Misc/Start/EnablePresenterScreen', target_value)
> +                impress_conf_props.commitChanges()
> +                # if target_value is False this is an attempt to disable the Presenter Screen
> +                # so we make a note that it has been disabled, so it can be enabled again on close.
> +                if target_value is False:
> +                    self.presenter_screen_disabled_by_openlp = True
> +        except Exception as e:
> +            log.exception(e)
> +            trace_error_handler(log)
> +            return

Not required

> +
> +    def create_property(self, name, value):
> +        """
> +        Create an OOo style property object which are passed into some Uno methods.
> +        """

can you document `name`, `value` and the return along with their types please?

> +        log.debug('create property OpenOffice')
> +        if is_win():
> +            property_object = self.manager.Bridge_GetStruct('com.sun.star.beans.PropertyValue')
> +        else:
> +            property_object = PropertyValue()
> +        property_object.Name = name
> +        property_object.Value = value
> +        return property_object
> +
>  
>  class ImpressDocument(PresentationDocument):
>      """
> @@ -483,3 +563,97 @@
>                  note = ' '
>              notes.append(note)
>          self.save_titles_and_notes(titles, notes)
> +
> +
> +class SlideShowListener(SlideShowListenerImport):
> +    """
> +    Listener interface to receive global slide show events.
> +    """
> +
> +    def __init__(self, document):
> +        """
> +        Constructor
> +
> +        :param document: The ImpressDocument being presented
> +        """
> +        self.document = document
> +
> +    def paused(self):
> +        """
> +        Notify that the slide show is paused
> +        """
> +        log.debug('LibreOffice SlideShowListener event: paused')
> +
> +    def resumed(self):
> +        """
> +        Notify that the slide show is resumed from a paused state
> +        """
> +        log.debug('LibreOffice SlideShowListener event: resumed')
> +
> +    def slideTransitionStarted(self):
> +        """
> +        Notify that a new slide starts to become visible.
> +        """
> +        log.debug('LibreOffice SlideShowListener event: slideTransitionStarted')
> +
> +    def slideTransitionEnded(self):
> +        """
> +        Notify that the slide transtion of the current slide ended.
> +        """
> +        log.debug('LibreOffice SlideShowListener event: slideTransitionEnded')
> +
> +    def slideAnimationsEnded(self):
> +        """
> +        Notify that the last animation from the main sequence of the current slide has ended.
> +        """
> +        log.debug('LibreOffice SlideShowListener event: slideAnimationsEnded')
> +        if not Registry().get('main_window').isActiveWindow():
> +            log.debug('main window is not in focus - should update slidecontroller')
> +            Registry().execute('slidecontroller_live_change', self.document.control.getCurrentSlideIndex() + 1)
> +
> +    def slideEnded(self, reverse):
> +        """
> +        Notify that the current slide has ended, e.g. the user has clicked on the slide. Calling displaySlide()
> +        twice will not issue this event.

Document `reverse` please with type info

> +        """
> +        log.debug('LibreOffice SlideShowListener event: slideEnded %d' % reverse)
> +        if reverse:
> +            self.document.slide_ended = False
> +            self.document.slide_ended_reverse = True
> +        else:
> +            self.document.slide_ended = True
> +            self.document.slide_ended_reverse = False
> +
> +    def hyperLinkClicked(self, hyperLink):
> +        """
> +        Notifies that a hyperlink has been clicked.
> +        """
> +        log.debug('LibreOffice SlideShowListener event: hyperLinkClicked %s' % hyperLink)
> +
> +    def disposing(self, source):
> +        """
> +        gets called when the broadcaster is about to be disposed.
> +        :param source:
> +        """
> +        log.debug('LibreOffice SlideShowListener event: disposing')
> +
> +    def beginEvent(self, node):
> +        """
> +        This event is raised when the element local timeline begins to play.
> +        :param node:
> +        """
> +        log.debug('LibreOffice SlideShowListener event: beginEvent')
> +
> +    def endEvent(self, node):
> +        """
> +        This event is raised at the active end of the element.
> +        :param node:
> +        """
> +        log.debug('LibreOffice SlideShowListener event: endEvent')
> +
> +    def repeat(self, node):
> +        """
> +        This event is raised when the element local timeline repeats.
> +        :param node:
> +        """
> +        log.debug('LibreOffice SlideShowListener event: repeat')
> 
> === modified file 'openlp/plugins/presentations/lib/presentationcontroller.py'
> --- openlp/plugins/presentations/lib/presentationcontroller.py	2019-05-22 06:47:00 +0000
> +++ openlp/plugins/presentations/lib/presentationcontroller.py	2019-05-29 19:24:56 +0000
> @@ -248,15 +248,17 @@
>      def next_step(self):
>          """
>          Triggers the next effect of slide on the running presentation. This might be the next animation on the current
> -        slide, or the next slide
> +        slide, or the next slide.
> +        Returns True if we stepped beyond the slides of the presentation

":returns bool: True if we stepped beyond the slides of the presentation"
would be better

>          """
> -        pass
> +        return False
>  
>      def previous_step(self):
>          """
>          Triggers the previous slide on the running presentation
> +        Returns True if we stepped beyond the slides of the presentation

Again: ":returns bool: ..."

>          """
> -        pass
> +        return False
>  
>      def convert_thumbnail(self, image_path, index):
>          """


-- 
https://code.launchpad.net/~tomasgroth/openlp/presentation-beyond-last/+merge/368091
Your team OpenLP Core is subscribed to branch lp:openlp.