Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Use windows-specific keybindings on linux#2331

Merged
redmunds merged 9 commits into
masterfrom
jasonsanjose/linux-keybindings
Dec 13, 2012
Merged

Use windows-specific keybindings on linux#2331
redmunds merged 9 commits into
masterfrom
jasonsanjose/linux-keybindings

Conversation

@jasonsanjose
Copy link
Copy Markdown
Member

Completes pull request #2000 from @pritambaral. Adds additional check for linux as the current platform before mapping linux.

@peterflynn
Copy link
Copy Markdown
Member

One quick note on this -- I think during sprint planning we talked about doing something a little farther from the original pull request. I think we wanted to have a notion of a default keybinding alongside platform-specific (so rather than registering Mac-specific + Win-specific, we could register Mac-specific plus default). We also talked about making it possible to register Linux-specific bindings distinct from Windows ones, but I forget where we landed on whether that needs to happen now or could wait.

@redmunds
Copy link
Copy Markdown
Contributor

Yes, that's also how I remember it.

@jasonsanjose
Copy link
Copy Markdown
Member Author

Thanks for the reminders guys. I'll get to it now.

@jasonsanjose
Copy link
Copy Markdown
Member Author

Long story, but I was working on this inside the linux shell and even though my date/time was managed automatically my system date was way wrong. Commits 6881bf2 and 9bca94b were submitted today 12/21 near 3:30pm.

Anyhow, I've fixed the base-config file to change windows-specific bindings default (for win and linux) and leave the mac shortcut untouched. I also corrected some logic around detecting mac vs. non-mac. Ready for review.

@pritambaral
Copy link
Copy Markdown
Contributor

I guess this would mean extra work converting all those win+mac bindings
back to default+mac. Say this is done for the built-in bindings, but third
party plugins would have to change too. And until they do, other platforms
(non-windows, non-mac) would not have full support for them.

Even so, I think this is a very good design decision, and only wish this
was taken at the very beginning.

On Thu, Dec 13, 2012 at 5:06 AM, Jason San Jose notifications@github.comwrote:

Long story, but I was working on this inside the linux shell and even
though my date/time was managed automatically my system date was way wrong.
Commits 6881bf2 6881bf2 and
9bca94b 9bca94b were submitted
today 12/21 near 3:30pm.

Anyhow, I've fixed the base-config file to change windows-specific
bindings default (for win and linux) and leave the mac shortcut untouched.
I also corrected some logic around detecting mac vs. non-mac. Ready for
review.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2331#issuecomment-11315324.

Comment thread src/command/KeyBindingManager.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wrap these lines at 80 (or so) chars.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed

@redmunds
Copy link
Copy Markdown
Contributor

@jasonsanjose : @pritambaral makes a good point. Maybe there should be a check for the case where:

  • keybinding is not defined for platform or generic
  • platform is not win or mac
  • keybinding is defined for win

Then use win keybinding

@redmunds
Copy link
Copy Markdown
Contributor

Done with initial review.

@jasonsanjose
Copy link
Copy Markdown
Member Author

By request, added a fallback for linux to use windows bindings. Also, if a linux or generic binding comes in after a windows binding is used, the windows binding is clobbered.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixes a bug loading unit tests on linux

@redmunds
Copy link
Copy Markdown
Contributor

The "fallback for linux to use windows bindings" code looks good. @pritambaral Please create a test extension to verify this works.

@redmunds
Copy link
Copy Markdown
Contributor

Looks good. Merging.

redmunds added a commit that referenced this pull request Dec 13, 2012
Use windows-specific keybindings on linux
@redmunds redmunds merged commit b1200e3 into master Dec 13, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants