Hammerspoon / hammerspoon

Staggeringly powerful macOS desktop automation with Lua
http://www.hammerspoon.org
MIT License
12.04k stars 584 forks source link

The road to 2.0 #2098

Closed cmsj closed 2 months ago

cmsj commented 5 years ago

I have thoughts :)

Over the last year or so, I've been isolating more and more of my config into Spoons, and one problem I keep running into is that most of our modules assume there will just be one callback function.

One way out of this would be to allow multiple callbacks to be registered, but I think that would likely end up feeling messy.

I think a much more usable setup would be if we redesign the modules around a scheme like hs.watchable. I really like the idea of random parts of Spoons/configs being able to just register themselves to receive events of a certain nature, and the underlying modules would take care of creating the relevant OS watchers and feeding events into the watchable-like system.

My thoughts would be that we'd just arbitrarily declare one of the upcoming 0.9.x releases as 1.0.0 and then get to work remaking all the callback modules, the first release of which would be 2.0.0, to signify the giant API breakage.

@asmagill and @latenitefilms - as our most active contributors, what are your thoughts on this?

cmsj commented 5 years ago

(other thoughts are certainly welcome, in case there is any doubt)

cmsj commented 5 years ago

In a more general sense, I'd see such a 2.0 as an opportunity to rethink a lot of our APIs, and bring them up to date with how things tend to work in more modern modules (e.g. getting rid of separate get/set methods)

latenitefilms commented 5 years ago

We have an extension in CommandPost called cp.prop that might be of interest?

To quote the manual:

--- === cp.prop ===
---
--- This is a utility library for helping keep track of single-value property states. Each property provides access to a single value. Must be readable, but may be read-only. It works by creating a table which has a `get` and (optionally) a `set` function which are called when changing the state.
---
--- --
---
--- ## Features:
--- ### 1. Callable
--- A `prop` can be called like a function once created. Eg:
---
--- ```lua
--- local value = true
--- local propValue = prop.new(function() return value end, function(newValue) value = newValue end)
--- propValue() == true     -- `value` is still true
--- propValue(false) == false   -- now `value` is false
--- ```
---
--- ### 2. Togglable
--- A `prop` comes with toggling built in - as long as the it has a `set` function. Continuing from the last example:
---
--- ```lua
--- propValue:toggle()  -- `value` went from `false` to `true`.
--- ```
---
---  **Note:** Toggling a non-boolean value will flip it to `nil` and a subsequent toggle will make it `true`. See the [toggle method](#toggle) for more details.
---
--- ### 3. Watchable
--- Interested parties can 'watch' the `prop` value to be notified of changes. Again, continuing on:
---
--- ```lua
--- propValue:watch(function(newValue) print "New Value: "...newValue) end) -- prints "New Value: true" immediately
--- propValue(false)    -- prints "New Value: false"
--- ```
---
--- This will also work on [AND](#and) and [OR][#or] properties. Any changes from component properties will trigger a notification.
---
--- ### 4. Observable
--- Similarly, you can 'observe' a prop as a `cp.rx.Observer` by calling the `observe` method:
---
--- ```lua
--- propValue:toObservable():subscribe(function(value) print(tostring(value) end))
--- ```
---
--- These will never emit an `onError` or `onComplete` message, just `onNext` with either `nil` or the current value as it changes.
---
--- ### 5. Combinable
--- We can combine or modify properties with AND/OR and NOT operations. The resulting values will be a live combination of the underlying `prop` values. They can also be watched, and will be notified when the underlying `prop` values change. For example:
---
--- ```lua
--- local watered   = prop.TRUE()               -- a simple `prop` which stores the current value internally, defaults to `true`
--- local fed       = prop.FALSE()              -- same as above, defautls to `false`
--- local rested    = prop.FALSE()              -- as above.
--- local satisfied = watered:AND(fed)        -- will be true if both `watered` and `fed` are true.
--- local happy     = satisfied:AND(rested)   -- will be true if both `satisfied` and `happy`.
--- local sleepy    = fed:AND(prop.NOT(rested)) -- will be sleepy if `fed`, but not `rested`.
---
--- -- These statements all evaluate to `true`
--- satisfied()     == false
--- happy()         == false
--- sleepy()        == false
---
--- -- Get fed
--- fed(true)       == true
--- satisfied()     == true
--- happy()         == false
--- sleepy()        == true
---
--- -- Get rest
--- rested:toggle() == true
--- satisfied()     == true
--- happy()         == true
--- sleepy()        == false
---
--- -- These will produce an error, because you can't modify an AND or OR:
--- happy(true)
--- happy:toggle()
--- ```
---
--- You can also use non-boolean properties. Any non-`nil` value is considered to be `true`.
---
--- ### 6. Immutable
--- If appropriate, a `prop` may be immutable. Any `prop` with no `set` function defined is immutable. Examples are the `prop.AND` and `prop.OR` instances, since modifying combinations of values doesn't really make sense.
---
--- Additionally, an immutable wrapper can be made from any `prop` value via either `prop.IMMUTABLE(...)` or calling the `myValue:IMMUTABLE()` method.
---
--- Note that the underlying `prop` value(s) are still potentially modifiable, and any watchers on the immutable wrapper will be notified of changes. You just can't make any changes directly to the immutable property instance.
---
--- For example:
---
--- ```lua
--- local isImmutable = propValue:IMMUTABLE()
--- isImmutable:toggle()    -- results in an `error` being thrown
--- isImmutable:watch(function(newValue) print "isImmutable changed to "..newValue end)
--- propValue:toggle()      -- prints "isImmutable changed to false"
--- ```
---
--- ### 7. Bindable
--- A property can be bound to an 'owning' table. This table will be passed into the `get` and `set` functions for the property if present. This is mostly useful if your property depends on internal instance values of a table. For example, you might want to make a property work as a method instead of a function:
---
--- ```lua
--- local owner = {
---    _value = true
--- }
--- owner.value = prop(function() return owner._value end)
--- owner:isMethod() -- error!
--- ```
---
--- To use a `prop` as a method, you need to `attach` it to the owning table, like so:
---
--- ```lua
--- local owner = { _value = true }
--- owner.isMethod = prop(function(self) return self._value end, function(value, self) self._value = value end):bind(owner)
--- owner:isMethod()                -- success!
--- owner.isMethod()                -- also works - will still pass in the bound owner.
--- owner.isMethod:owner() == owner -- is true~
--- ```
---
--- You can also use the [prop.bind](#bind) function to bind multple properties at once:
---
--- ```lua
--- local owner = { _value = true }
--- prop.bind(o) {
---     isMethod = prop(function(self) return self._value end)
--- }
--- owner:isMethod()                -- success!
--- ```
---
--- The [prop.extend](#extend) function will also bind any `cp.prop` values it finds:
---
--- ```lua
--- local owner = prop.extend({
---     _value = true,
---     isMethod = prop(function(self) return self._value end),
--- })
--- owner:isMethod()                -- success!
--- ```
---
--- The bound `owner` is passed in as the last parameter of the `get` and `set` functions.
---
--- ### 8. Extendable
--- A common use case is using metatables to provide shared fields and methods across multiple instances. A typical example might be:
---
--- ```lua
--- local person = {}
--- function person:name(newValue)
---     if newValue then
---         self._name = newValue
---     end
---     return self._name
--- end
---
--- function person.new(name)
---     local o = { _name = name }
---     return setmetatable(o, { __index = person })
--- end
---
--- local johnDoe = person.new("John Doe")
--- johnDoe:name() == "John Doe"
--- ```
---
--- If we want to make the `name` a property, we might try creating a bound property like this:
---
--- ```lua
--- person.name = prop(function(self) return self._name end, function(value, self) self._name = value end):bind(person)
--- ```
--- Unfortunately, this doesn't work as expected:
---
--- ```lua
--- johnDoe:name()          -- Throws an error because `person` is the owner, not `johnDoe`.
--- johnDoe.name() == nil   -- Works, but will return `nil` because "John Doe" is applied to the new table, not `person`
--- ```
---
--- The fix is to use `prop.extend` when creating the new person. Rewrite `person.new` like so:
---
--- ```lua
--- person.new(name)
---     local o = { _name = name }
---     return prop.extend(o, person)
--- end
--- ```
---
--- Now, this will work as expected:
---
--- ```lua
--- johnDoe:name() == "John Doe"
--- johnDoe.name() == "John Doe"
--- ```
---
--- The `prop.extend` function will set the `source` table as a metatable of the `target`, as well as binding any bound props that are in the `source` to `target`.
---
--- ## Tables
---
--- Because tables are copied by reference rather than by value, changes made inside a table will not necessarily
--- trigger an update when setting a value with an updated table value. By default, tables are simply passed in
--- and out without modification. You can nominate for a property to make copies of tables (not userdata) when
--- getting or setting, which effectively isolates the value being stored from outside modification. This can be
--- done with the [deepTable](#deepTable) and [shallowTable](#shallowTable) methods. Below is an example of them
--- in action:
---
--- ```lua
--- local value = { a = 1, b = { c = 1 } }
--- local valueProp = prop.THIS(value)
--- local deepProp = prop.THIS(value):deepTable()
--- local shallowProp = prop.THIS(value):shallowTable()
---
--- -- print a message when the prop value is updated
--- valueProp:watch(function(v) print("value: a = " .. v.a ..", b.c = ".. v.b.c ) end)
--- deepProp:watch(function(v) print("deep: a = " .. v.a ..", b.c = ".. v.b.c ) end)
--- shallowProp:watch(function(v) print("shallow: a = " .. v.a ..", b.c = ".. v.b.c ) end)
---
--- -- change the original table:
--- value.a             = 2
--- value.b.c           = 2
---
--- valueProp().a       == 2    -- modified
--- valueProp().b.c     == 2    -- modified
--- shallowProp().a     == 1    -- top level is copied
--- shallowProp().b.c   == 2    -- child tables are referenced
--- deepProp().a        == 1    -- top level is copied
--- deepProp().b.c      == 1    -- child tables are copied as well
---
--- -- get the 'value' property
--- value = valueProp()         -- returns the original value table
---
--- value.a             = 3     -- updates the original value table `a` value
--- value.b.c           = 3     -- updates the original `b` table's `c` value
---
--- valueProp(value)            -- nothing is printed, since it's still the same table
---
--- valueProp().a       == 3    -- still referencing the original table
--- valueProp().b.c     == 3    -- the child is still referenced too
--- shallowProp().a     == 1    -- still unmodified after the initial copy
--- shallowProp().b.c   == 3    -- still updated, since `b` was copied by reference
--- deepProp().a        == 1    -- still unmodified after initial copy
--- deepProp().b.c      == 1    -- still unmodified after initial copy
---
--- -- get the 'deep copy' property
--- value = deepProp()          -- returns a new table, with all child tables also copied.
---
--- value.a             = 4     -- updates the new table's `a` value
--- value.b.c           = 4     -- updates the new `b` table's `c` value
---
--- deepProp(value)             -- prints "deep: a = 4, b.c = 4"
---
--- valueProp().a       == 3    -- still referencing the original table
--- valueProp().b.c     == 3    -- the child is still referenced too
--- shallowProp().a     == 1    -- still unmodified after the initial copy
--- shallowProp().b.c   == 3    -- still referencing the original `b` table.
--- deepProp().a        == 4    -- updated to the new value
--- deepProp().b.c      == 4    -- updated to the new value
---
--- -- get the 'shallow' property
--- value = shallowProp()       -- returns a new table with top-level keys copied.
---
--- value.a             = 5     -- updates the new table's `a` value
--- value.b.c           = 5     -- updates the original `b` table's `c` value.
---
--- shallowProp(value)          -- prints "shallow: a = 5, b.c = 5"
---
--- valueProp().a       == 3    -- still referencing the original table
--- valueProp().b.c     == 5    -- still referencing the original `b` table
--- shallowProp().a     == 5    -- updated to the new value
--- shallowProp().b.c   == 5    -- referencing the original `b` table, which was updated
--- deepProp().a        == 4    -- unmodified after the last update
--- deepProp().b.c      == 4    -- unmodified after the last update
--- ```
---
--- So, a little bit tricky. The general rule of thumb is:
---
--- 1. If working with immutable objects, use the default `value` value copy, which preserves the original.
--- 2. If working with an array of immutible objects, use the `shallow` table copy.
--- 3. In most other cases, use a `deep` table copy.

We also have started using Reactive Extensions for Lua in cp.rx for a large majority of our code, that may also be of interest?

I'm definitely up for reviewing all the extensions and making sure they're consistent with their naming, terminology and getting/setting.

FWIW, here's some of the stuff I'd love to see in v1.0, before we move onto planning for v2.0:

1. Add "non activating" option to Console #2078 The fact that the Console always pops up when Hammerspoon is active is one of the most annoying things I come across. It would be great if there was a way to solve this.

2. hs.axuielement - Accessing Accessibility Objects with Hammerspoon #1630 We've been using hs._asm.axuielement solidly for a few years now, and it's fricken awesome. It's been rock solid for us, and such an important and useful extension. I'd love to see this finally added to the Hammerspoon core, so that others can start using it, and also offer ideas, suggestions and pull requests.

3. hs.menubar.new(false) appearing in system menubar? #1567 This bug is still pretty annoying, because every time I restart Hammerspoon it leaves some "ghost" items in the menu bar. I know @asmagill has been working on a new hs.menubar replacement in #1530 - so maybe this is something we can try and do for v1.0?

4. hs.preferences - Property List Editor with NSKeyedArchiver support #1498 There's quite a few issues and pull requests all tackling Property Lists & Defaults, so it would be great to finally consolidate and solve this feature requests for v1.0 as well. We currently use a mixture of hs.plist, hs._asm.cfpreferences and our own cp.plist

5. Add Ability to add Custom Items to Dock Icon Contextual Menu #1350 I feel like this would be a very useful feature to have for a lot of Hammerspoon users?

6. hs.touchbar - Add Extension for Touch Bar Support #1078 We've been using hs._asm.undocumented.touchbar for ages now - it's also awesome and rock solid. I reckon this is probably ready to merge into Hammerspoon too?

Sadly, between having a young child and running a business, my spare time is also lacking - so I haven't been able to spend as much time on Hammerspoon (and learning Obj-C) as I'd like, but I'm definitely all for trying to get v1.0 out the door, and start putting in the foundations for v2.0.

asmagill commented 5 years ago

@cmsj, re the overall plan, is the intention that updates to the 1.0 branch would be primarily bugfix/security related and less focused on adding new features?

@latenitefilms, I won't have time to look at cp.prop until this weekend, but at a glance, I wonder how much overlap it has with hs.watchable... it may be worth considering merging them if they really do provide at least some areas of overlap.

As to your suggestions for additions before 1.0, here are my thoughts:

  1. no disagreement; probably should make it an option to maintain current expectations and likely put the toggle into hs.console

  2. I've been considering merging this lately, though I'd need to review it and document it first -- it's been a while. My hesitations in the past on merging hs._asm.axuielement have basically fallen into two categories:

    1. the similarity in name and overlap with hs.uielement... and I'm not sure how much it would take to merge them or replace uielement; last time I tried doing anything with hs.uielement's code, I broke a relase :-)
    2. which methods/functions should be kept and which should be removed? There are a few different approaches I've taken at various times to solve basically the same problem -- how to get at a specific element when you don't know in advance the exact path to it... some of the work is done in lua only, others use C for speed, but are a little less flexible... and I'm not sure which is "better" but there is enough overlap that I'm also not sure that both should remain... So, what would you say are the key things that need to be kept? What have you used and your thoughts on documentation/examples/changes/updates?
  3. I noticed this myself recently in a spoon to control Roku devices that I'm planning to share soon, but haven't had the chance yet to try out my replacement module and see if it fixes the issue or not... the methods hs.menubar uses for creating and managing the status items (what we see as the menu icons in the menubar) have been deprecated for a long time... at a minimum, if we're going to close out additions to 1.0, we need to fix that... as to my replacement, it takes a whole different approach to menu building, and while I have written a wrapper, it's not 100% identical, and I don't recall off hand if it can be made to be so or not (I'll need to review it as well, it's been a while)...

  4. need to review them; no opinion atm, though I'll let you know if that changes.

  5. I know there has been talk of making this menu accessible... has anything been done on that front? If not, might it be better to make that a 2.0 feature and focus on solidifying 1.0 right now? We can always "backport" anything that seems useful that doesn't break anything existing once we've started concentrating primarily on the 2.x branch.

  6. Like 2, I'd like your take on the aspects you're using regularly and what might need to be tweaked... I'm not against adding this, but I'll also note that as long as Apple keeps the touchbar unique to specific Macbook Pro models (and not even all of those) and not part of their regular lineup of laptops or external keyboards, I'm not sure how much demand there really is or will be.

I'll post additional thoughts as I get a chance to review things and as I have them!

latenitefilms commented 5 years ago

@asmagill - Replies to your points below:

  1. Sounds good to me!

  2. I'm more than happy to help update the documentation if you need me to. In regards to hs.uielement, I've personally never really been able to make it do anything useful, but I don't see any disadvantage of leaving both hs.uielement and hs.axuielement in Hammerspoon v1.0, then we can re-evaluate in v2.0? In regards to which methods/functions to keep and which to remove, I feel like I've used everything at different points. Let me have a proper think later this week, and I'll put some thoughts into #1630.

  3. I think if the hs.menubar replacement is going to wait until v2.0, then yes, I think some of the hs.menubar bugs should be fixed in v1.0.

  4. I tried to do this myself when I originally posted the issue, but kept running into roadblocks. My Objective-C skills are probably a bit better now, so I might go back and have another look this weekend. You're right though - this might be better left as a v2.0 thing if it's too hard/complex. I always hoped it was an easy fix, as essentially I'm hoping we can just "attached" an existing hs.menubar to the dock icon.

  5. I think we're pretty much making use of everything hs._asm.undocumented.touchbar has to offer - and it's AWESOME. There's lots of things I'd love to see added eventually (mainly new hs._asm.undocumented.touchbar.item objects) - but the fundamentals all seem to be pretty rock solid. There's a few crash logs and errors in #1078 - however none of these are deal breakers. CommandPost now has 1.2K active monthly users, and Crashlytics reports very few crashes these days across the board, which is awesome!

cmsj commented 5 years ago

@asmagill I understand the desire to keep maintaining the 1.0 branch, but it’s not something I’d have time to do. If someone else wants to, I wouldn’t object, but the most obvious objection would be that Sparkle is going to tell everyone to upgrade to 2.0 as soon as we release it, and never offer them 1.x upgrades. The only way around that would be to have a completely separate appcast for 2.x and my concern with that would be splitting the community. We’d need to also have a parallel set of Spoons for 1.x and 2.x, etc. I think overall my strong preference would be to abandon 1.x and focus only on 2.x.

@latenitefilms If we did overhaul the API for a 2.x release, would you move CP to that, or would you stick with the 1.x API and diverge from our master branch?

Personally, I’m fine with overhauling our API, I would enjoy updating my config to work better, but I appreciate that that may not be true for all our users. I’m sympathetic to that, but I’m also wary of treating this project too much like a piece of Enterprise software that has paying customers. At least for me this is my fun coding side project and I think we’ve given people a very good run of API stability thus far.

I suspect the best option would be to have Sparkle force people to manually download 2.x and show them a web page detailing all the API breakage, put versioning into Spoons such that 2.x refuses to load any Spoon that doesn’t declare itself compatible, and state clearly that 1.x is abandoned. That gives people control of their upgrade timing, while making it clear they need to upgrade.

I suppose the biggest question is how much effort we should put into 1.0 before we shift focus. I’d vote for as little as possible, because otherwise it’ll be a year before we even get to start 2.0 work, but I’m prepared to be convinced otherwise. I think something like merging axuielement would be best kept for 2.0 - it would give us the space to remove uielement and unify the two.

cmsj commented 5 years ago

Perhaps it would be wise to wait a month and see what WWDC brings - we might find that all sorts of APIs we rely on are being removed in 10.15 and HS becomes unviable 🤣

latenitefilms commented 5 years ago

I think we'd definitely make sure CommandPost is always compatible with Hammerspoon, so yes, we'd update our code to stay in-sync with Hammerspoon.

My philosophy with CommandPost is always just to support the "latest and greatest" - so we actually removed a lot of backwards compatibility for older versions of Final Cut Pro, because it was just proving such a pain to keep all previous versions working. So for me, personally, I'm happy to only support High Sierra onwards, and not stress about breaking backwards compatibility, as users can always use older versions of Hammerspoon/CommandPost if they don't want to update.

To also echo what you've said in that for both Hammerspoon and CommandPost, the priority should always be for the unpaid developers to do this for FUN. We don't want either projects to turn into hard work.

At the same time, I would prefer to merge Touch Bar and axuielement into Hammerspoon v1.0, as I feel they're sooooooooooo useful - then continue on the v2.0 adventure.

And yes, I agree, it's probably best to wait until WWDC - as who knows what Apple's planning to do in terms of automation. Apple Events, the Accessiblity API, etc. might all die in favour of some kind of iOS-like Shortcuts app.

In the meantime, I'd love to get the remaining hs.chooser issues solved and merged! :)

cmsj commented 5 years ago

Perhaps there’s a middle ground here. Keep working on 1.x and introduce a new module that will form the basis of 2.x watchers, wire it up to all our existing modules and port all our Spoons to it. Then encourage users to do the same and only then branch for the 2.0 API overhaul. That way, all the time we spend getting 2.0 ready is time users have to get their config much closer to 2.x compatible. The changes then would hopefully be simpler (although no promises that we don’t make other big breaking changes).

latenitefilms commented 5 years ago

Sounds like a plan!

Or alternatively, we could just slowly update extension APIs gradually, and keep the 0.x versioning until everything's updated, then do v1.0?

Given all the little bugs that I introduced when doing a very simple lint-ing, I feel like major doing smaller incremental updates could be safer?

There might also be some extensions that we could depreciate and convert into spoons?

asmagill commented 5 years ago

Agree that WWDC is close enough that we should hear what's changing before committing.

I don't recall if it was before or after the mjolnir/hammerspoon split, but at one point the question was asked why we were sticking with version numbers less than 1.x.y and the answer at the time was something along the lines of "because it will never be 'done' and we don't want to commit to an API that can't change dramatically"... not those exact words, but that was the sentiment...

Given your description of what you're envisioning, I see releases where the major version number is even (0, 2, etc.) as being "in progress", the API can change at any time, etc. while the odd numbered releases (1, the eventual 3, etc.) as being static in features, but a stable API. I don't think we'd want an actual security hole or particularly onerous bug that doesn't have an easy work around, to remain in such a release, but otherwise, don't expect changes. I mentioned backporting above only because I know the menu from the application icon has been discussed before, but unless someone is all but ready to release it, I certainly can't spend time on it before moving to 2.0 -- like you, I'd like to see the API cleaned up in a lot of ways sooner, rather than later.

I guess it comes down to I get the feeling that we have a fair number of casual users who will be slow to move to a new version that breaks things until it actually offers significant new features... and I don't want to "punish" them with buggy code with security holes.... I'm fine saying "no new features will be added", but I'm reluctant to add "and you're screwed if a particularly bad security hole is discovered".

latenitefilms commented 5 years ago

One thing I love about both CommandPost & Hammerspoon is that both projects can move quickly. If someone comes out with something cool, we can push out an update almost straight away. I would hate to loose that flexibility whilst we try and do some major changes under the hood.

Whilst I think of it, one thing that I think we should probably do is drop all the MJ prefix's in the code.

My suggestion is that we basically slowly update each extension alphabetically over the coming months - basically clean them up, work out if they're still relevant, maybe add tests where their are none - sort of the same way I went through and made sure @stickler-ci passed all the Lua extensions.

Whilst this is happening, @cmsj (and @asmagill ?) can still be working on a bigger notifications/callbacks framework at the same time - kinda like #1646?

cmsj commented 5 years ago

@latenitefilms The only issue I have with working through the modules individually is that it means a constant dripping of breaking changes. I suspect it might be better to break all of our users' configs in one go, instead of making them deal with a bunch of breaking changes over the next year.

cmsj commented 5 years ago

@asmagill yeah I remember making that argument about staying in 0.9.x, but doing a 1.0 and immediately abandoning it to work on 2.0 is the only way I can think of to make it clear that we're doing big things (which is something we traditionally haven't done).

I'm fine with pushing out security fixes to older releases, although I think HS is essentially one giant security hole - we have so many ways to do terrible things to a Mac :D

cmsj commented 5 years ago

So, assuming that WWDC doesn't remove all of the things we rely on, are there any objections to this as the plan:

  1. Continue landing features/fixes in 0.9.x
  2. Design/Implement a new module that provides a pleasing API for watching for events from other modules, land it in 0.9.x
  3. When 2. is completed, release 1.0 and announce that the new watcher API is going to be the primary/only way to do events in 2.0
  4. Get to work converting all of the modules
  5. Integrate hs.axuielement (and possibly rebuild hs.application/hs.window on top of it?)
  6. Release 2.0
latenitefilms commented 5 years ago

Sounds good to me!

I still reckon it's worthwhile merging in hs.axuielement and the Touch Bar extension into Hammerspoon sooner rather than later, with the disclaimer in the documentation that it's an "experimental extension" (like you currently do with others), especially given they're so useful and they both work incredibly well, but I guess users can always manually add extensions so it really doesn't make that much of a difference. My only thought is that by bringing it into the Hammerspoon core, there's more likely hood of other people in the community using it, so there's a better chance of other people submitting pull requests to fix code, make improvements, etc.

Or going back to a much earlier discussion... I wonder if we should re-evaluate whether or not Spoons can contain Objective-C code - as moving the Touch Bar extension to a Spoon could be an interesting alternative?

Maybe one way moving forward is to try and "clean up" some of the existing extensions method/functions, but still offer backwards compatibility, with a hs.logger message alerting the user that depreciated methods will be removed in a future version of Hammerspoon - kinda like Apple does?

latenitefilms commented 5 years ago

It's also probably worthwhile doing a clean up of all the outstanding issues & pull requests, closing old ones and ones that will probably never be tackled, and also assigning milestones to those that are not closed.

cmsj commented 5 years ago

@latenitefilms Apple is terrible at deprecating and removing things. There are still things in Cocoa that have been deprecated since 10.0 ;) I think we should go for a clean break rather than tie ourselves up in deprecation knots for months/years.

I'm also still firmly opposed to ObjC in Spoons. It's far too easy a way for stability issues to creep in, and then we're dealing with the crash reports.

As for axuielement, I think saving it for 2.0 is probably the way to go, but if @asmagill decides it's ready to merge before 2.0, I wouldn't block it.

cmsj commented 5 years ago

@latenitefilms Apple has had things deprecated in AppKit since 10.0 and not removed them. I think we should go for a clean break and not tie ourselves up in knots for months/years with trying to be backwards compatible.

As for ObjC in Spoons, I remain firmly opposed to that - it's far too easy a way for weirdness to creep in, and for crashes to be our problem.

On the axuielement subject, I think leaving it for 2.0 is the right thing to do, but if @asmagill says it's ready to merge before we get to that point, I wouldn't block it.

cmsj commented 5 years ago

is github broken? I've tried to reply here several times and it's not showing my comments.

cmsj commented 5 years ago

Apple gets to spend lots of money on slowly deprecating things (very slowly sometimes, there are things in AppKit that have been deprecated since 10.0 ;) and I think we should try to avoid that. It's more maintenance, more testing and less reliability.

On the ObjC-in-Spoons issue, I'm still firmly opposed to the idea. We will get the crash reports for whatever random C is crashing all over the place, and it also means we introduce an ABI that we have to worry about changing later (e.g. when upgrading Lua).

My thought with keeping axuielement until 2.0 was that it gives us time to check over its API and possibly rebuild hs.application/hs.window on top of it. However, if @asmagill thinks it's ready to merge before 1.0 is done, I'm fine with that.

latenitefilms commented 5 years ago

I’ve enjoyed reading your three, ever-so-slightly differently worded replies!

Yes, I think GitHub is having issues.

Either way, I’m happy with all your explanations and proposals - let’s do it!

Now can we merge in the Chooser changes? :)

cmsj commented 5 years ago

I've started throwing some ideas about hs.watcher into a google doc. Please feel free to comment:

https://docs.google.com/document/d/18teVg1NxZkzsFco88bhRdVHNkgQ9_oB0zPq-qDkEcKM/edit?usp=sharing

randomeizer commented 5 years ago

I'm not exactly sure what the goals are with hs.watcher exactly (is there documentation out there somewhere?), but regarding cp.prop, its goal was to expose property values in a 'watchable' way, on an individual basis. Whereas from what what I can see so far, hs.watcher seems to be about using string keys to add watchers into a central tracker?

cp.prop is a little bit heavy-weight, but does have some fairly advanced features, such as chaining/mutating properties together, so that if a property that other properties depend on changes, they can notify their own watchers - but only if the dependent value actually changes as a result. For example:

local prop = require "cp.prop"

local state = prop.THIS("foo")
local barred = state:mutate(function(original) return original() == "bar" end)

state:watch(function(value) print("state: " .. value) end, true) -- adding `true` forces it to run immediately, rather than waiting for a change.
barred:watch(function(value) print("barred: " .. value) end) -- only print on changes

state("barged")
state("barred")
state("foo")

The out put will be:

state: foo
state: barged
state: barred
barred: true
state: foo
barred: false

Not sure this is exactly what you're planning to do with hs.watcher exactly.

The other thing we've started doing internally is using middleclass for our FCPX API. I had been avoiding it for some time to keep our code 'idiomatic', but the amount of inheritance in the AXUIElements that get exposed by Apple made it more pain than it was worth to avoid it further. But an improvement that I've added is a lazy method/field/prop extension. This make it easy to have lazy-loading properties/fields/etc, which will only get created once, on demand, and then return the same result from then on.

An example of it in use is the root cp.ui.Element class, which has some lazy.prop and lazy.method definitions.

Anyway, not sure if this is useful for what you're thinking of regarding configurations, since I'm not sure how you're doing it currently. But we often expose configurable or interesting values from our plugins via cp.prop, which makes them watchable, composable and always in sync.

cmsj commented 5 years ago

I don’t think shared, in-sync variables is quite what we need, it’s more about propagating events. Also see my previous comment for a link to the google doc exploring some ideas for the API.

randomeizer commented 5 years ago

Hmm, yeah, different use cases. cp.prop changes (for example) could trigger a hs.watcher event, but a lot of other things could also.

Would the goal be to replace the various hs.application/path/wifi.watcher implementations with a central API?

latenitefilms commented 5 years ago

Looking through the Google document again, it seems like hs.watcher could actually make things more confusing?

Currently you don't really need hs.watcher for hs.application, as each Spoon can create its own hs.application object. I've never looked into how hs.application works internally - if I create two hs.application objects for Safari in Lua-land, does it then create two seperate watchers in ObjC-land, or is it smart enough to combine things internally?

hs.usb.watcher also creates a new watcher object, so that should be fine within different Spoons?

Things like hs.midi.deviceCallback however are obviously an issue, as hs.midi.deviceCallback can only have one callback function - which I'm assuming is the whole point of this issue in the first place.

However, in this case, it might make more sense to move deviceCallback from a function to a method, so that each hs.midi object can have it's own deviceCallback?

Another example is hs.audiodevice.watcher, which currently only supports a single callback, but I would suggest it would be better to just rewrite it like hs.battery.watcher, which creates a new hs.battery.watcher object (so that each object can have it's own callback function).

Maybe I'm missing something?

asmagill commented 5 years ago

Something I'd like to see in 2.0 are a couple of changes to the LuaSkin log* methods:

  1. the methods should all accept varargs like NSString stringWithFormat:, so we could do things like [skin logWarn:@"%s -- oopsie!", USERDATA_TAG] rather than [skin logWarn:[NSString stringWithFormat:@"%s -- oopsie!", USERDATA_TAG]] like we have to currently.
  2. Tie the log messages into a separate hs.logger instance for each module -- we've talked about this before and realized that it would require some significant, but as of yet not fully fleshed out, changes since the instance would have to be created and tracked when the module was first loaded and the current mechanism doesn't suggest an obvious way to do this without changes to every module.
asmagill commented 5 years ago

Re my other modules listed above, I'm out of town right now and can't do much with them for the next couple of weeks, but I'd also like to add as either potential last minute 1.0, or early 2.0, modules:

hs._asm.bonjour -- (currently undocumented) allows browsing, discovery, and publishing things being served on the specific Mac Hammerspoon is running on, but I'm looking at how hard it would be to migrate this to using dns_sd.h instead which would also allow publishing proxy records for other things on your network, and making NAT PMP (aka Apple's version of UPNP) requests. It may make the most sense to document and add the existing version to 1.0 and the more advanced one to 2.0.

hs._asm.serial -- I've used it off and on and never had any problems like I feared when I first developed it and was reluctant to add it to core. I may see about limiting some of the attributes which it currently allows you to change (I've never found a need for them, and some of them very definitely could lead to a lockup if done incorrectly).

hs._asm.diskarbitration -- just looked and it looks like more than just documentation isn't implemented yet, so maybe this should wait for 2.0.

asmagill commented 5 years ago

As mentioned in #1063, revisit hs._asm.consolepipe

asmagill commented 5 years ago

Just to put it out there, I will not be able to reliably contribute much until after September due to family obligations. That said, I will try and do what I can when I can...

In that spirit, which of my modules should I try to prioritize for the 1.0 cutoff?

I'm assuming that @latenitefilms wants to see hs._asm.axuielement added first, and I think it can be, though in 2.0 I'd like to see about merging it with hs.uielement (or replacing it entirely -- that module has always seemed like voodoo to me whenever I try to extend or modify it).

I'd personally like to see about hs._asm.bonjour (not the dns_sd version, yet, that will be a 2.0 project) as I have a nice bonjour launcher I've been using for a while that would make a great spoon once this module is in core.

Any others I should concentrate on?

randomeizer commented 5 years ago

We essentially exclusively use hs._asm.axuielement, so can't really comment on uielement. But unification/replacement seems like a good idea in principle, if it's doable.

latenitefilms commented 5 years ago

Like David said, we use hs._asm.axuielement and hs._asm.axuielement.observer a HUGE amount in CommandPost. We've been using it for a few years now, and it's pretty rock solid - I'd be tempted to just remove the "_asm." references and merge away into Hammerspoon. The main benefit I see of adding hs._asm.axuielement into the Hammerspoon core sooner rather than later is that hopefully more people in the Hammerspoon community will start using it, and offer further ideas and suggestions on how it can be improved. We also still occasionally run into issues where we see an "out of application memory" message, which may or may not be related to hs._asm.axuielement - so hopefully if other people start using hs._asm.axuielement as well, we can see if they also run into the same issue (of course it could be something completely unrelated - we've never found the cause).

I've never really fully got my head around hs.uielement (see #1793) - and it seems other people are also having issues (see #1104).

In terms of other extensions you've been working on I'd love to see in the Hammerspoon core, to quote from above:

  1. hs.menubar.new(false) appearing in system menubar? #1567 This bug is still pretty annoying, because every time I restart Hammerspoon it leaves some "ghost" items in the menu bar. I know @asmagill has been working on a new hs.menubar replacement in #1530 - so maybe this is something we can try and do for v1.0?

I realise the work you've been doing with a new hs.menubar has been in the hs._asm.guitk branch - so this is probably a bit hard to add to Hammerspoon easily?

There's quite a few issues related to hs.menubar, so would be good to come up with a game plan for this longer term.

  1. hs.preferences - Property List Editor with NSKeyedArchiver support #1498 There's quite a few issues and pull requests all tackling Property Lists & Defaults, so it would be great to finally consolidate and solve this feature requests for v1.0 as well. We currently use a mixture of hs.plist, hs._asm.cfpreferences and our own cp.plist

Again, there's a few issues related to property lists, so would be good to come up with a game plan for this longer term too.

Thanks again for EVERYTHING @asmagill ! If there's ANYTHING I can do to help, please let me know.

asmagill commented 5 years ago

Been poking at hs.menubar and not having much luck fixing it so far, not sure why...

But, I have also tried the legacy module of hs._asm.guitk.menubar and it works fine (well, I found a bug in the code that is currently in my github repo, but once I fixed that, it worked)... Now that I know that the old behavior is still possible and not completely broken with the recent macOS updates, I'll take another crack at fixing hs.menubar... it just may take more of a rewrite than I'd originally hoped.

@cmsj, what versions of macOS are we supporting now? I may have to break 10.10 compatibility (IIRC menus worked properly then and I know at least a few portions of my new version had to test for 10.10 because of some new additions, which suggests to me that that is when things probably broke for us)

cmsj commented 5 years ago

As of 0.9.76, the minimum is 10.12 😁

latenitefilms commented 5 years ago

@asmagill - This may or may not be helpful, but I did run across a few issues with hs._asm.guitk.menubar when I tested it a while back:

https://github.com/asmagill/hammerspoon_asm/issues/14 https://github.com/asmagill/hammerspoon_asm/issues/13 https://github.com/asmagill/hammerspoon_asm/issues/15

asmagill commented 5 years ago

Ok, I think I've fixed the menubar issue with detached menus and the changes can be found at https://github.com/asmagill/hammerspoon_core/tree/master/menubar

Because I also cleaned up the warnings during compiling, enough code changed that I want to run some more tests before I do an actual pull against Hammerspoon with the updates. I probably won't get a chance until later tonight or tomorrow, but figured I'd put it out there if others wants to give it a test in the meantime.

Simple version is that NSStatusItem doesn't give a specific initializer outside of creating it in the menubar so we never investigated beyond that; in the past using the removeStatusItem and keeping the same NSStatusItem in our own wrapper was sufficient to remove it; apparently some of Apple's changes in macOS see that the status item was attached to the menubar at some point and even though it isn't now, and since it hasn't been collected by ARC (remember, we kept the same item rather than create a brand new one), it gets reattached because something somewhere thinks it "should be".

I ran some tests, and it appears that NSStatusItem does support NSObject's most basic initializer (e.g. [[NSStatusItem alloc] init]) and we can just replicate the properties -- thus we have a status item which has never been attached to the menubar so the macOS doesn't think that it needs to be reattached during whatever process was re-attaching our previously detached one.

asmagill commented 5 years ago

(oh, in case anyone's wondering, to apply my changes, you just need to overwrite the menubar extensions internal.m and init.lua and recompile Hammerspoon to test out my changes... you don't need the whole folder unless you want to compile and install just the module so that it loads instead of the Hammerspoon one (the .hammerspoon/ directory comes first in the path, so if ~/.hammerspoon/hs/menubar/... exists, it will get used instead of the bundled versions in the application itself)

latenitefilms commented 5 years ago

AMAZING - thanks so much @asmagill, you're awesome.

I've tested this code in our CommandPost fork, and all seems to work properly - I haven't noticed any issues, so I think this should be all good to merge into Hammerspoon.

latenitefilms commented 5 years ago

Now that I think about it @asmagill - another extension of yours that we use heaps is hs._asm.undocumented.touchbar - it also seems pretty rock solid, so I reckon this could be merged into Hammerspoon too.

asmagill commented 5 years ago

@latenitefilms do you mean the entire module (bar and item submodules as well)? Or just the ability to create/use the virtual touchbar?

I'm more comfortable with hs._asm.undocumented.touchbar then I am with its sub-modules, though I'll add it to the list of things I'm trying to take a closer look at as I can this summer.

latenitefilms commented 5 years ago

I actually meant the entire module. We’ve been using it for a long time now and there’s no major issues that we’ve come across - works great!

asmagill commented 5 years ago

Add this to the 2.0 roadmap: we need to rethink how we are creating and maintaining documentation:

roeybiran commented 4 years ago

I would like to add that I am too eager to see hs._asm.axuielement added to Hammerspoon core. It’s probably the module I use the most, it has performed flawlessly for me, and so far it’s the biggest step I’ve taken towards ditching AppleScript altogether. I also use the observer submodule to a small extent, and it has too worked without problems. So, if this module would see some further work to zero-out any instability issues (so all of those “work in progress” and “experimental” words could be safely omitted from the docs 😁), and to have more comprehensive documentation — it would be perfect. Would love to help with anything whatsoever. Huge thanks to anyone involved in the amazing Hammerspoon project!

asmagill commented 4 years ago

@cmsj, I would like to get my current project hs.text (see #2215 if you haven't already) done, take another look at #2197 now that my desktop is running Catalina (still on Mojave for my laptop since I use a few 32bit apps that haven't been updated... looking into alternatives since I don't think they will be at this point), and then take a crack at cleaning up hs._asm.axuielement... probably should reread this thread to see what else I should consider in the short term.

If you're serious about 2.0, we should probably start thinking about setting at least a soft target date for the cutover or it will never happen... just to throw it out and get the conversation started again, does the end of the year sound reasonable for locking in 1.0 and starting on 2.0 in January?

cmsj commented 4 years ago

@asmagill I've been struggling a bit to find time for HS for a while now, but I do tend to have a bit more time over the christmas break, so if we're going to do a little push for 1.0, I think that would be the time to do it.

latenitefilms commented 4 years ago

FWIW, once hs.text is complete my personal wish list, in order of priority would be:

In terms of other smaller issues I'd love to tick off ASAP, the main one is:

latenitefilms commented 4 years ago

Actually, we should probably also fix Stream Deck support in Catalina, as from all reports it's currently broken (I haven't tested first hand).

asmagill commented 4 years ago

Something to peruse for 2.0 concerning application energy impact: https://developer.apple.com/library/archive/documentation/Performance/Conceptual/power_efficiency_guidelines_osx/ especially the sections on Idle and Timers.

latenitefilms commented 4 years ago

Is Centralized Task Scheduling (CTS) or Grand Central Dispatch (GCD) used at all in Hammerspoon currently?