Kalmat / PyWinCtl

Cross-Platform module to get info on and control windows on screen
Other
191 stars 20 forks source link

`_pywinctl_macos.title` typed as potentially None #19

Closed Avasam closed 2 years ago

Avasam commented 2 years ago

_pywinctl_macos.title is typed as potentially None. That seems incorrect. A quick glance at the code, it seems self._winTitle should always be a string. And the title property defaults to an empty string if not found in titles.

The other OSes are also typed to only return a str

Kalmat commented 2 years ago

Hi! Thank you again! This is something I had to struggle a lot with, and still not sure if I managed to solve it in the best possible way. Your opinion or a different point of view might be very useful.

MacOSWindow class uses Apple Script (the only way I found to control other apps' windows). Since Catalina, there was no reference I could use to uniquely identify a window (e.g. no window id or window number nor similar), so I had to rely on the window title as a "window handle". The problem is that the title can change, so this "handle" would be totally useless. This is why I had to introduce updatedTitle() method to try to find the new window title (no guaranteed since this method uses a similarity check, so that new title can be either not found, or belong to another window).

According to all this, "" (empty) title means that the window has no title, meanwhile None title means title has changed and it is not valid any more, letting the user the decission to use updatedTitle() or not, and what to do with it (e.g. trust it or make a new search in order to create a new updated MacOSWindow object). As I said before, not sure if this is a good solution.

I tried to explain all this in the documentation of these methods, but maybe it's not very clear!:

@property
def title(self) -> Union[str, None]:
    """
    Get the current window title, as string.
    IMPORTANT: window title may change. In that case, it will return None.
    You can use ''updatedTitle'' to try to find the new window title.
    You can also use ''watchdog'' submodule to be notified in case title changes and try to find the new one (Re-start watchdog in that case).

    :return: title as a string or None
    """

@property
def updatedTitle(self) -> str:
    """
    Get and updated title by finding a similar window title within same application.
    It uses a similarity check to find the best match in case title changes (no way to effectively detect it).
    This can be useful since this class uses window title to identify the target window.
    If watchdog is activated, it will stop in case title changes.

    IMPORTANT:

    - New title may not belong to the original target window, it is just similar within same application
    - If original title or a similar one is not found, window may still exist

    :return: possible new title, empty if no similar title found or same title if it didn't change, as a string
    """
Kalmat commented 2 years ago

Taking a look at the code, I think I didn't remember all this and decided to change it at a given moment by mistake, so it's incorrect now.

This:

@property
def title(self) -> Union[str, None]:
    titles = _getAppWindowsTitles(self._appName)
    if self._winTitle not in titles:
        return ""
    return self._winTitle

Should be:

@property
def title(self) -> Union[str, None]:
    titles = _getAppWindowsTitles(self._appName)
    if self._winTitle not in titles:
        return None
    return self._winTitle
Avasam commented 2 years ago

In this case there's a semantic difference between "" and None, and it makes sense. So does the documentation. I just fail to see how it'll ever return None. In fact, a simple test in VSCode (since changing the current file changes the window name)

>>> import pywinctl
>>> pywinctl.getActiveWindow()
MacOSWindow(hWnd=<NSRunningApplication: 0x7ff36cf1a340 (com.microsoft.VSCode - 260)>)
>>> window = pywinctl.getActiveWindow()
>>> window.title
'__init__.py — Auto-Split'
# Change file to change the title
>>> window.title
''
>>> 

Edit, just saw your new comment, that would make more sense yeah :)

Kalmat commented 2 years ago

HAHAHA! Yes, I think I spotted that part of the code and (like you) thought to myself it was wrong to return None, and replaced it by ""!!

Thank you so much for your comments!