ENH: add `--screenshot` to the typhos CLI #566

klauer · 2023-08-09T16:38:56Z

Description

Add typhos --screenshot filename_pattern to take screenshots of typhos displays prior to exiting early (in combination with --exit-after).

Motivation and Context

Larger changes to layouts coming in #563
We should check screenshots from the current master and compare them with #563 to ensure that the latter is strictly an improvement

How Has This Been Tested?

Test suite and interactively

Where Has This Been Documented?

This PR text and pre-release notes

Screenshots

$ typhos --fake-device ophyd.EpicsMotor[] --screenshot "{device.name}.png" --exit-after 1
[2023-08-09 09:40:41] - WARNING - Exiting typhos early due to --exit-after=1.0 CLI argument.
[2023-08-09 09:40:41] - INFO - Saving screenshot of 'Typhos Suite - EpicsMotor': 'EpicsMotor' to 'EpicsMotor.png'

klauer · 2023-08-09T16:59:07Z

typhos/suite.py

+                )
+                continue
+
+            filename = filename_format.format(


This means that the CLI --screenshot can be formatted with the following variables:
suite_title, widget_title, device, name (or any attribute thereof)

This should be better communicated in the CLI description

tangkong

I think the results here speak for themselves. A stray thought from me and not much else

tangkong · 2023-08-09T18:46:26Z

typhos/tests/test_suite.py

    handle = io.StringIO()
    save_suite(suite, handle)
    handle.seek(0)
    devices = [device.name for device in suite.devices]
    assert str(devices) in handle.read()


-def test_suite_save(suite, monkeypatch):
+def test_suite_save_screenshot(suite: TyphosSuite, device: MockDevice):
+    with tempfile.NamedTemporaryFile(mode="wb") as fp:


I've been using pytest's tmp_path fixtures for temporary file generation, and I'm surprised/ashamed I didn't know about python's built-in tempfile until just now. I wasn't able to google an appreciable difference between the two so I have no further feedback 🤷

pytest fixtures generally feel like the right way of doing things in the test suite - I'm just more familiar with this standard lib one. I'll have to peek at the tmp_path API a bit out of curiosity...

tangkong · 2023-08-09T19:15:35Z

typhos/suite.py

+                image = display.to_image()
+            else:
+                # This is a fallback for if/when we don't have a TyphosDisplay
+                image = utils.take_widget_screenshot(display)


Do you know how often this happens? I was under the impression most everything was a TyphosDeviceDisplay

It shouldn't ever happen in the current codebase. The truth is the following:

I added utils.take_widget_screenshot first, added tests and had it all working

Then I shifted to "it'd be better to save each device display"

Then I recalled that I had already made to_image on TyphosDeviceDisplay (a few years ago perhaps?)

Then I needed to justify the work from (1) so this else clause stayed in

ZLLentz

Looks great to me

I think the only thing missing is something you pointed out yourself: docs about the format strings that are accepted in the screenshot filename.

ZLLentz · 2023-08-09T20:35:25Z

typhos/cli.py

@@ -25,6 +29,31 @@

 logger = logging.getLogger(__name__)

+
+class TyphosArguments(types.SimpleNamespace):


I'm a big fan of using placeholder classes like this for arg type handling

ZLLentz · 2023-08-09T20:37:41Z

typhos/cli.py


-        return launch_suite(suite, initial_size=initial_size)
+    return launch_suite(suite, initial_size=initial_size)


I appreciate the cleanup/dedentation you did here

ZLLentz · 2023-08-09T20:40:32Z

typhos/cli.py



 def typhos_cli(args):
    """Command Line Application for Typhos."""
-    args = parser.parse_args(args)
+    args = typing.cast(TyphosArguments, parser.parse_args(args))


There is also a built-in feature of argparse to fill a namespace instead of creating one. I'm not sure which way is better from a maintainability and type-checking standpoint.

args = parser.parse_args(args, TyphosArguments())

Ah, thanks for that tip! I think your way is better than the type cast 👍

ZLLentz · 2023-08-09T20:41:17Z

typhos/display.py

@@ -1405,6 +1405,9 @@ def add_device(self, device, macros=None):
        self.macros = self._build_macros_from_device(device, macros=macros)
        self.load_best_template()

+        if not self.windowTitle():
+            self.setWindowTitle(getattr(device, "name", ""))


Out of curiosity: which edge case is this fixing?

All widgets in Qt can have window titles, as you already know, in case they're displayed as-is in the window manager.

TyphosDeviceDisplay or its subclasses may choose to customize the window title but they don't currently. This says "if there's no title set, at least give it the device name"

Then to answer your actual question: this title is then used during the screenshot process, with output saying "Taking a screenshot of (suite title) (device display title)":
[2023-08-09 09:40:41] - INFO - Saving screenshot of 'Typhos Suite - EpicsMotor': 'EpicsMotor' to 'EpicsMotor.png'

ZLLentz · 2023-08-09T20:43:31Z

typhos/utils.py

@@ -1704,3 +1704,62 @@ def set_no_edit_style(object: QtWidgets.QLineEdit):
                "QLineEdit { background: transparent }"
            )
        object.setReadOnly(True)
+
+
+def take_widget_screenshot(widget: QtWidgets.QWidget) -> Optional[QtGui.QImage]:


I like how these utils aren't actually typhos-specific and may have broader use in the future

klauer · 2023-08-14T17:14:27Z

One CI job timed out during the test suite - re-running it prior to merging

klauer added 5 commits August 9, 2023 08:56

ENH: add in some screenshotting utilities

54d698c

MNT: warning with title info

5f9e00f

MNT: set TyphosDeviceDisplay title to device name if unset

881dff4

TST: screenshots/utils

bd02669

DOC: pre-release notes

2013b3d

klauer marked this pull request as ready for review August 9, 2023 16:46

klauer requested review from ZLLentz and tangkong August 9, 2023 16:52

klauer commented Aug 9, 2023

View reviewed changes

klauer mentioned this pull request Aug 9, 2023

ENH: layout changes for compact complex screens + row positioner widget #563

Merged

4 tasks

tangkong approved these changes Aug 9, 2023

View reviewed changes

ZLLentz approved these changes Aug 9, 2023

View reviewed changes

klauer added 2 commits August 9, 2023 13:55

MNT: use built-in parse_args support for ns

599911b

MNT: better document CLI parameter

93dadca

klauer merged commit be1a7b7 into pcdshub:master Aug 14, 2023
7 of 9 checks passed

klauer deleted the enh_screenshot branch August 14, 2023 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: add `--screenshot` to the typhos CLI #566

ENH: add `--screenshot` to the typhos CLI #566

klauer commented Aug 9, 2023 •

edited

Loading

klauer Aug 9, 2023

tangkong left a comment

tangkong Aug 9, 2023

klauer Aug 9, 2023

tangkong Aug 9, 2023

klauer Aug 9, 2023

ZLLentz left a comment

ZLLentz Aug 9, 2023

ZLLentz Aug 9, 2023

ZLLentz Aug 9, 2023

klauer Aug 9, 2023

ZLLentz Aug 9, 2023

klauer Aug 9, 2023

ZLLentz Aug 9, 2023

klauer commented Aug 14, 2023

		@@ -25,6 +29,31 @@

		logger = logging.getLogger(__name__)


		class TyphosArguments(types.SimpleNamespace):


		return launch_suite(suite, initial_size=initial_size)
		return launch_suite(suite, initial_size=initial_size)

ENH: add --screenshot to the typhos CLI #566

ENH: add --screenshot to the typhos CLI #566

Conversation

klauer commented Aug 9, 2023 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Where Has This Been Documented?

Screenshots

Choose a reason for hiding this comment

tangkong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZLLentz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klauer commented Aug 14, 2023

ENH: add `--screenshot` to the typhos CLI #566

ENH: add `--screenshot` to the typhos CLI #566

klauer commented Aug 9, 2023 •

edited

Loading