phillco / talon-axkit

Talon macOS accessibility magic!
MIT License
47 stars 12 forks source link

dictation_context: use talon.ui import to fix running on windows #21

Closed phillco closed 2 years ago

phillco commented 2 years ago

Per Slack discussion with aegis, talon.ui.mac shouldn't imported from it all, we should import use the platform independent version instead.

Specifically, the mac package fails to raise ImportError properly:

2022-04-04 15:04:10 ERROR user.talon_axkit.dictation.dictation_context (C:\Users\nriley\AppData\Roaming\talon\user\talon_axkit\dictation\dictation_context.py) import failed
   15:                                    threading.py:930* # loader thread
   14:                                    threading.py:973*
   13:                                    threading.py:910*
   12:                         app\resources\loader.py:887|
   11:                         app\resources\loader.py:823|
   10:                         app\resources\loader.py:641|
    9:                         app\resources\loader.py:677|
    8:                         app\resources\loader.py:731|
    7:                         app\resources\loader.py:427| # [stack splice]
    6:                           importlib\__init__.py:127|
    5:                         app\resources\loader.py:248|
    4:                         app\resources\loader.py:243|
    3: user\talon_axkit\dictation\dictation_context.py:9  | from talon.mac.ui import Element
    2:                         app\resources\loader.py:361|
    1:                                 talon\mac\ui.py:29 |
AttributeError: 'Library' object has no attribute 'tl_mac_has_separate_spaces'
nriley commented 2 years ago

Do we want to add a if typing.TYPE_CHECKING per Slack?

I wasn't sure if that is helpful in our case as it'll still generate an error if undefined!

phillco commented 2 years ago

Do we want to add a if typing.TYPE_CHECKING per Slack?

Sorry, I didn't follow this part of the discussion. Is this suggesting we set mypy?

nriley commented 2 years ago

Do we want to add a if typing.TYPE_CHECKING per Slack?

Sorry, I didn't follow this part of the discussion. Is this suggesting we set mypy?

This constant allows you to distinguish between evaluation for type checking and evaluation for execution. I think this isn't too useful here as the problem isn't with type checking per se, it's that talon.ui.Element isn't even defined on Windows (or Linux).

pokey commented 2 years ago

Do we want to add a if typing.TYPE_CHECKING per Slack?

Sorry, I didn't follow this part of the discussion. Is this suggesting we set mypy?

This constant allows you to distinguish between evaluation for type checking and evaluation for execution. I think this isn't too useful here as the problem isn't with type checking per se, it's that talon.ui.Element isn't even defined on Windows (or Linux).

Yeah I think it wouldn't necessarily solve any problems, just was thinking might be good hygiene, but the docs seem to imply that it's only worth the complexity if the module is expensive to import, so maybe we should just leave it

phillco commented 2 years ago

@nriley @pokey mind stamping this one? Thanks

pokey commented 2 years ago

Pretty sure @nriley has to do it because he requested changes

phillco commented 2 years ago

Ah, interesting.

phillco commented 2 years ago

@nriley think you actually have to explicitly give this an "approved" code review, because you previously requested changes (that's what pokey is referring to) :)

Screen Shot 2022-04-17 at 12 15 48 PM
nriley commented 2 years ago

OK, that works! Still learning here…

phillco commented 2 years ago

It's actually new to me too -- our GitHub Enterprise instance at work doesn't have this feature yet, so it actually causes a little bit of conflict (e.g. "I requested changes but then somebody else approved it and you merged it without addressing my concern, etc etc") :)