Skip to content

Screen.find by Buffer #209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

lewisl9029
Copy link

@lewisl9029 lewisl9029 commented Feb 8, 2021

Hi there!

This is a very early proof of concept for #204 , definitely not ready to merge yet(got it a bit closer to mergeable over the weekend).

Just wanted throw together a quick and dirty working implementation (I've tested this locally and it seems to be working as expected) in order to iron out the implementation details and kickstart the discussion on what kind of API we want to offer in the final version.

In this version, I've added a separate screen.findImage method with an API that accepts the raw buffer data along with a string identifier. A lot of stuff downstream expect to have the string path available for logging, identifying callbacks, etc, so the string identifier acts as a substitute for these use cases since using the raw buffer data there wouldn't make any sense. This id could also become useful for as a cache key for the needle caching feature you had planned. Open to other ideas on how to handle this if you had something else in mind though.

For the final API, here are the approaches I've considered:

  1. Keep 2 separate methods for finding by buffer vs finding by file path. In this world I'd imagine we may want to use more specific names to avoid confusion and misuse, i.e. find -> findImageFile, findImage -> findImageData.

  2. Refactor into a single polymorphic find method that accepts both image path and the id + buffer.

I'm personally a fan of approach 1, since it gives us a bit more flexibility in how much or little to share between the methods. If in the future we do want to add a single polymorphic find function as an API, that can be added pretty trivially on top of the two separate APIs, whereas if we start with approach 2 and later want to move to approach 1, we'd have to unwind a bunch of internal branching that could end up getting nasty down the line.

In terms of implementation details, I was thinking of maybe changing the find by path version to first read the image file into a Buffer, so both implementations can just use the Buffer-based logic from that point onwards, which would simplify things quite a bit compared to what's currently in the branch where I've copy pasted everything downstream. I went ahead and made this refactor.

Wanted to get your preferences & thoughts on these topics before continuing further. WDYT?

@s1hofmann
Copy link
Member

Hi @lewisl9029 👋

Thank you for providing a POC #204, awesome!
Unfortunately, it'll take some more time until I can focus on reviewing it properly as I'm currently moving into a new home.
As soon as I'll have some spare time again, I'll get back to you!

@lewisl9029 lewisl9029 force-pushed the screen-find-by-buffer branch 2 times, most recently from d14fadd to fce10c3 Compare February 14, 2021 01:27
@lewisl9029 lewisl9029 force-pushed the screen-find-by-buffer branch from fce10c3 to 29733a6 Compare February 14, 2021 01:40
@lewisl9029 lewisl9029 force-pushed the screen-find-by-buffer branch from 29733a6 to d8e846a Compare February 14, 2021 01:58
@s1hofmann
Copy link
Member

@lewisl9029 I'm really sorry, it took a long time, but his functionality is going to land in 2.0.0 which is about to be released in the very near future.

@s1hofmann s1hofmann closed this Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants