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

Fix KeyError crash on OSX if using different terminal #232

Merged
merged 1 commit into from
Nov 18, 2015

Conversation

davetcoleman
Copy link
Contributor

Silently fail to create notification instead of crashing. Using get will return None if a key is not present rather than raise a KeyError

Background: I'm working on OSX via a SSH shell in Linux (can't stand those key bindings :) ) and I don't have the TERM_PROGRAM env variable. All I have is TERM, which is valued as TERM=xterm

Silently fail to create notification instead of crashing. Using ``get`` will return `None` if a key is not present rather than raise a `KeyError`

Background: I'm working on OSX via a SSH shell in Linux (can't stand those key bindings :) ) and I don't have the TERM_PROGRAM env variable. All I have is TERM, which is valued as ``TERM=xterm``
@NikolausDemmel
Copy link
Member

Hang on, so your Host is Linux and you ssh into OSX? (I initially understood it the other way round, mazbe because I'm biased towards liking the OSX keybindings :-P)

Your change will still (try to) invoke the notification, just without the "activation" of the terminal app. I take it that results in neither a notification, nor an error in your scenario?

@davetcoleman
Copy link
Contributor Author

lol, yes, I prefer linux.

I still get a notification on the host OSX, but of course not on the SSH terminal I'm working from in Linux

This PR fixes a bug, and doesn't add any new features/functionality

@NikolausDemmel
Copy link
Member

Ah ok, I just wanted to check if the notification itself causes any issues in the SSH scenario.

So +1 from me.

Maybe a source code comment would be nice to point out that "TERM_PROGRAM does not exists" is really a valid use case here. Maybe something like:

# try to use the environment to detect the use of a known terminal program and bring it
# to the foreground in addition to the desktop notification

@wjwwood
Copy link
Member

wjwwood commented Nov 18, 2015

lol, yes, I prefer linux.

Are you talking about copy, paste, etc hotkeys? How weird 😄, I always found OS X's hot keys to make more sense.

The fix looks ok to me, I'll probably add an explicit default argument in a follow up commit (I don't care for the readability of the implicit None default that .get() has).

wjwwood added a commit that referenced this pull request Nov 18, 2015
Fix KeyError crash on OSX if using different terminal
@wjwwood wjwwood merged commit 4671721 into catkin:master Nov 18, 2015
@davetcoleman davetcoleman deleted the patch-3 branch November 19, 2015 07:43
@davetcoleman
Copy link
Contributor Author

I've trained myself for years to be really good at emac's Alt/Ctrl shortcut keys and all that goes away on a Mac keyboard. I've never fully figured out alt key bindings on a mac

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants