GNOME Bugzilla – Bug 677866
[PATCH] OSX: move to cocoa and fix keyboard shortcuts
Last modified: 2012-08-18 17:00:53 UTC
Created attachment 216121 [details] [review] see commit message in patch This patch drops deprecated carbon API support on OS X in favor for gtk-mac-integration with Cocoa API. Also introduces OS X specific keyboard shortcuts, which are defined in its own osx_accel_map file and reflect the iTunes bindings. See the commit message of the patch for more details. All of this changes only affect the src/Backends/Banshee.Osx directory, except some minor change: the separator in the core-ui-actions-layout must be given a name so I can remove it for OS X, as well as some keyboard shortcuts that were not bound are initialised with an empty accelerator "" so it can be bound by the Osx backend.
Created attachment 216821 [details] [review] newer version of the patch with some issues fixed this is an improved version of the patch that fixes two issues with keybindings with the BackSace and Questionmark (?) key. Additionally, osx_accel_map is put into its own Resources folder, else MonoDevelop will mess up with the Makefile.am.
Review of attachment 216821 [details] [review]: Thanks for the patch ! I've just committed the "move to cocoa" parts with a few cosmetic change to fit with our code convention guidelines (see the HACKING) file: http://git.gnome.org/browse/banshee/commit/?id=26a17639 I've left out the keyboard accelerator changes, because I'm not sure if doesn't change the behavior on Linux: setting the accelerator to "" means a default accelerator might not get used. For more info see bug #657565 and the corresponding commit: http://git.gnome.org/browse/banshee/commit?id=346b7ccc Could you re-submit those changes in a new patch ? I'm marking this one as committed. (As a side note, it's always better to have separate patches for changes that can stand on their own.)
We have to somehow find out whether changing the default accelerator from null to "" actually prevents users from changing menu accelerators in linux. After hours of trying I have finally found out, that Ubuntu 12.04 does not support re-assigning menu accelerators, even not after tweaking gconf entries as suggested here: http://askubuntu.com/questions/5083/can-i-set-up-nautilus-to-use-a-midnight-commander-like-hot-keys-scheme-for-2-pan/87118#87118 There is a bug report https://bugs.launchpad.net/ubuntu/+source/indicator-appmenu/+bug/610234 for this issue. Although, the workaround of remove indicator-appmenu didn't work for me. I used a university computer which uses debian-stable, and there I was able to re-assign shortcuts for GTK apps. Sadly I have no root rights there and the only development Linux i got available is the mentioned ubuntu 12.04 box. So I'm having a hard time checking whether the null to "" change interferes for linux users. Could somebody using a non-ubuntu box try this and confirm? Else I'd have to setup a new linux devel box to test.
Created attachment 217548 [details] [review] add empty accelerators so one can set them in an accel_map I could get my hands on an opensuse 12.1 box and tested against latest banshee-git. Again, I could not get to set my own (reassign) accelerators in ANY gtk+ app nor Banshee, the same way I was able to reassign with the debian-stable. I also tried to set can_accel_map via gconf-editor, same result. Is this feature even present in recent linux distributions or has it been abandoned? I also checked the latest gtk+ sourcecode, neither GTK_STOCK_PROPERTIES or GTK_STOCK_PREFERENCES have a default Stock accelerator set: (gtk 2.24.10, in file gtk/gtkstock.c:410): { GTK_STOCK_PREFERENCES, NC_("Stock label", "_Preferences"), 0, 0, GETTEXT_PACKAGE }, /* ... */ { GTK_STOCK_PROPERTIES, NC_("Stock label", "_Properties"), 0, 0, GETTEXT_PACKAGE }, { GTK_STOCK_QUIT, NC_("Stock label", "_Quit"), GTK_DEFAULT_ACCEL_MOD_MASK_VIRTUAL, 'q', GETTEXT_PACKAGE }, Note that GTK_STOCK_QUIT has the stock accelerator of CTRL+Q, while GTK_STOCK_PREFERENCES and GTK_STOCK_PROPERTIES have 0,0, which is no default accelerator according to the documentation. I therefore think the attached patch should be applied. There is no default stock accelerator in upstream GTK+2, and none set (to my testing) in Ubuntu 12.04 or openSUSE 12.1 gtk+ package, that can be overwritten by the patch.
Comment on attachment 217548 [details] [review] add empty accelerators so one can set them in an accel_map Thanks for updating the patch and all the research on that question. Committed, with a different commit message ;)
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.