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

Rudimentary Touch Bar support #528

Merged
merged 5 commits into from Nov 15, 2017
Merged

Rudimentary Touch Bar support #528

merged 5 commits into from Nov 15, 2017

Conversation

@greg
Copy link
Contributor

@greg greg commented Sep 19, 2017

Adds a basic Safari-style control for switching between open tabs, showing the filename of the most recently selected window within that tab as the title (which the nvim provided tab bar behaviour).

This addition required some modifications to NeoVimServer and NeoVimView+Api which do not appear to have broken anything, but I am not familiar enough with the codebase to be completely sure.

I hope this is useful for some people; if it is, I'll work on adding more Touch Bar features.

Testing note: if you don't have a Touch Bar, Xcode has a Touch Bar simulator (Window > Show Touch Bar, or ⌘⇧5).

greg added 4 commits Sep 19, 2017
- in addition to being called when a file is read or written, as before
…e tracking

Also adds -currentWindow to NeoVimTab for convenience
@greg greg force-pushed the greg:develop branch from c0cef07 to d13e4b7 Sep 19, 2017
@qvacua
Copy link
Owner

@qvacua qvacua commented Sep 23, 2017

Thank you for the PR. I'll review it and come back to you. 😀

@qvacua
Copy link
Owner

@qvacua qvacua commented Sep 28, 2017

I finally came to try it (haven't had time to look at the code though...). It seems that sometimes the titles of tabs are not displayed. I also tried it on an MBP with TB and it also happens there. Could you look at it?

2017-09-28 10_22_13

@greg
Copy link
Contributor Author

@greg greg commented Oct 7, 2017

I couldn't reproduce this on macOS 10.13, but it looks like I was setting the segmented control titles incorrectly — I've pushed a fix that will hopefully correct this on earlier versions.

(If you are also on 10.13, then I'm a little lost as to why this issue would occur 😝)

@qvacua
Copy link
Owner

@qvacua qvacua commented Oct 20, 2017

Thanks. Yes, I'm on 10.12... I'll re-check and give you feedback. :)

@qvacua qvacua added the in progress label Oct 23, 2017
@qvacua qvacua mentioned this pull request Oct 23, 2017
func updateTouchBarTab() {
guard let tabsControl = getTabsControl() else { return }
tabsCache = self.agent.tabs()
if tabsControl.numberOfItems != tabsCache.count {

This comment has been minimized.

@qvacua

qvacua Oct 24, 2017
Owner

This seems to be the problematic check (on 10.12.x at least): If I remove it

guard ...
tabsCache = ...
tabsControl.reloadData()
tabsControl.selectedIndex = selectedTabIndex()
return // do we need `reloadItems(at:)` even we reload completely?

the touch bar is updated correctly.

I think that the count check is too coarse and it's not expensive just to reload here. (We could iterate through all the tabs and windows and update the touch bar items individually, but that's probably an overkill.) The transition of the touch bar when the number of tabs changes is not smooth, but I think that that's a minor detail we can improve later. 😀


func updateTouchBarCurrentBuffer() {
guard let tabsControl = getTabsControl() else { return }
tabsCache = self.agent.tabs()

This comment has been minimized.

@qvacua

qvacua Oct 24, 2017
Owner

I think that it'd be better to update tabsCache at appropriate places in NeoVimView+UiBridge since it's not necessarily touch bar specific (although it's currently used only by the touch bar logic)

@qvacua qvacua merged commit c136e55 into qvacua:develop Nov 15, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@qvacua qvacua removed the in progress label Nov 15, 2017
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