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

Alter the single modifier code to use device independent key masks as opposed to key codes #1723

Merged
merged 3 commits into from Dec 19, 2013

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 18, 2013

Fixes #1712

Basically, using key codes meant the right keys didn't work. Instead I've used the device independent masks - exactly what they're for.

Aside: I don't like how the masks are hard-coded as the tags for the popup menu in the prefs, but that's how it's always been so there's no regression here.

@@ -207,6 +179,6 @@ -(void)resetTimesKeysPressed:(id)sender {

+ (void)resetModifierState
{
lastModifiers = 0;
// lastModifiers = 0;
Copy link
Member

@skurfer skurfer Dec 18, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this disabled for a reason or just for testing? Because it re-introduces the ⌘⇥ bug. :-)

Copy link
Member Author

@pjrobertson pjrobertson Dec 18, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just for testing. I’ll force push that out of there.

Yes, everyone will have to re-select it (well, all dev preview testers)

On 18 Rhag 2013, at 11:20, Rob McBroom notifications@github.com wrote:

In Quicksilver/Code-App/QSModifierKeyEvents.m:

@@ -207,6 +179,6 @@ -(void)resetTimesKeysPressed:(id)sender {

  • (void)resetModifierState
    {
    • lastModifiers = 0;
      +// lastModifiers = 0;
      Was this disabled for a reason or just for testing? Because it re-introduces the ⌘⇥ bug. :-)


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member

@skurfer skurfer commented Dec 18, 2013

So everyone will have to go re-select the correct modifier one time in the preferences after this?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Dec 18, 2013

Not sure if my last message got though :/

Here's what I said:

Sorry, just for testing. I’ll force push that out of there.

Yes, everyone will have to re-select it (well, all dev preview testers)

I've made the change now

@skurfer
Copy link
Member

@skurfer skurfer commented Dec 18, 2013

Yeah, it came through. I’ll look this over.

@skurfer
Copy link
Member

@skurfer skurfer commented Dec 18, 2013

Cool. Everything seems to work as advertised now, but…

Yes, everyone will have to re-select it (well, all dev preview testers)

Are you sure it doesn’t affect everyone?

Using ⌃ as an example, for both 1.1.3 and 1.2.0 dev preview, the value of QSModifierActivationKey is 18. When using this branch, I have to go in and re-select a modifier, which changes the value of QSModifierActivationKey to 524288.

This isn’t necessarily a deal-breaker, but we’ll have to communicate to users that a one-time step of reselecting the modifier in the prefs is necessary (or change the code to work around it).

@skurfer
Copy link
Member

@skurfer skurfer commented Dec 18, 2013

We should probably also add QSModifierActivationCount to QSDefaults.plist with a value of 0. Leaving stuff undefined has bit us in the past.

mods = NSAlphaShiftKeyMask;
}

QSModifierKeyEvent *match = [modifierKeyEvents objectForKey:mods ? [NSNumber numberWithUnsignedInteger:mods] : [NSNumber numberWithUnsignedInteger:lastModifiers]];
Copy link
Member

@tiennou tiennou Dec 18, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap in ().
EDIT the ternary I meant. Feel free to use @() also ;-).

Copy link
Member Author

@pjrobertson pjrobertson Dec 18, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's @()... Google!

@tiennou
Copy link
Member

@tiennou tiennou commented Dec 18, 2013

Your indent settings looks "not correct" (for that file definition of correct).

Also, I'm not fond of the "let's ask users to reset their stuff". Wouldn't it be possible to convert on the fly ? And I'm really not sure how going from keycodes to bitmask can change anything, especially given my last comment ;-).

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Dec 18, 2013

Sorry, another force push. I've:

  • Added brackets around the ternary operator
  • Changed the method definition back to how it was (Etienne was right - it wasn't used)
  • Changed the tags of the prefs items back to what they were - no need for users to reset anything

:)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Dec 18, 2013

And finally - not sure about the indentation. Here are my indentation settings (default)

screen shot 2013-12-19 at 07 13 14

mods = NSAlphaShiftKeyMask;
}

QSModifierKeyEvent *match = [modifierKeyEvents objectForKey:(mods ? [NSNumber numberWithUnsignedInteger:mods] : [NSNumber numberWithUnsignedInteger:lastModifiers])];
Copy link
Member

@tiennou tiennou Dec 18, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still no @() ?

@tiennou
Copy link
Member

@tiennou tiennou commented Dec 18, 2013

I'm not even sure we have a consistent indent character anyway... If you like nitpicking, can you

  • check how the majority of the file is indented (using Text > Show Invisibles),
  • make sure the lines you're changing respect that "majority" and
  • update Xcode's indent character settings for that file so it's good (IIRC it's in the inspector pane that allows you to repoint missing (a.k.a "red") files, near the middle-bottom).

Other than my lone comment, looks good.

(FTR, @() wraps int/long/yougettheidea to NSNumber, char * to NSString * and a bunch of others. You just have to make sure you don't pass it NULL (for the char * case only). That's nice shortcut ;-).

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Dec 19, 2013

OK, I’ve done the @()

I’ll leave the nitpicking for now, we’ve said before that we need to sort all the indentation - maybe in one sweep. Lots of our files are already mangled :(

On 19 Rhag 2013, at 07:38, Etienne Samson notifications@github.com wrote:

I'm not even sure we have a consistent indent character anyway... If you like nitpicking, can you

check how the majority of the file is indented (using Text > Show Invisibles),
make sure the lines you're changing respect that "majority" and
update Xcode's indent character settings for that file so it's good (IIRC it's in the inspector pane that allows you to repoint "red" files, near the middle-bottom).
Other than my lone comment, looks good.

(FTR, @() wraps int/long/yougettheidea to NSNumber, char * to NSString * and a bunch of others. You just have to make sure you don't pass it NULL (for the char * case only). That's nice shortcut ;-).


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member

@skurfer skurfer commented Dec 19, 2013

we’ve said before that we need to sort all the indentation - maybe in one sweep

We can use “Code cleanup II” as the commit message, since every git blame will refer to it. 😉

skurfer added a commit that referenced this issue Dec 19, 2013
Alter the single modifier code to use device independent key masks as opposed to key codes
@skurfer skurfer merged commit db8e721 into master Dec 19, 2013
1 check passed
@skurfer skurfer deleted the 1712 branch Dec 19, 2013
skurfer added a commit that referenced this issue Dec 19, 2013
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Dec 20, 2013

We can use “Code cleanup II” as the commit message, since every git blame will refer to it.

I thought about that, but since we (hopefully) won’t break anything by doing the re-indentation then it should never be in the git blame!
As long as we do nothing else in that commit everything should be fine. We just need to find a quiet spell when there aren’t any pull requests open… gulp ;-)

On 20 Rhag 2013, at 03:23, Rob McBroom notifications@github.com wrote:

We can use “Code cleanup II” as the commit message, since every git blame will refer to it.

@skurfer
Copy link
Member

@skurfer skurfer commented Dec 20, 2013

I thought about that, but since we (hopefully) won’t break anything by doing the re-indentation then it should never be in the git blame!

I rarely use it to find out who broke something. When I run across something that doesn’t seem to make sense, I try to see if there’s an explanation for it in the commit message.

So maybe when there are no pull requests and the code all makes intuitive sense, we can get away with it. 😃

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Dec 22, 2013

I rarely use it to find out who broke something. When I run across something that doesn’t seem to make sense, I try to see if there’s an explanation for it in the commit message.

Good point, I hadn’t thought about git blame. There may be a way to ignore a certain commit from git blame googles something… OK there is: http://stackoverflow.com/questions/4112410/git-change-styling-whitespace-without-changing-ownership-blame

We could do that :)

On 20 Rhag 2013, at 11:20, Rob McBroom notifications@github.com wrote:

I thought about that, but since we (hopefully) won’t break anything by doing the re-indentation then it should never be in the git blame!

I rarely use it to find out who broke something. When I run across something that doesn’t seem to make sense, I try to see if there’s an explanation for it in the commit message.

So maybe when there are no pull requests and the code all makes intuitive sense, we can get away with it.


Reply to this email directly or view it on GitHub.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Dec 22, 2013

…but it would involve rewriting history for the QS master branch. Uh oh…

On 22 Rhag 2013, at 20:13, Patrick Robertson robertson.patrick@gmail.com wrote:

I rarely use it to find out who broke something. When I run across something that doesn’t seem to make sense, I try to see if there’s an explanation for it in the commit message.

Good point, I hadn’t thought about git blame. There may be a way to ignore a certain commit from git blame googles something… OK there is: http://stackoverflow.com/questions/4112410/git-change-styling-whitespace-without-changing-ownership-blame

We could do that :)

On 20 Rhag 2013, at 11:20, Rob McBroom notifications@github.com wrote:

I thought about that, but since we (hopefully) won’t break anything by doing the re-indentation then it should never be in the git blame!

I rarely use it to find out who broke something. When I run across something that doesn’t seem to make sense, I try to see if there’s an explanation for it in the commit message.

So maybe when there are no pull requests and the code all makes intuitive sense, we can get away with it.


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member

@skurfer skurfer commented Dec 23, 2013

but it would involve rewriting history for the QS master branch.

Well, as long as everything’s quiet and there are no open pull requests… Wait, that sounds familiar. ;-)

Probably not worth it.

skurfer added a commit that referenced this issue Jan 2, 2014
skurfer added a commit that referenced this issue Jan 14, 2014
skurfer added a commit that referenced this issue Jan 17, 2014
skurfer added a commit that referenced this issue Jan 17, 2014
skurfer added a commit that referenced this issue Jan 21, 2014
skurfer added a commit that referenced this issue Jan 25, 2014
skurfer added a commit that referenced this issue Feb 2, 2014
skurfer added a commit that referenced this issue Feb 2, 2014
skurfer added a commit that referenced this issue Feb 5, 2014
skurfer added a commit that referenced this issue Feb 11, 2014
skurfer added a commit that referenced this issue Mar 19, 2014
skurfer added a commit that referenced this issue Apr 14, 2014
skurfer added a commit that referenced this issue May 13, 2014
skurfer added a commit that referenced this issue May 30, 2014
skurfer added a commit that referenced this issue Aug 7, 2014
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