feat: Enhance take_screenshot for multi-monitor support

[onyedikachi-david]

Fixes #766 /claim #766

What kind of change does this PR introduce?

Feature

Summary

This PR introduces support for multi-monitor setups in the take_screenshot function. It includes the following changes:

• Implemented get_current_monitor to determine the monitor where the cursor is currently located.
• Modified take_screenshot to use get_current_monitor for capturing the correct monitor.
• Added comprehensive tests for take_screenshot with multiple monitors.
• Mocked get_current_monitor and mss.mss in tests to simulate multiple monitor configurations.
• Ensured take_screenshot correctly handles multiple monitors and returns the expected screenshot.

Checklist

• [x] My code follows the style guidelines of OpenAdapt
• [x] I have performed a self-review of my code
• [x] If applicable, I have added tests to prove my fix is functional/effective
• [x] I have linted my code locally prior to submission
• [x] I have commented my code, particularly in hard-to-understand areas
• [x] I have made corresponding changes to the documentation (e.g. README.md, requirements.txt)
• [x] New and existing unit tests pass locally with my changes

How can your code be run and tested?

To run and test the code:

1. Ensure you have the necessary dependencies installed.
2. Run the test suite using pytest:

   pytest tests/openadapt/test_monitors.py

3. Verify that all tests pass, including the new tests for multi-monitor support.

Other information

No additional context needed.

[abrichr]

Thank you @onyedikachi-david ! What do you think about saving taking a screenshot of all of the available monitors, rather than just the active one? This can provide useful additional context to the model if something changes on one of the screens.

[onyedikachi-david]

Thank you @onyedikachi-david ! What do you think about saving taking a screenshot of all of the available monitors, rather than just the active one? This can provide useful additional context to the model if something changes on one of the screens.

Okay, I'll implement it. just that I was being mindful of how it is being used across the codebase. If there are more than one monitors, then a tuple or list will be returned, which will affect the screenshot function usage across the codebase.

[abrichr]

Thank you please do. Maybe keep the current functionality behind a config param.

[onyedikachi-david]

Thank you please do. Maybe keep the current functionality behind a config param.

New take_screenshot function using config:

def take_screenshot() -> list:
    """Take screenshots of the current monitor or all available monitors.

    Returns:
        list of PIL.Image.Image: A list of screenshot images.
    """
    with mss.mss() as sct:
        monitors = sct.monitors
        screenshots = []

        if config.CAPTURE_ALL_MONITORS:
            for monitor in monitors[1:]:  # Skip the first entry which is a union of all monitors
                sct_img = sct.grab(monitor)
                image = Image.frombytes("RGB", sct_img.size, sct_img.bgra, "raw", "BGRX")
                screenshots.append(image)
        else:
            current_monitor = get_current_monitor(monitors)
            sct_img = sct.grab(current_monitor)
            image = Image.frombytes("RGB", sct_img.size, sct_img.bgra, "raw", "BGRX")
            screenshots.append(image)

        return screenshots

If I got you right, I should go ahead and change it to have a list return type, if that is the case, then its current implementation in models.py

 @classmethod
    def take_screenshot(cls: "Screenshot") -> "Screenshot":
        """Capture a screenshot."""
        image = utils.take_screenshot()
        screenshot = Screenshot(image=image)
        return screenshot

will change to this:

@classmethod
    def take_screenshot(cls: "Screenshot") -> list:
        """Capture a screenshot."""
        images = take_screenshot(all_monitors=all_monitors)
        screenshots = [cls(image=image) for image in images]
        return screenshots

And its usage will also change:

screenshots = Screenshot.take_screenshot(all_monitors=True)
for idx, screenshot in enumerate(screenshots):
    screenshot.image.show(title=f"Monitor {idx + 1}")

[abrichr]

I think we want to return a single PIL.Image containing all screenshots. Any way to determine relative positioning of the screens?

[onyedikachi-david]

I think we want to return a single PIL.Image containing all screenshots. Any way to determine relative positioning of the screens?

Yes, it is best to return PIL.Image. Let me think of a way to go about it.

[onyedikachi-david]

@abrichr I just implemented the requested changes.

[onyedikachi-david]

@abrichr I don't know if you've found time to review the PR.

[onyedikachi-david]

Thank you for putting this together @onyedikachi-david ! And thank you for your patience.

I left a few comments concerning performance. Because we call take_screenshot in a tight loop during recording, it's important that it is as performant as possible.

Can you please run python -m openadapt.record with CAPTURE_ALL_MONITORS = True and = False (while recording similar behavior, e.g. opening the calculator and clicking 2 x 3), and paste the performance plots here? (The path to the performance plot is logged to stdout at the end of the recording.)

You're welcome. Thanks for the feedback and all. Two issues though:

1. I don't have an external monitor to test.
2. This one isn't much of an issue though, I'll see how to run it on my Linux machine (although it didn't work last time I tried); my Mac OS crashed yesterday.

Just give me awhile to implement the requested changes.

[onyedikachi-david]

Please review @abrichr, just implemented the requested changes, no screenshot yet though. Yet to fix out my Mac OS crash

[onyedikachi-david]

@abrichr I'm sorry for keeping a stale PR. I hope it isn't blocking anything. I'm still trying to get my macOS setup together. I was using a Hackintosh (dual-booted with Linux), but it got corrupted while I was reinstalling Docker. I'm still figuring out how to resolve this without making things worse. I don't want to lose my Linux files/partition, which is still functional.

[abrichr]

@onyedikachi-david thank you for the update. For now it is not blocking. 🙏

[onyedikachi-david]

@onyedikachi-david thank you for the update. For now it is not blocking. 🙏

Okay.