Fix terminalapp plugin #2416

[madyankin]

Fix #2416 in terminalapp plugin

[ncanceill]

Hi, and thanks for bringing a fix for this.

One small thing: in its current form, this PR will not auto-close #2416. You need to specify the exact phrase "fix #2416" — you can replace "fix" by any of the words mentioned there, but there needs to be nothing in between.

As a result, your commit message

Fix terminalapp plugin #2416

will not do. However, no need to commit --amend and push --force: you just need to write it in the PR description — where it currently reads "No description provided.".

---

@guidobouman please test this and confirm that it fixes your issue.

[madyankin]

Done, thank you.

On 06 Jul 2014, at 01:36, Nicolas Canceill notifications@github.com wrote:

Hi, and thanks for bringing a fix for this.

One small thing: in its current form, this PR will not auto-close #2416. You need to specify the exact phrase "fix #2416" — you can replace "fix" by any of the words mentioned there, but there needs to be nothing in between.

As a result, your commit message

Fix terminalapp plugin #2416
will not do. However, no need to commit --amend and push --force: you just need to write it in the PR description — where it currently reads "No description provided.".

@guidobouman please test this and confirm that it fixes your issue.

—
Reply to this email directly or view it on GitHub.

[guidobouman]

Awesome work guys, will test this in a couple of hours. (If it's not merged in by then.)

[mcornella]

Hi guys, since this hasn't been merged yet, a couple of changes:

• Use $OSTYPE instead of $(uname).
  Also, I'm not crazy about "$(uname -r | cut -d . -f 1)" -lt 13 to check for OSX < 10.9; can you provide examples as to what the output of this command gives in various versions?
• Not use add-zsh-hook and use precmd_functions+= syntax instead, see add-zsh-hook: function definition file not found #748.

[mcornella]

Just realised we pushed a duplicate version of Terminal.app plugin in lib/termsupport.zsh, though I'm not sure about it causing #2416. Gonna have to ask @apjanke about this.

[apjanke]

I, uhhh, was not aware of the existence of the terminalapp plugin. (And neither was the poster of #2646, which prompted its addition.)

I read through the plugin's source. Yeah, the stuff I added to lib/termsupport.zsh in #3429 is just a less-fleshed-out dup of the existing functionality in the terminalapp plugin.

FWIW, I don't think it's causing #2416. (Either the plugin or #3429.) For one, looking through the StackOverflow post, it looks like it's related to color/bold control stuff and not the pwd reporting stuff. Also, I'm on Mavericks, and I'm not seeing the bad behavior described in #2416, with or without the terminalapp plugin enabled.

Anyway, it sounds like we should just remove the #3429 Terminal.app stuff from termsupport.zsh and tell #2646 (and me) to turn on the terminalapp plugin instead.

As an aside, I'm curious about what #2416 means by "the whole functionality has been replaced by Mavericks natively". I'm on Mavericks, and when I don't have the oh-my-zsh pwd-updating behavior enabled, Terminal.app running zsh is not aware of pwd changes. AFAIK, this is implemented in OS X via /etc/bashrc which doesn't apply to zsh invoked through normal means. (Didn't work for me on Yosemite back when I was running it, either.)

[mcornella]

Thank you Andrew, I also thought the linked SO thread in #2416 wasn't related to this functionality. I went a little further and a quick google search returned this other SO thread which complains about being unable to open a new tab on the same folder on Mavericks. Other related links also mention the same problem and the same solution, see superuser.com, russellfinn.com, and PR #522.

FWIW, I think the way to go is keeping the functionality in lib/termsupport.zsh: we want to maintain every type of terminal emulator as a first class citizen and out of the box, without the need to enable a plugin. I'll be following closely the current implemented solution and how it was arrived to, since it seems more complex than the one we pushed.

If we could also hear how exactly this breaks Ubuntu or other non-OSX platforms we can maybe provide a better fix. Work-in-progress will be, it seems.

[apjanke]

The implementation in plugins/terminalapp URL-escapes all special characters. The simpler one in lib/termsupport.zsh escapes only spaces. The more complex one in the plugin is better; dirs with special characters in them can break the simpler one we pushed in termsupport. Here's an example.

$ mkdir '$#@%^&*%$'
$ cd \$\#@%\^\&\*%\$ 

Also it looks like the termsupport.zsh version is using the incorrect variable $HOSTNAME instead of $HOST. (I think $HOSTNAME is a bash-ism.)

I also like the idea of pulling it in to lib/termsupport.zsh so it's supported out of the box.

[apjanke]

@mcornella - I made PR #3582 that will go ahead and fold terminalapp in to termsupport. Went ahead and did it because we need to switch over to their implementation to fix the escaping and host issues anyway.

Anybody else we should ask about merging the plugin? Do plugins have notional owners/maintainers?

[mcornella]

Do plugins have notional owners/maintainers?

Plugin mantainers in theory are all documented in the wiki, below each plugin (ping @Outpunk).
Another method we used with @ncanceill was looking at the history of the plugin files using git log -- path/to/plugin/directory, or github itself.

@apjanke regarding this issue, it should be auto-closed too with one of the PRs you made.

[apjanke]

Thanks. I'll read through the rest of the wiki, and I'm getting up to speed on git log/git blame.