openlp-core team mailing list archive

[Ian Knight <ieknight1101@xxxxxxxxx>]

Thanks for the feedback. See replies to inline comments.

Diff comments:

> 
> === 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-03-20 15:33:07 +0000
> @@ -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:

A max height <= 0 indicates disabled, hence testing for 0.
Any positive value is considered valid, but I chose for the spinbox to increment by 20 as a convenient value.
Users can still manually enter other values in the range 0 to 1000 in the spin box.
1000 is an arbitrary value that I chose as the spinbox requires an upper bound. If, for any reason, a larger value makes its way into Settings(), this won't impact the code for ListPreviewWidget.

> +            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
> 
> === 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-03-20 15:33:07 +0000
> @@ -49,4 +53,246 @@
>  
>          # 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.Settings')
> +    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.viewport')
> +    @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,
> +                                             mocked_viewport, mocked_Settings):
> +        """
> +        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')
> +        mocked_Settings_obj = MagicMock()
> +        mocked_Settings_obj.value.return_value = 100
> +        mocked_Settings.return_value = mocked_Settings_obj
> +        # Mock self.viewport().width()
> +        mocked_viewport_obj = MagicMock()
> +        mocked_viewport_obj.width.return_value = 200
> +        mocked_viewport.return_value = mocked_viewport_obj
> +        # 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
> +        mocked_viewport_obj.width.return_value = 400
> +
> +        # WHEN: __recalculate_layout() is called (via resizeEvent)
> +        list_preview_widget.resizeEvent(None)
> +
> +        # THEN: 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.Settings')
> +    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.viewport')
> +    @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,
> +                                            mocked_viewport, mocked_Settings):
> +        """
> +        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')
> +        mocked_Settings_obj = MagicMock()
> +        mocked_Settings_obj.value.return_value = 0
> +        mocked_Settings.return_value = mocked_Settings_obj
> +        # Mock self.viewport().width()
> +        mocked_viewport_obj = MagicMock()
> +        mocked_viewport_obj.width.return_value = 200
> +        mocked_viewport.return_value = mocked_viewport_obj
> +        # 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
> +        mocked_viewport_obj.width.return_value = 400
> +
> +        # WHEN: __recalculate_layout() is called (via resizeEvent)
> +        list_preview_widget.resizeEvent(None)
> +
> +        # THEN: timer should have been started

That's a copy-paste error. Sorry. I'll fix before resubmitting.

> +        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.Settings')
> +    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.viewport')
> +    @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,
> +                                                mocked_viewport, mocked_Settings):
> +        """
> +        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')
> +        mocked_Settings_obj = MagicMock()
> +        mocked_Settings_obj.value.return_value = 100
> +        mocked_Settings.return_value = mocked_Settings_obj
> +        # Mock self.viewport().width()
> +        mocked_viewport_obj = MagicMock()
> +        mocked_viewport_obj.width.return_value = 200
> +        mocked_viewport.return_value = mocked_viewport_obj
> +        # 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
> +        mocked_viewport_obj.width.return_value = 400
> +
> +        # WHEN: __recalculate_layout() is called (via resizeEvent)
> +        list_preview_widget.resizeEvent(None)
> +
> +        # THEN: timer should have been started
> +        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.Settings')
> +    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.viewport')
> +    @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,
> +                              mocked_viewport, mocked_Settings):
> +        """
> +        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')

There are some differences in the values Mocked against Settings and service_item between tests, but I can certainly set common defaults. I didn't make them common across all tests as they only apply to the tests I've added (though I may be missing something as far as applying them across my tests only).
Recommendations very welcome here.

> +        mocked_Settings_obj = MagicMock()
> +        mocked_Settings_obj.value.return_value = 100
> +        mocked_Settings.return_value = mocked_Settings_obj
> +        # Mock self.viewport().width()
> +        mocked_viewport_obj = MagicMock()
> +        mocked_viewport_obj.width.return_value = 200
> +        mocked_viewport.return_value = mocked_viewport_obj
> +        # 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.Settings')
> +    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.viewport')
> +    @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,
> +                             mocked_viewport, mocked_Settings):
> +        """
> +        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')
> +        mocked_Settings_obj = MagicMock()
> +        mocked_Settings_obj.value.return_value = 0
> +        mocked_Settings.return_value = mocked_Settings_obj
> +        # Mock self.viewport().width()
> +        mocked_viewport_obj = MagicMock()
> +        mocked_viewport_obj.width.return_value = 200
> +        mocked_viewport.return_value = mocked_viewport_obj
> +        # 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.Settings')
> +    @patch(u'openlp.core.ui.listpreviewwidget.ListPreviewWidget.viewport')
> +    @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,
> +                                 mocked_viewport, mocked_Settings):
> +        """
> +        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')
> +        mocked_Settings_obj = MagicMock()
> +        mocked_Settings_obj.value.return_value = 100
> +        mocked_Settings.return_value = mocked_Settings_obj
> +        # Mock self.viewport().width()
> +        mocked_viewport_obj = MagicMock()
> +        mocked_viewport_obj.width.return_value = 200
> +        mocked_viewport.return_value = mocked_viewport_obj
> +        # 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)


-- 
https://code.launchpad.net/~knightrider0xd/openlp/better-slide-scaling/+merge/289580
Your team OpenLP Core is subscribed to branch lp:openlp.