X-1.0rc2: findAll() fails instead of returning None --- doc updated

[anatoly techtonik]

the docs have been updated to clearly describe the current behavior of findAll()

To make findAll() easier to use, a request bug was posted:
bug 779099: [request] findAll() should force getLastMatch() to return None if nothing was found

*** A workaround to avoid a failing findAll():
matches = findAll(img) if exists(img, 0) else ()
for x in matches:
   pass # will not execute if nothing found
--------------------------------------------------------------------------

According to docs, findAll() should return None if images are not found, but it fails instead.
Docs: http://sikuli.org/docx/region.html#Region.findAll
Command: fields = findAll([image here])

[profile] Finder.findAll START
[profile] Finder.findAll END: 1074ms
[error] Stopped
[error] An error occurs at line 6
[error] Error message: Traceback (most recent call last):
  File "C:\Users\useri\AppData\Local\Temp\sikuli-tmp2446174342086249181.py", line 6, in
    lettu_fields = findAll("1304497590779.png")
  File "C:\SikuliX\sikuli-script.jar\Lib\sikuli\Region.py", line 76, in findAll
  Line 14, in file C:\Users\useri\AppData\Local\Temp\sikuli-tmp2446174342086249181.py

   at org.sikuli.script.Region.handleFindFailed(Region.java:349)
  at org.sikuli.script.Region.findAll(Region.java:386)
  at org.python.proxies.sikuli.Region$Region$1.super__findAll(Unknown Source)
  at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
  at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
  at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
  at java.lang.reflect.Method.invoke(Unknown Source)

   org.sikuli.script.FindFailed: FindFailed: can not find 1304497590779.png
  Line 14
, in file C:\Users\useri\AppData\Local\Temp\sikuli-tmp2446174342086249181.py

[RaiMan (raimund-hocke)]

I will change the docs accordingly, since it is in fact the current behavior.

If you want to have that findAll() returns None instead of failing, you have to use setThrowException(False) for the respective region (see docs: http://sikuli.org/docx/region.html#exception-findfailed)

[RaiMan (raimund-hocke)]

Had a look at the docs:

It is a bit tricky, if you look only at the description of findAll():

Returns: .... None if not found

But in the preface of class Region, it says:

As a default, if the visual object (image or text) cannot be found, Sikuli will stop the script by raising an Exception FindFailed. This follows the standards of the Python language, so that you could handle such exceptions using try: ... except: ....

And this is the fact for ALL explicit and implicit find operations (except exists()).

So the docs are correct in a weird way:
it only returns None, IF it returns - but in the standard it does not return at all, it stops with an exception ;-)

Since it is hair-splitting and not user friendly, I will find a way to rewrite the docs to be clear for that case (all explicit and implicit find operations affected)

[RaiMan (raimund-hocke)]

@ anatoly: ?????

I have changed the docs, so it should be clearer now.

findAll() like all implicit and explicit find operations raise exception FindFailed in case of not found. So they only come back and return None in case FindFailded should be ignored (Region.setThrowException(False)).

[anatoly techtonik (techtonik)]

http://sikuli.org/docx/region.html#Region.findAll
There is a mistake - returns 'one ore more'. Like when Sikuli is used to control mines, it results in more ore extracted. =)

I'd rewrite this statement as `Returns: matched objects as an iterator or fails`with `fails` as a link. Less text - easier to read - makes Sikuli more intuitive, i.e. more awesome.

For me returning None when nothing is found is perfectly fine for a scripting language. Exceptions seriously complicate stuff, especially in findAll() case. IMO it should not be default if the default is not publicly grounded (e.g. like http://www.pyside.org/docs/pseps/psep-0100.html). I see the reasoning behind debug, but I believe there is a much better way - by producing debug messages about how much results findAll() returns for certain debug log level. Then, using transparent log window that's always on top (and global shortcuts), it will be possible to debug execution more conveniently.

Maybe it is useful to add parameter for raising exception into findAll()?

http://sikuli.org/docx/region.html#patternnotfound
Is there any difference between "as a default" and "by default"?

[anatoly techtonik (techtonik)]

@RaiMan: Launchpad changed the status before I clicked submit button.

[RaiMan (raimund-hocke)]

@ anatoly
thanks again for the doc hints. changed it accordingly.
Any help, to improve the docs, make things clear and user friendly or even turn them into real english (sorry for my german english ;-) are always welcome.

--- "For me returning None when nothing is found is perfectly fine for a scripting language."
That might be right in general, but with Sikuli we have a slightly different situation. When they started, the default was "do nothing, if not found". This left the users with scripts, that might run and finish without doing nothing and no information on what happened. Now we have a compromise: If a find is not successful, the script stops with an error, since normally it does not make any sense to continue.
I usually use exists() for workflow situations, where you have different options or it is possible to recover from a not found. If a FindFailed exception in such a workflow is raised, I know, that something did not work as expected. I analyze the situation and try to make the workflow more robust.
The average Sikuli user mostly has workflows that consist of a sequence of find()'s, click()'s and wait()'s. For them I think the default behavior together with the new interactive FindFailed options is a good solution.
Others who have some scripting/programming experience will find ways to solve more complex situations.

--- "Maybe it is useful to add parameter for raising exception into findAll()?"
definitely no. The current behavior is consistent with the rest of the API. What you could request is an existsAll(), which would be again consistent (it is up to you to post a request bug for that).

If I would have the situation, I would do this:
matches = findAll(img) if exists(img, 0) else False

[anatoly techtonik (techtonik)]

@ RaiMan
It makes sense. Even though programming Sikuli becomes more error prone, it becomes more cumbersome (more like Java), but at the same time maybe it is good to have this by default, because otherwise I wouldn't even consider this alternative. =) Maybe for new users it is good to have some "starter" or "novice" mode with no complicated words like "exception".

> If I would have the situation, I would do this:
> matches = findAll(img) if exists(img, 0) else False

This has a drawback that it runs match two times. I resorted to handling exception. Not that beautiful, but more practical. =)

I've gathered more comments while reviewing the docs in response to your reply:

1. findAll() http://sikuli.org/docx/region.html#Region.findAll - doesn't mention getLastMatches(), the explanation is a little more verbose that in find()

2. neither find*() nor http://sikuli.org/docx/region.html#patternnotfound explains what happens with lastMatches when exception occurs - can I 100% rely that it will be None?

3. http://sikuli.org/docx/region.html#patternnotfound may have an example of handling exception - while it may seem obvious, it was once hard to find proper syntax in Python docs

  try:
    region.findAll(what)
  except FindFailed:
    pass
  for w in region.getLastMatches():
    ...

[RaiMan (raimund-hocke)]

Wow, really thankful for the feedback. It is always a pain, to write some docs and be the one yourself to check it for usability.
I will go through and enhance the docs.

Whats really missing is some beginners guide, that is even "configurable" based on your knowledge. I have it on my list and want to combine it with the extension guide, which is extremely enhanced during the last weeks by Tom.

One more thing:
you say:
matches = findAll(img) if exists(img, 0) else False
This has a drawback that it runs match two times.

principally this is true, but it is only one additional search (mind the 0 as second parameter) and not the standard up to ten searches (scanrate is 0.3 by default, default waiting time 3 seconds). So I think this is acceptable and makes it lean and robust.

[anatoly techtonik (techtonik)]

exists(img, 0) seems to help. Thanks.

I am not an Englishman, but it seems the `seconds` parameter description can be improved as well.

From:
seconds – a number, which can have a fraction, as maximum waiting time in seconds. The internal granularity is milliseconds. If not specified, the auto wait timeout value set by Region.setAutoWaitTimeout() is used. Use the constant FOREVER to wait for an infinite time.

To:
seconds – maximum waiting time in seconds, may have a fraction up to milliseconds. By default uses auto wait timeout value, which is set by Region.setAutoWaitTimeout(). Use the constant FOREVER to wait indefinitely.