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 ...

bzr-revno: 2636
Fixes: https://launchpad.net/bugs/891860, https://launchpad.net/bugs/1550856

openlp/core/common/settings.py

@@ -121,6 +121,7 @@ class Settings(QtCore.QSettings):
         '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,

openlp/core/ui/advancedtab.py

@@ -83,6 +83,13 @@ class AdvancedTab(SettingsTab):
         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 @@ class AdvancedTab(SettingsTab):
                                                                       '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 @@ class AdvancedTab(SettingsTab):
         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 @@ class AdvancedTab(SettingsTab):
         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())

openlp/core/ui/listpreviewwidget.py

@@ -26,7 +26,7 @@ It is based on a QTableWidget but represents its contents in list form.
 
 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 @@ class ListPreviewWidget(QtWidgets.QTableWidget, RegistryProperties):
         # 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 @@ class ListPreviewWidget(QtWidgets.QTableWidget, RegistryProperties):
             # 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 @@ class ListPreviewWidget(QtWidgets.QTableWidget, RegistryProperties):
                     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)

tests/functional/openlp_core_ui/test_listpreviewwidget.py

@@ -23,9 +23,12 @@
 Package to test the openlp.core.ui.listpreviewwidget package.
 """
 from unittest import TestCase
-from openlp.core.ui.listpreviewwidget import ListPreviewWidget
 
-from tests.functional import patch
+from openlp.core.common import Settings
+from openlp.core.ui.listpreviewwidget import ListPreviewWidget
+from openlp.core.lib import ServiceItem
+
+from tests.functional import MagicMock, patch, call
 
 
 class TestListPreviewWidget(TestCase):
@@ -34,9 +37,27 @@ class TestListPreviewWidget(TestCase):
         """
         Mock out stuff for all the tests
         """
-        self.setup_patcher = patch('openlp.core.ui.listpreviewwidget.ListPreviewWidget._setup')
+        # Mock self.parent().width()
-        self.mocked_setup = self.setup_patcher.start()
+        self.parent_patcher = patch('openlp.core.ui.listpreviewwidget.ListPreviewWidget.parent')
-        self.addCleanup(self.setup_patcher.stop)
+        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 @@ class TestListPreviewWidget(TestCase):
 
         # 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)