openlp-core team mailing list archive

[Tomas Groth <tomasgroth@xxxxxxxx>]

Review: Needs Fixing

Just tested a bit.
You have introduced the "Click live slide to unblank" setting, but a "Unblank display when adding new item" also exists in the general tab. You should probably move yours to be under the exiting one to keep similar settings in the same place. Currently your code doesn't honor the "Unblank display when adding new item", which it will have to do. As it is now the item goes live no matter if the setting is enabled or not.
Also the new setting should be "false" as default, this is new behavior, so users should enabled it if they want it.
Also added a code-comment below.

Diff comments:

> 
> === modified file 'openlp/core/ui/slidecontroller.py'
> --- openlp/core/ui/slidecontroller.py	2016-02-28 20:33:19 +0000
> +++ openlp/core/ui/slidecontroller.py	2016-03-15 14:12:45 +0000
> @@ -789,11 +789,26 @@
>      def replace_service_manager_item(self, item):
>          """
>          Replacement item following a remote edit
> +        This action  also takes place when a song that is sent to live from Service Manager is edited.
> +        If display is blanked, this will update the song and then re-blank the display.
> +        As result, lyrics are flashed on screen for a very short time before re-blanking happens. (Bug)
>  
>          :param item: The current service item
>          """
>          if item == self.service_item:
> -            self._process_item(item, self.preview_widget.current_slide_number())
> +            if not self.hide_mode():
> +                self._process_item(item, self.preview_widget.current_slide_number())
> +            # "isChecked" method is required for checking blanks, on_xx_display(False) does not work.
> +            elif self.hide_mode():
> +                if self.blank_screen.isChecked():
> +                    self._process_item(item, self.preview_widget.current_slide_number())
> +                    self.on_blank_display(True)
> +                elif self.theme_screen.isChecked():
> +                    self._process_item(item, self.preview_widget.current_slide_number())
> +                    self.on_theme_display(True)
> +                elif self.desktop_screen.isChecked():
> +                    self._process_item(item, self.preview_widget.current_slide_number())
> +                    self.on_hide_display(True)

Is there any reason why you don't do "self._process_item(item, self.preview_widget.current_slide_number())" outside the ...isChecked()-ifs?
And why don't you use the returned value of hide_mode() instead of isChecked?

>  
>      def add_service_manager_item(self, item, slide_no):
>          """


-- 
https://code.launchpad.net/~suutari-olli/openlp/click-slide-to-go-live-from-blank/+merge/289026
Your team OpenLP Core is subscribed to branch lp:openlp.