Merge lp:~knightrider0xd/openlp/better-slide-scaling into lp:openlp

[Ian Knight]

Adds the ability to choose a maximum height for non-text slides in the list-preview-widget in slide controllers. See branch description for further details. May require sanity check.

Changes since last proposal:
- Fixed copy-paste error in comments
- Shifted common test code into setup function as recommended.
- Testing max height doesn't exceed settings spin-box (as requested) not changed as any positive int is valid & spin-box values are arbitrary. (See reply to diff comments on r2630 for details)

lp:~knightrider0xd/openlp/better-slide-scaling (revision 2632)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1340/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1260/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1199/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1034/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/625/
[SUCCESS] https://ci.openlp.io/job/Branch-05a-Code_Analysis/692/
[SUCCESS] https://ci.openlp.io/job/Branch-05b-Test_Coverage/560/

[Tim Bentley (trb143)]

see inline comments

review: Needs Fixing

[Ian Knight (knightrider0xd)]

Thanks for the feedback. See replies to inline comments.

[Ian Knight (knightrider0xd)]

Shifted common test code into setup function as recommended.

lp:~knightrider0xd/openlp/better-slide-scaling (revision 2632)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1340/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1260/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1199/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1034/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/625/
[SUCCESS] https://ci.openlp.io/job/Branch-05a-Code_Analysis/692/
[SUCCESS] https://ci.openlp.io/job/Branch-05b-Test_Coverage/560/

[Tomas Groth (tomasgroth)]

Hi,
Remember to resubmit your merge proposal when it is ready for another review.
And you should probably just leave the branchs status as "development", we use "mature" for the release branches :)

[Ian Knight (knightrider0xd)]

Thanks Tomas, I wasn't sure what the procedure was :)

[Raoul Snyman (raoul-snyman)]

Works for me.

review: Approve

[Tim Bentley (trb143)]

review: Approve

=== modified file 'openlp/core/common/settings.py'
--- openlp/core/common/settings.py	2016-03-15 21:34:58 +0000
+++ openlp/core/common/settings.py	2016-04-01 09:26:21 +0000
@@ -121,6 +121,7 @@
         'advanced/double click live': False,
         'advanced/enable exit confirmation': True,
         'advanced/expand service item': False,
+        'advanced/slide max height': 0,
         'advanced/hide mouse': True,
         'advanced/is portable': False,
         'advanced/max recent files': 20,

=== modified file 'openlp/core/ui/advancedtab.py'
--- openlp/core/ui/advancedtab.py	2016-01-19 06:52:23 +0000
+++ openlp/core/ui/advancedtab.py	2016-04-01 09:26:21 +0000
@@ -83,6 +83,13 @@
         self.expand_service_item_check_box = QtWidgets.QCheckBox(self.ui_group_box)
         self.expand_service_item_check_box.setObjectName('expand_service_item_check_box')
         self.ui_layout.addRow(self.expand_service_item_check_box)
+        self.slide_max_height_label = QtWidgets.QLabel(self.ui_group_box)
+        self.slide_max_height_label.setObjectName('slide_max_height_label')
+        self.slide_max_height_spin_box = QtWidgets.QSpinBox(self.ui_group_box)
+        self.slide_max_height_spin_box.setObjectName('slide_max_height_spin_box')
+        self.slide_max_height_spin_box.setRange(0, 1000)
+        self.slide_max_height_spin_box.setSingleStep(20)
+        self.ui_layout.addRow(self.slide_max_height_label, self.slide_max_height_spin_box)
         self.search_as_type_check_box = QtWidgets.QCheckBox(self.ui_group_box)
         self.search_as_type_check_box.setObjectName('SearchAsType_check_box')
         self.ui_layout.addRow(self.search_as_type_check_box)
@@ -277,6 +284,9 @@
                                                                       'Preview items when clicked in Service Manager'))
         self.expand_service_item_check_box.setText(translate('OpenLP.AdvancedTab',
                                                              'Expand new service items on creation'))
+        self.slide_max_height_label.setText(translate('OpenLP.AdvancedTab',
+                                                      'Max height for non-text slides\nin slide controller:'))
+        self.slide_max_height_spin_box.setSpecialValueText(translate('OpenLP.AdvancedTab', 'Disabled'))
         self.enable_auto_close_check_box.setText(translate('OpenLP.AdvancedTab',
                                                            'Enable application exit confirmation'))
         self.service_name_group_box.setTitle(translate('OpenLP.AdvancedTab', 'Default Service Name'))
@@ -346,6 +356,7 @@
         self.single_click_preview_check_box.setChecked(settings.value('single click preview'))
         self.single_click_service_preview_check_box.setChecked(settings.value('single click service preview'))
         self.expand_service_item_check_box.setChecked(settings.value('expand service item'))
+        self.slide_max_height_spin_box.setValue(settings.value('slide max height'))
         self.enable_auto_close_check_box.setChecked(settings.value('enable exit confirmation'))
         self.hide_mouse_check_box.setChecked(settings.value('hide mouse'))
         self.service_name_day.setCurrentIndex(settings.value('default service day'))
@@ -428,6 +439,7 @@
         settings.setValue('single click preview', self.single_click_preview_check_box.isChecked())
         settings.setValue('single click service preview', self.single_click_service_preview_check_box.isChecked())
         settings.setValue('expand service item', self.expand_service_item_check_box.isChecked())
+        settings.setValue('slide max height', self.slide_max_height_spin_box.value())
         settings.setValue('enable exit confirmation', self.enable_auto_close_check_box.isChecked())
         settings.setValue('hide mouse', self.hide_mouse_check_box.isChecked())
         settings.setValue('alternate rows', self.alternate_rows_check_box.isChecked())

=== modified file 'openlp/core/ui/listpreviewwidget.py'
--- openlp/core/ui/listpreviewwidget.py	2015-12-31 22:46:06 +0000
+++ openlp/core/ui/listpreviewwidget.py	2016-04-01 09:26:21 +0000
@@ -26,7 +26,7 @@
 
 from PyQt5 import QtCore, QtGui, QtWidgets
 
-from openlp.core.common import RegistryProperties
+from openlp.core.common import RegistryProperties, Settings
 from openlp.core.lib import ImageSource, ServiceItem
 
 
@@ -63,6 +63,8 @@
         # Initialize variables.
         self.service_item = ServiceItem()
         self.screen_ratio = screen_ratio
+        # Connect signals
+        self.verticalHeader().sectionResized.connect(self.row_resized)
 
     def resizeEvent(self, event):
         """
@@ -80,12 +82,30 @@
             # Sort out songs, bibles, etc.
             if self.service_item.is_text():
                 self.resizeRowsToContents()
+            # Sort out image heights.
             else:
-                # Sort out image heights.
+                height = self.viewport().width() // self.screen_ratio
+                max_img_row_height = Settings().value('advanced/slide max height')
+                # Adjust for row height cap if in use.
+                if max_img_row_height > 0 and height > max_img_row_height:
+                    height = max_img_row_height
+                # Apply new height to slides
                 for frame_number in range(len(self.service_item.get_frames())):
-                    height = self.viewport().width() // self.screen_ratio
                     self.setRowHeight(frame_number, height)
 
+    def row_resized(self, row, old_height, new_height):
+        """
+        Will scale non-image slides.
+        """
+        # Only for non-text slides when row height cap in use
+        if self.service_item.is_text() or Settings().value('advanced/slide max height') <= 0:
+            return
+        # Get and validate label widget containing slide & adjust max width
+        try:
+            self.cellWidget(row, 0).children()[1].setMaximumWidth(new_height * self.screen_ratio)
+        except:
+            return
+
     def screen_size_changed(self, screen_ratio):
         """
         This method is called whenever the live screen size changes, which then makes a layout recalculation necessary
@@ -139,8 +159,26 @@
                     pixmap = QtGui.QPixmap.fromImage(image)
                     pixmap.setDevicePixelRatio(label.devicePixelRatio())
                     label.setPixmap(pixmap)
-                self.setCellWidget(frame_number, 0, label)
                 slide_height = width // self.screen_ratio
+                # Setup row height cap if in use.
+                max_img_row_height = Settings().value('advanced/slide max height')
+                if max_img_row_height > 0:
+                    if slide_height > max_img_row_height:
+                        slide_height = max_img_row_height
+                    label.setMaximumWidth(max_img_row_height * self.screen_ratio)
+                    label.resize(max_img_row_height * self.screen_ratio, max_img_row_height)
+                    # Build widget with stretch padding
+                    container = QtWidgets.QWidget()
+                    hbox = QtWidgets.QHBoxLayout()
+                    hbox.setContentsMargins(0, 0, 0, 0)
+                    hbox.addWidget(label, stretch=1)
+                    hbox.addStretch(0)
+                    container.setLayout(hbox)
+                    # Add to table
+                    self.setCellWidget(frame_number, 0, container)
+                else:
+                    # Add to table
+                    self.setCellWidget(frame_number, 0, label)
                 row += 1
             text.append(str(row))
             self.setItem(frame_number, 0, item)

=== modified file 'tests/functional/openlp_core_ui/test_listpreviewwidget.py'
--- tests/functional/openlp_core_ui/test_listpreviewwidget.py	2015-12-31 22:46:06 +0000
+++ tests/functional/openlp_core_ui/test_listpreviewwidget.py	2016-04-01 09:26:21 +0000
@@ -23,9 +23,12 @@
 Package to test the openlp.core.ui.listpreviewwidget package.
 """
 from unittest import TestCase
+
+from openlp.core.common import Settings
 from openlp.core.ui.listpreviewwidget import ListPreviewWidget
+from openlp.core.lib import ServiceItem
 
-from tests.functional import patch
+from tests.functional import MagicMock, patch, call
 
 
 class TestListPreviewWidget(TestCase):
@@ -34,9 +37,27 @@
         """
         Mock out stuff for all the tests
         """
-        self.setup_patcher = patch('openlp.core.ui.listpreviewwidget.ListPreviewWidget._setup')
-        self.mocked_setup = self.setup_patcher.start()
-        self.addCleanup(self.setup_patcher.stop)
+        # Mock self.parent().width()
+        self.parent_patcher = patch('openlp.core.ui.listpreviewwidget.ListPreviewWidget.parent')
+        self.mocked_parent = self.parent_patcher.start()
+        self.mocked_parent.width.return_value = 100
+        self.addCleanup(self.parent_patcher.stop)
+
+        # Mock Settings().value()
+        self.Settings_patcher = patch('openlp.core.ui.listpreviewwidget.Settings')
+        self.mocked_Settings = self.Settings_patcher.start()
+        self.mocked_Settings_obj = MagicMock()
+        self.mocked_Settings_obj.value.return_value = None
+        self.mocked_Settings.return_value = self.mocked_Settings_obj
+        self.addCleanup(self.Settings_patcher.stop)
+
+        # Mock self.viewport().width()
+        self.viewport_patcher = patch('openlp.core.ui.listpreviewwidget.ListPreviewWidget.viewport')
+        self.mocked_viewport = self.viewport_patcher.start()
+        self.mocked_viewport_obj = MagicMock()
+        self.mocked_viewport_obj.width.return_value = 200
+        self.mocked_viewport.return_value = self.mocked_viewport_obj
+        self.addCleanup(self.viewport_patcher.stop)
 
     def new_list_preview_widget_test(self):
         """
@@ -49,4 +70,206 @@
 
         # THEN: The object is not None, and the _setup() method was called.
         self.assertIsNotNone(list_preview_widget, 'The ListPreviewWidget object should not be None')
-        self.mocked_setup.assert_called_with(1)
+        self.assertEquals(list_preview_widget.screen_ratio, 1, 'Should not be called')
+
+    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.resizeRowsToContents')
+    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.setRowHeight')
+    def replace_recalculate_layout_test_text(self, mocked_setRowHeight, mocked_resizeRowsToContents):
+        """
+        Test if "Max height for non-text slides..." enabled, txt slides unchanged in replace_service_item & __recalc...
+        """
+        # GIVEN: A setting to adjust "Max height for non-text slides in slide controller",
+        #        a text ServiceItem and a ListPreviewWidget.
+
+        # Mock Settings().value('advanced/slide max height')
+        self.mocked_Settings_obj.value.return_value = 100
+        # Mock self.viewport().width()
+        self.mocked_viewport_obj.width.return_value = 200
+        # Mock text service item
+        service_item = MagicMock()
+        service_item.is_text.return_value = True
+        service_item.get_frames.return_value = [{'title': None, 'text': None, 'verseTag': None},
+                                                {'title': None, 'text': None, 'verseTag': None}]
+        # init ListPreviewWidget and load service item
+        list_preview_widget = ListPreviewWidget(None, 1)
+        list_preview_widget.replace_service_item(service_item, 200, 0)
+        # Change viewport width before forcing a resize
+        self.mocked_viewport_obj.width.return_value = 400
+
+        # WHEN: __recalculate_layout() is called (via resizeEvent)
+        list_preview_widget.resizeEvent(None)
+
+        # THEN: setRowHeight() should not be called, while resizeRowsToContents() should be called twice
+        #       (once each in __recalculate_layout and replace_service_item)
+        self.assertEquals(mocked_resizeRowsToContents.call_count, 2, 'Should be called')
+        self.assertEquals(mocked_setRowHeight.call_count, 0, 'Should not be called')
+
+    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.resizeRowsToContents')
+    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.setRowHeight')
+    def replace_recalculate_layout_test_img(self, mocked_setRowHeight, mocked_resizeRowsToContents):
+        """
+        Test if "Max height for non-text slides..." disabled, img slides unchanged in replace_service_item & __recalc...
+        """
+        # GIVEN: A setting to adjust "Max height for non-text slides in slide controller",
+        #        an image ServiceItem and a ListPreviewWidget.
+
+        # Mock Settings().value('advanced/slide max height')
+        self.mocked_Settings_obj.value.return_value = 0
+        # Mock self.viewport().width()
+        self.mocked_viewport_obj.width.return_value = 200
+        # Mock image service item
+        service_item = MagicMock()
+        service_item.is_text.return_value = False
+        service_item.get_frames.return_value = [{'title': None, 'path': None, 'image': None},
+                                                {'title': None, 'path': None, 'image': None}]
+        # init ListPreviewWidget and load service item
+        list_preview_widget = ListPreviewWidget(None, 1)
+        list_preview_widget.replace_service_item(service_item, 200, 0)
+        # Change viewport width before forcing a resize
+        self.mocked_viewport_obj.width.return_value = 400
+
+        # WHEN: __recalculate_layout() is called (via resizeEvent)
+        list_preview_widget.resizeEvent(None)
+
+        # THEN: resizeRowsToContents() should not be called, while setRowHeight() should be called
+        #       twice for each slide.
+        self.assertEquals(mocked_resizeRowsToContents.call_count, 0, 'Should not be called')
+        self.assertEquals(mocked_setRowHeight.call_count, 4, 'Should be called twice for each slide')
+        calls = [call(0, 200), call(1, 200), call(0, 400), call(1, 400)]
+        mocked_setRowHeight.assert_has_calls(calls)
+
+    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.resizeRowsToContents')
+    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.setRowHeight')
+    def replace_recalculate_layout_test_img_max(self, mocked_setRowHeight, mocked_resizeRowsToContents):
+        """
+        Test if "Max height for non-text slides..." enabled, img slides resized in replace_service_item & __recalc...
+        """
+        # GIVEN: A setting to adjust "Max height for non-text slides in slide controller",
+        #        an image ServiceItem and a ListPreviewWidget.
+
+        # Mock Settings().value('advanced/slide max height')
+        self.mocked_Settings_obj.value.return_value = 100
+        # Mock self.viewport().width()
+        self.mocked_viewport_obj.width.return_value = 200
+        # Mock image service item
+        service_item = MagicMock()
+        service_item.is_text.return_value = False
+        service_item.get_frames.return_value = [{'title': None, 'path': None, 'image': None},
+                                                {'title': None, 'path': None, 'image': None}]
+        # init ListPreviewWidget and load service item
+        list_preview_widget = ListPreviewWidget(None, 1)
+        list_preview_widget.replace_service_item(service_item, 200, 0)
+        # Change viewport width before forcing a resize
+        self.mocked_viewport_obj.width.return_value = 400
+
+        # WHEN: __recalculate_layout() is called (via resizeEvent)
+        list_preview_widget.resizeEvent(None)
+
+        # THEN: resizeRowsToContents() should not be called, while setRowHeight() should be called
+        #       twice for each slide.
+        self.assertEquals(mocked_resizeRowsToContents.call_count, 0, 'Should not be called')
+        self.assertEquals(mocked_setRowHeight.call_count, 4, 'Should be called twice for each slide')
+        calls = [call(0, 100), call(1, 100), call(0, 100), call(1, 100)]
+        mocked_setRowHeight.assert_has_calls(calls)
+
+    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.resizeRowsToContents')
+    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.setRowHeight')
+    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.cellWidget')
+    def row_resized_test_text(self, mocked_cellWidget, mocked_setRowHeight, mocked_resizeRowsToContents):
+        """
+        Test if "Max height for non-text slides..." enabled, text-based slides not affected in row_resized.
+        """
+        # GIVEN: A setting to adjust "Max height for non-text slides in slide controller",
+        #        a text ServiceItem and a ListPreviewWidget.
+
+        # Mock Settings().value('advanced/slide max height')
+        self.mocked_Settings_obj.value.return_value = 100
+        # Mock self.viewport().width()
+        self.mocked_viewport_obj.width.return_value = 200
+        # Mock text service item
+        service_item = MagicMock()
+        service_item.is_text.return_value = True
+        service_item.get_frames.return_value = [{'title': None, 'text': None, 'verseTag': None},
+                                                {'title': None, 'text': None, 'verseTag': None}]
+        # Mock self.cellWidget().children().setMaximumWidth()
+        mocked_cellWidget_child = MagicMock()
+        mocked_cellWidget_obj = MagicMock()
+        mocked_cellWidget_obj.children.return_value = [None, mocked_cellWidget_child]
+        mocked_cellWidget.return_value = mocked_cellWidget_obj
+        # init ListPreviewWidget and load service item
+        list_preview_widget = ListPreviewWidget(None, 1)
+        list_preview_widget.replace_service_item(service_item, 200, 0)
+
+        # WHEN: row_resized() is called
+        list_preview_widget.row_resized(0, 100, 150)
+
+        # THEN: self.cellWidget(row, 0).children()[1].setMaximumWidth() should not be called
+        self.assertEquals(mocked_cellWidget_child.setMaximumWidth.call_count, 0, 'Should not be called')
+
+    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.resizeRowsToContents')
+    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.setRowHeight')
+    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.cellWidget')
+    def row_resized_test_img(self, mocked_cellWidget, mocked_setRowHeight, mocked_resizeRowsToContents):
+        """
+        Test if "Max height for non-text slides..." disabled, image-based slides not affected in row_resized.
+        """
+        # GIVEN: A setting to adjust "Max height for non-text slides in slide controller",
+        #        an image ServiceItem and a ListPreviewWidget.
+
+        # Mock Settings().value('advanced/slide max height')
+        self.mocked_Settings_obj.value.return_value = 0
+        # Mock self.viewport().width()
+        self.mocked_viewport_obj.width.return_value = 200
+        # Mock image service item
+        service_item = MagicMock()
+        service_item.is_text.return_value = False
+        service_item.get_frames.return_value = [{'title': None, 'path': None, 'image': None},
+                                                {'title': None, 'path': None, 'image': None}]
+        # Mock self.cellWidget().children().setMaximumWidth()
+        mocked_cellWidget_child = MagicMock()
+        mocked_cellWidget_obj = MagicMock()
+        mocked_cellWidget_obj.children.return_value = [None, mocked_cellWidget_child]
+        mocked_cellWidget.return_value = mocked_cellWidget_obj
+        # init ListPreviewWidget and load service item
+        list_preview_widget = ListPreviewWidget(None, 1)
+        list_preview_widget.replace_service_item(service_item, 200, 0)
+
+        # WHEN: row_resized() is called
+        list_preview_widget.row_resized(0, 100, 150)
+
+        # THEN: self.cellWidget(row, 0).children()[1].setMaximumWidth() should not be called
+        self.assertEquals(mocked_cellWidget_child.setMaximumWidth.call_count, 0, 'Should not be called')
+
+    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.resizeRowsToContents')
+    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.setRowHeight')
+    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.cellWidget')
+    def row_resized_test_img_max(self, mocked_cellWidget, mocked_setRowHeight, mocked_resizeRowsToContents):
+        """
+        Test if "Max height for non-text slides..." enabled, image-based slides are scaled in row_resized.
+        """
+        # GIVEN: A setting to adjust "Max height for non-text slides in slide controller",
+        #        an image ServiceItem and a ListPreviewWidget.
+
+        # Mock Settings().value('advanced/slide max height')
+        self.mocked_Settings_obj.value.return_value = 100
+        # Mock self.viewport().width()
+        self.mocked_viewport_obj.width.return_value = 200
+        # Mock image service item
+        service_item = MagicMock()
+        service_item.is_text.return_value = False
+        service_item.get_frames.return_value = [{'title': None, 'path': None, 'image': None},
+                                                {'title': None, 'path': None, 'image': None}]
+        # Mock self.cellWidget().children().setMaximumWidth()
+        mocked_cellWidget_child = MagicMock()
+        mocked_cellWidget_obj = MagicMock()
+        mocked_cellWidget_obj.children.return_value = [None, mocked_cellWidget_child]
+        mocked_cellWidget.return_value = mocked_cellWidget_obj
+        # init ListPreviewWidget and load service item
+        list_preview_widget = ListPreviewWidget(None, 1)
+        list_preview_widget.replace_service_item(service_item, 200, 0)
+
+        # WHEN: row_resized() is called
+        list_preview_widget.row_resized(0, 100, 150)
+
+        # THEN: self.cellWidget(row, 0).children()[1].setMaximumWidth() should be called
+        mocked_cellWidget_child.setMaximumWidth.assert_called_once_with(150)