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

prevent a crash with Command Window in Text Mode #1545

Merged
merged 4 commits into from Aug 1, 2013
Merged

prevent a crash with Command Window in Text Mode #1545

merged 4 commits into from Aug 1, 2013

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Jul 31, 2013

I can't fathom a reason for getting the characters from the event, so I just removed that code. Say your trigger for this is ⌃T. Does that mean searching for "xyz", then at some later point hitting the trigger, you should expect to see "xyzt"? Doesn't make sense. I must be missing something, but I can't find anything that doesn't work with these changes.

I also simplified the way length was gotten for the calls to NSMakeRange().

Don't attempt to append characters from the event.

fixes #1544
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 31, 2013

I may be wrong, but is it perhaps so that if you have 'switch to text mode if no match is found' the last character you press is also entered?

e.g. try enabling that and typing:

abcdefghij

hopefully whichever char triggers switching to text mode is also inputted.

On 31 Gorff 2013, at 21:15, Rob McBroom notifications@github.com wrote:

I can't fathom a reason for getting the characters from the event, so I just removed that code. Say your trigger for this is ⌃T. Does that mean searching for "xyz", then at some later point hitting the trigger, you should expect to see "xyzt"? Doesn't make sense. I must be missing something, but I can't find anything that doesn't work with these changes.

I also simplified the way length was gotten for the calls to NSMakeRange().

You can merge this Pull Request by running

git pull https://github.com/skurfer/Quicksilver i1544
Or view, comment on, or merge it at:

#1545

Commit Summary

use only the search string when going into text mode
File Changes

M Quicksilver/Code-QuickStepInterface/QSSearchObjectView.m (8)
Patch Links:

https://github.com/quicksilver/Quicksilver/pull/1545.patch
https://github.com/quicksilver/Quicksilver/pull/1545.diff

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 31, 2013

I may be wrong, but is it perhaps so that if you have 'switch to text mode if no match is found' the last character you press is also entered?

A good theory. I looked at all the places that call transmogrifyWithText: and that's the only other one. But the "no match found" scenario is the only time when string != nil coming into the method. So the code I changed isn't called in that case.

Also, I tried it (I use that pref) and all the characters are there.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 31, 2013

Oh, hang on. Found what I broke. If you type "www.blah", the "." that switches to text mode is lost.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 31, 2013

OK, fixed. I tried your suggested way, and it worked, but I didn't like checking specific event types as they could change. So instead, I just try to get the characters and ignore them if there's an exception.

@tiennou
Copy link
Member

@tiennou tiennou commented Jul 31, 2013

Hem..., specifically :

Rationale
The standard Cocoa convention is that exceptions signal programmer error
and are not intended to be recovered from. Making code exceptions-safe by
default would impose severe runtime and code size penalties on code that
typically does not actually care about exceptions safety. Therefore,
ARC-generated code leaks by default on exceptions, which is just fine if the
process is going to be immediately terminated anyway. Programs which do
care about recovering from exceptions should enable the option.

Please don't use exception for flow-control, unless you like breakpoints to be triggered and memory to be leaked ;-).

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 31, 2013

You act like it's not Python or something. 😉 Thanks for pointing that out.

I didn't see a clean way to test it prior to asking for characters, other than hard-coding a test for a bunch of event types. Any other ideas?

@tiennou
Copy link
Member

@tiennou tiennou commented Jul 31, 2013

Apart from creating a category -[NSEvent isKeyboardEvent], I don't see another way to make that cleaner.

Also I thing you can hardcode them : NSEventType, being used as a bitfield in NSEventMask, makes them pretty scare (they're limited by the width of NSUInteger), so Apple ponders greatly before adding any of them ;-).

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 31, 2013

OK, how's it look now?

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 1, 2013

Hehehe, Etienne beat me to it. I was gonna say: it's fine for Python, since it's an interpreted language and doing try: except is normally quicker than if: else:
But… yeah ;-)

I didn't like the fact we had to hard-code the types as well, but couldn't think of a better solution. My idea to add an isKeyEvent method to NSEvent is because I thought one would have existed already.
P.S. see https://developer.apple.com/library/mac/#documentation/cocoa/Conceptual/EventOverview/HandlingKeyEvents/HandlingKeyEvents.html#//apple_ref/doc/uid/10000060i-CH7-SW1
NSFlagsChanged is also classed as a key event

On 31 Gorff 2013, at 23:25, Rob McBroom notifications@github.com wrote:

You act like it's not Python or something. Thanks for pointing that out.

I didn't see a clean way to test it prior to asking for characters, other than hard-coding a test for a bunch of event types. Any other ideas?


Reply to this email directly or view it on GitHub.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 1, 2013

Woops, I missed the rest of the topic for some reason.

  • Maybe add NSFlagsChanged but not really necessary
  • Would be nice to make a category :)

@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 1, 2013

NSFlagsChanged is also classed as a key event

Yeah. I read that and had it in there, but decided it was pointless. Maybe I'm wrong? Could we see "modifier only text-mode" some day?

@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 1, 2013

Don't want to hold up the release over this, so I added the flags changed event type.

pjrobertson added a commit that referenced this issue Aug 1, 2013
prevent a crash with Command Window in Text Mode
@pjrobertson pjrobertson merged commit 4ebc7ba into quicksilver:release Aug 1, 2013
skurfer added a commit that referenced this issue Aug 1, 2013
@skurfer skurfer deleted the i1544 branch Aug 1, 2013
@purp
Copy link

@purp purp commented Sep 18, 2013

Two things:

  1. Just in case you don't think we read the changenotes, we do.
  2. This was such an excellent example of open source developers collaborating for the right result that I just couldn't stand not to donate.

Thanks, all, for spending the time to make this excellent tool better.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Sep 18, 2013

Thanks for the kind words, @purp.

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.

None yet

4 participants