Skip to content
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

Prototype for moving core functionality into a Swift framework #3573

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

cmsj
Copy link
Member

@cmsj cmsj commented Dec 20, 2023

@latenitefilms @asmagill one for y'all to chew on :)

I've been thinking more about the possible roads to Hammerspoon 2.0 and it seems to me like there isn't enough energy or Swift experience in the project to tackle a ground-up rewrite.

So, it therefore seems to me to make sense to try and explore ways to modernise the project in isolated chunks.

We've previously figured out how to write an extension in pure Swift, but we never tackled what I think is probably the most important piece - extracting and isolating the core functionality of extensions, into a Framework. I've said before that I strongly dislike how intertwined all of the "Do Useful Thing" code is with the "Bridge with Lua" code.

This PR serves a a very simple example of how one can add a pure Swift framework that does useful things, while leaving the Lua bridging parts in ObjC.

Of particular note is an opportunity for bike-shedding around error handling - if you compare the changes to math_randomFloatFromRange with those to math_randomFromRange, in the former I am throwing an exception from Swift when the range values are impossible, and merely catching that exception in ObjC. In the latter I am validating the range from ObjC and assuming the Swift will always succeed.

Personally I prefer the former, mostly because keeps the ObjC code to just "get values out of Lua, call useful method, return values to Lua".

If we are ok with this sort of thing, we would be able to convert modules to this form one at a time (and take the opportunity to modernise their APIs, with appropriate backwards compatibility where at all possible) and then later rewrite the Lua bridging part to also be Swift. (I deliberately chose not to replace the Lua bridging parts with pure Swift in this example, because I suspect there will be cases where we want to partially migrate something)

Then separately we can consider when to rewrite the core binary in Swift, and then once all of those things have happened, we would have a mostly-Swift codebase, but one that still relied a lot on NSObject. We'd then be in a position to rewrite LuaSkin and converge on native Swift types, and by that stage everything will be neatly isolated/encapsulated such that it's relatively easy to also do that in chunks that are likely to succeed individually.

I don't know if any of those things will actually happen, but this seems to me like a plan which is more likely to succeed because it doesn't fail if we only ever do part of it.

@cmsj cmsj self-assigned this Dec 20, 2023
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request does not contain a valid label. Please add one of the following labels: ['pr-fix', 'pr-change', 'pr-feature', 'pr-maintenance']

Copy link

github-actions bot commented Dec 20, 2023

View Test Results

2 tests   2 ✔️  0s ⏱️
2 suites  0 💤
1 files    0

Results for commit ea66aa9.

♻️ This comment has been updated with latest results.

@latenitefilms
Copy link
Contributor

I love this!!! Will have a proper look/play soon!

@cmsj
Copy link
Member Author

cmsj commented Dec 21, 2023

This now covers three extensions - hs.math, hs.base64 and hs.camera.

hs.math is trivial, hs.base64 is also relatively simple, but showed a nice reduction in complexity (which, to be fair, we could have achieved using NSData without involving Swift), and hs.camera is a properly representative, complex extension with watchers.

hs.camera is not yet fully working - the isInUse KVO watcher doesn't seem to be working, but I'm not sure why yet. Possibly because the app needs Camera access permissions. Also I strongly dislike that one LuaSkin thing (callback lua ref) has now leaked into Hammertime, and one thing (camera object canary) is missing. Both of these call for switching the C-side userdata object to a struct that contains the canary, callback ref and camera object, rather than having the camera object be the userdata. This is something @asmagill and I have discussed previously and I've forgotten which of us preferred using structs and which preferred directly using ObjC objects as userdata, but whichever way round we were, this prototype indicates that structs are the only sane option if we want Hammertime to remain completely isolated from Lua (which we should want, because otherwise there's little point abstracting all the core functionality of the extensions into a framework).

Would welcome thoughts and reviews. I'll push to this a little more tomorrow to take care of the FIXMEs.

@asmagill
Copy link
Member

I am on the pro Object side because it simplifies allowing the use of one module's objects in another module without actually having to have any specific code to know how to handle the object -- it's always a (possibly distant) descendant of NSObject and can always be treated that way.

A structure could probably provide a similar "simplicity of mixing" if we insisted that the first three members in the structure were always the same and in the same order (the object, the callback, and the canary, for example) with any additional unique pieces of information afterwards -- then the structure could be cast to the base type to get at the core object if required in another module.

@cmsj
Copy link
Member Author

cmsj commented Dec 21, 2023

@asmagill we could also make it an ObjC class, with an id for the real object and callbackref/canary properties. That way at least we're protected against the fickle nature of struct offsets if we need to change things later

Edit: I'll rework this PR to do exactly that - having an HSuserdata class probably actually makes tons of sense.

@cmsj
Copy link
Member Author

cmsj commented Dec 21, 2023

Ok, so a hosting ObjC class turned out to not be great because then those have to be cached on the ObjC side as well as the Swift side having to cache its objects. Instead I added a raw pointer on the Swift side for the ObjC side to do what it likes with - in this case, storing a struct there which has the callbackRef and canary.

Oddly, the CMIOObjectRemovePropertyListenerBlock() call isn't working. It returns success, but the callback block is still called after an hs.reload(). It's sufficiently well guarded that it doesn't actually do anything (or crash), but I am totally baffled as to why it's happening.

That aside, I am now pretty happy with the state of hs.camera.

@cmsj
Copy link
Member Author

cmsj commented Dec 22, 2023

If anyone wants to play with the part that isn't working:

  1. Add a breakpoint on Camera.swift line 213
  2. Build & Run
  3. Run the Lua below in the HS console:
cam = hs.camera.allCameras()[1]
print(cam:name())
cam:setPropertyWatcherCallback(function() print("isInUse changed") end)
cam:startPropertyWatcher()

If you now fire up something that uses the webcam, you'll see isInUse changed printed in the HS console.

Now run hs.reload() - at this point our Camera object has been garbage collected (which you can verify with a breakpoint on libcamera.m line 503 and some stepping in (which will take you through Camera.swift line 230, where you can observe that CMIOObjectRemovePropertyListenerBlock() returns successfully).

Finally, either close or re-open the webcam-using app. You'll see the Camera.swift breakpoint is hit, even though it shouldn't be.

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.

None yet

3 participants