Hammerspoon / hammerspoon

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

Add hints module #146

Closed cmsj closed 9 years ago

cmsj commented 9 years ago

Currently staged at https://github.com/Hammerspoon/hammerspoon/tree/add-hints-extension, ported from https://github.com/trishume/mjolnir.th.hints

Nits I noticed:

I also wondered if it would make sense to print the window title below the icon (likely with shadowed text, for readability) for when folks have, e.g. multiple Safari windows.

asmagill commented 9 years ago

fwiw, I don't see the icons at all on my second screen, though the assignment occurs, because I can type the letter that I know is/should be assigned to it and it works... just don't know where they are being sent!

Also, the assignment ordering should be editable for those of us not used to Dvorak-like key distributions.

sebastiansturm commented 9 years ago

Thanks alot for this great module! It might make sense to replace window.allWindows() in line 52 by window.visibleWindows()? Or maybe display the hints corresponding to invisible windows in a special location?

cmsj commented 9 years ago

@sebastiansturm I tend to think that having invisible windows be selectable is probably a good idea, but agree that having them clustered in a special location would make sense.

cmsj commented 9 years ago

@asmagill I've made hintChars overridable on my branch.

cmsj commented 9 years ago

So an important question here is what we do if there are more windows than hintChars. Ultimately I'm not sure there is a great answer, even if we use the full alphabet and 0-9 that's not going to cover everyone. I guess we could throw in punctuation too. I guess that gets us up to around 65 possible characters.

cmsj commented 9 years ago

I've added an extra check that only shows hints for things that pass window:isStandard(), which seems to cut out weird things like GeekTool. I also added a test to cut out windows that are off the top of the screen (e.g. Unclutter.app). Finally, I've expanded the list of hint characters to have all of the a-z0-9 and punctuation keys that hs.hotkeys knows about. If there are more windows than available hotkeys, the loop will print an error and continue. It's ugly, but at least it doesn't explode, and there isn't really a smart way around the situation where there are more windows than keys. I also fixed a bug where available hint chars were being consumed by windows that didn't qualify for a hint key. Now we only allocate a key for windows that are allowed to have it, which makes things slightly more efficient!

So I think that just leaves some API docs and this module can land!

(one question I have is the original choice of hintChars seemed pretty random. Was there a reason for those choices? I've replaced them with the chars I listed, and it seems fine to me).

cmsj commented 9 years ago

@asmagill fancy testing my branch again and seeing if hints are now displayed properly on your second screen?

sebastiansturm commented 9 years ago

concerning num(windows) > num(hintchars), the use of multi-letter identifiers seems like a natural choice to me. This is also what Vimperator does and more than two characters in a row shouldn't be needed in practice. Ideally, this could be used to sort windows by their parent application. I.e., instead of assigning something like 'aa' to 'c8' to all of my windows, assign 'f-something' to the windows belonging to Firefox or other applications that start with an 'F', 'p-something' to Preview windows, etc. Typing an incomplete identifier should update the display such that only identifiers with a matching prefix remain visible.

cmsj commented 9 years ago

@sebastiansturm yes, double letters would seem the obvious choice, except that it's going to be quite tricky to implement - the hotkey system in Hammerspoon is not able to deal with such a hotkey, AFAIK. It could be emulated with expanded use of hotkey.modal, which is how the hints module works in the first place, but I'm not sure it's going to make this initial cut, unfortunately (unless someone else has time to work on it :D )

sebastiansturm commented 9 years ago

hm, I see. I have little experience with lua or the hammerspoon api, but I guess I could try over the weekend.

sebastiansturm commented 9 years ago

I have something that more or less seems to work: https://gist.github.com/sebastiansturm/63f07742d9e5de2c6a2f However, it comes with a free memory leak (or at least Hammerspoon uses an excessive amount of RAM on the order of 100MB, haven't checked yet if that stabilizes at this high value). Do you know what causes this?

sebastiansturm commented 9 years ago

I'm only speculating here (haven't done any CoreFoundation memory management so far), but maybe something like CFRelease((__bridge CFTypeRef)hint) is needed after the [hint close]-line within hint_gc?

cmsj commented 9 years ago

I think you're on the right track there, the HintWindow is created with __bridge_retained, which is telling ARC "give me a C pointer to this Objective C object and add +1 to its refcount". There should be a corresponding __bridge_transfer if we want ARC to clear it up, which is typically what I would do (vs explicitly freeing it ourselves). So right now, the HintWindow (a subclass of NSWindow) gets closed, but never freed.

It looks to me like hint_gc and hint_close are doing the same thing, which makes very little sense, I would suggest that we unify those somewhat, and do a __bridge_transfer cast on the result of get_hint_arg. (I'm not saying you should do those things, more writing a reminder for myself :)

I also want to check over the __gc stuff more generally. There is one for each HintWindow, but not a top-level one, which would probably be necessary to clear up all the hints (e.g. an hs.reload() while hints are displayed, might leave them on screen, I haven't tested).

sebastiansturm commented 9 years ago

thank you! Right now I'm using the stupid CFRelease kludge on my machine, so I'm very much looking forward to a proper solution.

cmsj commented 9 years ago

I've just pushed up a reworked version that, AFAICT, doesn't leak. Rather than using a __bridge_transfer, I just set the windows to release themselves when they are closed, and forcibly assign them to 'nil' after closing. This seems to allow ARC to properly release the resources, and I'm not seeing any significant leakage.

cmsj commented 9 years ago

@sebastiansturm if you could test that and let me know if you see the leaking or not, that would be awesome. I'll work up the API documentation next, and hopefully we can land this shortly, and cut a release with it included! Thanks for your help :)

sebastiansturm commented 9 years ago

Hi, it seems that memory usage now stays constant, though at a higher value than before (~55MB instead of 27MB). Is that expected? I also had a crash during my first test run, but so far couldn't reproduce it. BTW, did you have a look at my implementation of multi-character hints? Is it suitable for inclusion in the official hints module? I like the Vimperator-like behavior very much, but would rather not maintain my personal copy of init.lua. thanks, Sebastian

cmsj commented 9 years ago

I'm not sure the RAM usage is necessarily supposed to be that high, but so long as it's not leaking, that's good news. We can revisit to optimise it later, if there are savings to be had.

I have checked over your implementation. I do like it and would like to see it in the official hints extension, but I don't think it should be the default implementation. How about we work towards getting it in as an optional behaviour?

It's worth noting that I've landed hs.hints on master now, but I slightly changed the behaviour - I didn't like that setupModal() was binding a hotkey all of its own, so I've modified hs.hotkey.modal.new() to accept (nil, nil) parameters, thus creating a modal state that can only be entered programatically. I prefer this route because it means users just need to bind a hotkey to hs.hints.windowHints() and they don't need to care about the implementation.

How about we have a parameter to windowHints() that specifies which style of hints the user would like, so in your case, you'd bind something to hs.hints.windowHints("vimperator")?

cmsj commented 9 years ago

(note, I'm closing this only because the extension has landed, we can continue the discussion about how to get an optional vimperator style in, either here or in a new issue, your choice :)

sebastiansturm commented 9 years ago

I'd like that (the "vimperator" option). I'll try to create a suitable pull request over christmas and then get back to you, thanks!

sebastiansturm commented 9 years ago

Hi, I have created a pull request that allows the user to switch between vimperator-style hinting and the default single-character hints. The default method now switches to multi-character hints if the hintChars are exhausted, I hope that is acceptable?

maxrothman commented 9 years ago

Is there any interest in generalizing this extension to be a multi-purpose hint draw-er? I'd be interested in using hints do to other things (show locations a window can be moved to, dismiss specific notifications, etc.). It seems like a good general-purpose context-giving feature.

cmsj commented 9 years ago

@maxrothman it's an interesting idea. Perhaps some more work on hs.drawing would be a better angle to come at this from?

maxrothman commented 9 years ago

@cmsj That sound like a good approach. More functionality would need to get added to hs.drawing though (e.g. rounded rectangles, fill gradients, etc.) so it wasn't ugly as sin.

cmsj commented 9 years ago

@maxrothman certainly true :)

cmsj commented 9 years ago

@maxrothman well both of those options turned out to be very easy to add. Obviously the main missing thing now is support for rendering images. I'll try and get to that tomorrow. Anything else you can think of that we might be missing here, drawing-wise?

maxrothman commented 9 years ago

Looking at the Cocoa drawing docs, their drawing primitives include lines, rectangles, ovals/circles, arcs, bezier cubic curves, images, and text. They support RGB/CMYK/grey colors with transparencies, gradient and pattern fills, as well as shadows.

I think all of those would be good to support except for multiple color spaces. I think RGB is more than sufficient on that front.

cmsj commented 9 years ago

@maxrothman agreed that RGB is sufficient, I have no intention of going anywhere near supporting multiple colourspaces! Lines, rectangles, ovals/circles and text we have already. Images is definitely needed. I think for arcs/curves I'd want to see a solid use case before I implemented them (I'd also be fine with someone sending in a solid patch to do them, then I care much less about the use case ;) We now have linear gradients. I suspect we might want to extend that to also support angular gradients. So that just leaves pattern fills and shadows. I haven't looked at the API for pattern fills yet, but shadows look like they should be do-able, although it does raise the interesting issue that right now we create the NSView/NSWindow at the same size as the requested drawing object, but if we're going to start rendering a shadow, we'll need to embiggen the view/window by arbitrary amounts, so that will take some thought.

Thanks!

maxrothman commented 9 years ago

Pattern fills are just another type of color, so it seems like images would need to be done first. I agree that arcs/curves can wait for now, but I'd vote for rounded rectangle support (maybe as part of the rectangle abstraction?)

The other previously unmentioned feature that should receive some thought is line dashing, but that might be non-urgent enough to wait for a while.

cmsj commented 9 years ago

@maxrothman rounded rects are done already, see 26ab4a6 from last night :)

maxrothman commented 9 years ago

Woohoo!

cmsj commented 9 years ago

@maxrothman simple images are now implemented :)