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.
_______________________________________________
Mailing list: https://launchpad.net/~openlp-core
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openlp-core
More help : https://help.launchpad.net/ListHelp
