[WIP] LuaSkin and coroutines #2308
Conversation
fixed inverted logic for validation with webview.reload and possible dispatch related issue when parameters supplied
|
Amazing! Will test on Monday. |
|
@asmagill - Hope you're keeping safe and healthy in these crazy times! Just checking in... what are the chances of merging this code into the Hammerspoon master branch? @stickler-ci issues aside, it seems like this pull request basically just fixes The Film & Television in Australia is basically now completely on hold for the foreseeable future, so apologies in advance, as I'll no doubt have a lot more time on my hands to poke at Hammerspoon & CommandPost! |
|
I consider it such and have been using it on both Mojave and Catalina since this was originally put up here. As long as you do not use coroutines, this doesn't introduces any new bugs (that wouldn't also be introduced by #2307) and does in fact fix a few bugs (as does #2307). If you do use coroutines... well, it is possible that I've missed one or two places where I should be using The real test would be for people to use it and code with coroutines. I've started a few, but most of the ones I want to do to replace things that are a little slow or laggy also require @cmsj, the real question is what level of example or testing will make you comfortable incorporating this? I can put together a short write up (I meant to already but got distracted) so that it's obvious to those of us working directly on the Objective-C extensions what changes should be made and where. Beyond that... what would help your comfort level? I don't think many of our users will be rushing to use coroutines, so most won't notice a difference... it's not an obvious way of thinking about things until you run into slow or laggy code, and most of Hammerspoon is designed around a trigger-response model to do something short and specific in response to a pre-defined occurrence (a key stroke, a timer, an event, etc.). But for those of us trying to make Hammerspoon seem more responsive or who do write more complex, longer running code (@latenitefilms and his crew), it becomes a very useful addition to support them -- much simpler (and certainly much more understandable to others who may want to modify it) in many cases than rewriting a threaded version solely in Objective-C. As a reminder, at present, if anyone does use coroutines that either use any Hammerspoon specific Objective-C addition or that resume/yield in different code "blocks" (i.e. there is any Hammerspoon application idle time between them), then Hammerspoon currently crashes (and often in a way that the exception handler can't catch) and we have yet to discover any way to detect or prevent this. Ultimately, we need this pull, or we need to remove the coroutine library and make it clear that we're only supporting a subset of Lua. |
|
@asmagill - I just finally worked out how to merge this branch into CommandPost, so will keep you posted on how we go with it! We're also now using |
|
You already know this, but just FYI - we're also using these extensions: 2020-03-22 09:18:28: 09:18:28 ** Warning: LuaSkin: Deprecated LuaSkin method [LuaSkin shared] invoked by `/Users/chrishocking/Github/CommandPost/src/extensions/hs/_asm/cfpreferences/internal.so`. Please notify developer of module to upgrade as this method is unsafe for use with coroutines and may disappear in a future Hammerspoon release.
2020-03-22 09:18:28: 09:18:28 ** Warning: LuaSkin: Deprecated LuaSkin method [LuaSkin shared] invoked by `/Users/chrishocking/Github/CommandPost/src/extensions/hs/_asm/xml/internal.so`. Please notify developer of module to upgrade as this method is unsafe for use with coroutines and may disappear in a future Hammerspoon release. |
|
Your |
|
Just spotted this too: 2020-03-22 10:40:51: 10:40:51 ** Warning: LuaSkin: Deprecated LuaSkin method [LuaSkin shared] invoked by `/Users/chrishocking/Github/CommandPost-App/build/CommandPost.app/Contents/Resources/extensions/hs/image/internal.so`. Please notify developer of module to upgrade as this method is unsafe for use with coroutines and may disappear in a future Hammerspoon release. |
|
At present, the warnings are just that -- warnings, and they will only appear once per application launch. As long as you don't use methods from those specific modules (or the specific function/method, if you can identify which it is within The The I'm hoping to have a writeup added to the wiki sometime Sunday evening/Monday morning as well -- I've finished (though not pushed) a document outlining the problem and giving a brief introduction of coroutines and why I think we haven't really seen an issue before, but I'm only about half way done with the companion page which describes how to properly fix existing modules and write them going forwards based upon the pull I am recommending. |
|
Ah, yes, you're absolutely right - I had |
|
@cmsj, @latenitefilms -- it's a first draftish kind of thing, but I've documented the problem and how to apply this proposed solution going forward. Note that all of the fixes outlined in the second document have already been applied in this pull to the core modules; the documentation is for future modules and for converting the third party ones that still exist outside of core at the moment. |
|
This is AMAZING. THANK YOU!! |
| --- * unlike `coroutine.yield`, this function does not allow the passing of (new) information to or from the coroutine while it is running; this function is to allow long running tasks to yield time to the Hammerspoon application so other timers and scheduled events can occur without requiring the programmer to add code for an explicit resume. | ||
| --- | ||
| --- * this function is added to the lua `coroutine` library as `coroutine.applicationYield` as an alternative name. | ||
| local resumeTimers = {} |
There was a problem hiding this comment.
(W241) variable 'resumeTimers' is mutated but never accessed
| --- * this function is added to the lua `coroutine` library as `coroutine.applicationYield` as an alternative name. | ||
| local resumeTimers = {} | ||
|
|
||
| hs.coroutineApplicationYield = function(delay) |
There was a problem hiding this comment.
(W122) setting read-only field 'coroutineApplicationYield' of global 'hs'
| end | ||
| end | ||
|
|
||
| coroutine.applicationYield = hs.coroutineApplicationYield |
There was a problem hiding this comment.
(W142) setting undefined field 'applicationYield' of global 'coroutine'
|
|
||
| return setmetatable(module, { | ||
| -- only invoked if key not already in `module` | ||
| __index = function(self, key) |
Just spotted them - thanks heaps @asmagill !!! Will let you know if we run into any issues. |
|
For now this is merged by hand, along with #1646, but a lot of tests are failing, so I must have broken something in the merge. I'll investigate before I revert. |
|
Most of the test failures were LuaSkin, which I think I've resolved in 54a2b09 but the lifecycle stuff around this is very tricky and subtle, so it's possible I've made things worse in other ways. |
|
FWIW I never got it to run the test suite correctly myself, nor when Travis did it; but when I ran the failing tests individually in XCode, they all passed... I don't know the testing environment that well, and you'd mentioned that Travis sometimes fails in weird ways as well, so I haven't really done much on that front since... if you figure out something I missed or should be doing, don't hesitate to let me know. |
|
@cmsj - Awesome! Now that this is all merged into the master Hammerspoon branch, I'll attempt to manually merged across into CommandPost-App and see what breaks. |
|
Ok, and now we're back to the same 4 test failures that I was getting before, so I'm going to close this PR out. @asmagill I think I got all of the |
|
@cmsj & @asmagill - I've attempted to merge these changes into CommandPost-App, but I'm getting errors when I try and build, so I've screwed something up: Last line in I'll keep playing, but in the meantime any ideas? |
|
Never mind... worked it out. Will test it out and let you know any problems I find in a new issue (to keep things clean). |
|
Should the docs describing the changes to LuaSkin be added to the standard Hammerspoon wiki, or is there another place we should be using for LuaSkin specific documentation? Should the warning printed when LuaSkin detects the old usage of The specific docs I'm talking about are: And, any suggestions to make them better or clearer? Any other changes? |
|
@asmagill I think the HS wiki is a good place for them, they're helpful docs that don't really fit in the API docs. Also having the URL in the warning is a great idea. |
Putting it here for anyone interested to review.
This has #2307 already applied and will continue to track it if I find additional dispatch related or other bugs during this conversion.
It's minimally tested, but doesn't break anything if you're not using coroutines. So far, only some of the modules have been converted to the new LuaSkin constructor -- check the commits as I'm doing each module as a separate commit.
Coroutines do work now, as long as you stick to pure lua or functions/methods that only come from or depend on the converted modules... any unconverted module, even if it's loaded as a dependency is likely to still cause a crash if used from within a coroutine.
This is a work in progress and is not yet ready for formal testing or use.