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

Add API for focusing OSX tabs #1513

Merged
merged 3 commits into from Aug 31, 2017
Merged

Add API for focusing OSX tabs #1513

merged 3 commits into from Aug 31, 2017

Conversation

@trishume
Copy link
Contributor

trishume commented Aug 17, 2017

macOS Sierra added the ability to use tabs with any document-based app.
Unfortunately for some apps like Sublime Text, there's no way by default
to use keyboard shortcuts to switch between these tabs.

This adds an API for doing so.

macOS Sierra added the ability to use tabs with any document-based app.
Unfortunately for some apps like Sublime Text, there's no way by default
to use keyboard shortcuts to switch between these tabs.

This adds an API for doing so.
@codecov
Copy link

codecov bot commented Aug 17, 2017

Codecov Report

Merging #1513 into master will decrease coverage by 0.05%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #1513      +/-   ##
=========================================
- Coverage   23.66%   23.6%   -0.06%     
=========================================
  Files         129     129              
  Lines       23979   24030      +51     
  Branches     2842    2856      +14     
=========================================
- Hits         5674    5672       -2     
- Misses      17939   17992      +53     
  Partials      366     366
/// * The `hs.window` object
static int window_focustab(lua_State* L) {
AXUIElementRef win = get_window_arg(L, 1);
double indexDouble = luaL_checknumber(L, 2);

This comment has been minimized.

Copy link
@asmagill

asmagill Aug 18, 2017

Member

Is there a reason you're using a double here? Since you immediately put it into a CFIndex and don't use indexDouble again, it's probably cleaner to use luaL_checkinteger and assign it directly to tabIndex.

/// index is out of bounds. Returns true if a tab was pressed.
///
/// Parameters:
/// * index - A number, true if the window should be set fullscreen, false if not

This comment has been minimized.

Copy link
@asmagill

asmagill Aug 18, 2017

Member

Unclear meaning here -- in lua any number, including 0 is considered "true"

This comment has been minimized.

Copy link
@trishume

trishume Aug 18, 2017

Author Contributor

This was a copy-paste mistake.

/// * index - A number, true if the window should be set fullscreen, false if not
///
/// Returns:
/// * The `hs.window` object

This comment has been minimized.

Copy link
@asmagill

asmagill Aug 18, 2017

Member

wrong return description; per tag line description should probably read "true if the tab was successfully pressed, or false if there was a problem" or similar

This comment has been minimized.

Copy link
@trishume

trishume Aug 18, 2017

Author Contributor

Copy paste mistake, fixed.

@asmagill
Copy link
Member

asmagill commented Aug 18, 2017

See above line comments. Also, how hard would it be to add a method which returns an integer specifying the number of tabs in a given window?

@trishume
Copy link
Contributor Author

trishume commented Aug 18, 2017

I addressed all comments and added a method for counting tabs, and tested it.

Interestingly, I tested it and this method of finding and switching tabs also finds Chrome and Safari tabs.

@asmagill
Copy link
Member

asmagill commented Aug 21, 2017

lgtm; @cmsj and @trishume a point to consider before merging:

The tabs are indexed starting at 0; Lua and most of the extensions we've added use 1 as the starting point -- in fact in a lot of the methods we've added we specifically add/subtract 1 when interfacing with the macOS APIs to keep Hammerspoon's indexing consistent with the Lua conventions since this is what most users know/see/interact with. If these methods were adding string or table like functionality or acted in a string or table like manner, I would recommend doing the same here; since it's not, I don't have strong feelings one way or the other, but I will say it took me by surprise when I tested this on my own machine and I had to re-read the documentation to make sure this was known/expected behavior.

Thoughts? I'm fine either way, but I did want to pose the question before merging.

@trishume
Copy link
Contributor Author

trishume commented Aug 21, 2017

Cool, I wondered about that, knowing that there are other APIs that use 1-based indices makes it easy. I just changed it to use 1-based indexing. And that lets me remove the - 1 in my config where I bind it to alt+1-9.

@asmagill
Copy link
Member

asmagill commented Aug 23, 2017

@trishume do you consider this complete or do you have additional plans?
@cmsj, if complete, objections to merging?

@trishume
Copy link
Contributor Author

trishume commented Aug 24, 2017

I consider this complete. I don't plan on adding anything else, at least for now.

@asmagill
Copy link
Member

asmagill commented Aug 31, 2017

@trishume -- I hadn't thought about this until just now, so consider it a future feature request if you don't have time right now, but any idea on how hard it might be to add a method to determine what tab is currently focused in a window and another to get the names(title) of all of the tabs in a window?

If you tell me you don't have the time right now or that you don't know how long it might take to add, I'll merge this now; on the other hand if you agree with the usefulness of these methods and think you can get these added shortly, I'll hold off.

@trishume
Copy link
Contributor Author

trishume commented Aug 31, 2017

I'm unlikely to work on those methods myself. I don't think they'd be too hard, but they're not trivial like the tab count one. They also wouldn't be useful to me personally. Although I can see the current tab index being useful to others for a "next/previous tab" binding, and the names being useful in combo with hs.chooser.

@asmagill
Copy link
Member

asmagill commented Aug 31, 2017

Understood, I may tackle them myself at some point then as I've worked with the accessibility API myself before, but figured I'd bounce it off of you since you've done so more recently.

Merging as is then.

@asmagill asmagill merged commit 3ddac80 into Hammerspoon:master Aug 31, 2017
2 of 4 checks passed
2 of 4 checks passed
codecov/patch 0% of diff hit (target 23.66%)
Details
codecov/project 23.6% (-0.06%) compared to 15db725
Details
VersionEye All software dependencies are fine. You are awesome!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.