fix #60 (shorcuts settings on linux)

[mmai]

This fixes the session crashing on ubuntu 17.10 (and probably on other versions as well).

[nitzanel]

When running this fork i get the following error:

expected ',' or ']' to follow array element:
  ['/org/gnome/settings-daemon/plugins/media-keys/custom-keybindings/custom0/,'/org/gnome/settings-daemon/plugins/media-keys/custom-keybindings/vim-anywhere/']

I think there is a problem in the sed command at line 115.

[nitzanel]

When running this fork i get the following error:

expected ',' or ']' to follow array element:
  ['/org/gnome/settings-daemon/plugins/media-keys/custom-keybindings/custom0/,'/org/gnome/settings-daemon/plugins/media-keys/custom-keybindings/vim-anywhere/']

I think there is a problem in the sed command at line 115.

[mmai]

Oh, you are right. It's fixed, thanks

[cknadler]

Hey Henri,

Thanks for the PR! I'm not very familiar with sed, so line 115 throws me for a bit of a loop. Any way you can break this down / document what you're doing here so I can understand? Thanks!

Cheers,
Chris

[cknadler]

Hey Henri,

Thanks for the PR! I'm not very familiar with sed, so line 115 throws me for a bit of a loop. Any way you can break this down / document what you're doing here so I can understand? Thanks!

Cheers,
Chris

[mmai]

Hi Chris,

Thanks for this nice app

Here I use sed with the > character as a separator, for example echo "hello" | sed -e "s>ell>arp>" replaces ell by arp in hello and gives harpo.

The goal of the line is to get the current 'custom-keybindigs' configuration and to add the $kbd_path entry. So we first get the configuration with gsettings get $media_keys custom-keybindings which gives something like ['/org/gnome.../custom0', '/org/gnome/.../custom1/'] if there are some bindings, but returns @as [] if there is no binding.

There are two sed commands chained to take these two cases in account.

The first sed command manages the case when there are some bindings :
it replaces '] by ', '$kbd_path'], so ['/org/gnome.../custom0', '/org/gnome/.../custom1/'] becomes ['/org/gnome.../custom0', '/org/gnome/.../custom1/', '$kb_path']

The second one manages the case when there is no binding, it replaces @as [] by ['$kbd_path'].

The first sed command does nothing when there is no binding because there is no ' character in the original string @as [], and the second sed command does nothing when there are bindings because in this case the original string does not contain the sequence @as [] .

I hope it makes more sense,

Henri

[mmai]

Hi Chris,

Thanks for this nice app

Here I use sed with the > character as a separator, for example echo "hello" | sed -e "s>ell>arp>" replaces ell by arp in hello and gives harpo.

The goal of the line is to get the current 'custom-keybindigs' configuration and to add the $kbd_path entry. So we first get the configuration with gsettings get $media_keys custom-keybindings which gives something like ['/org/gnome.../custom0', '/org/gnome/.../custom1/'] if there are some bindings, but returns @as [] if there is no binding.

There are two sed commands chained to take these two cases in account.

The first sed command manages the case when there are some bindings :
it replaces '] by ', '$kbd_path'], so ['/org/gnome.../custom0', '/org/gnome/.../custom1/'] becomes ['/org/gnome.../custom0', '/org/gnome/.../custom1/', '$kb_path']

The second one manages the case when there is no binding, it replaces @as [] by ['$kbd_path'].

The first sed command does nothing when there is no binding because there is no ' character in the original string @as [], and the second sed command does nothing when there are bindings because in this case the original string does not contain the sequence @as [] .

I hope it makes more sense,

Henri

[base0x10]

Hey, I tested the most recent version of this PR on Fedora 27 and Ubuntu 17.10 (both of which suffer from #60 which crashes gnome on startup). LGTM

[base0x10]

Hey, I tested the most recent version of this PR on Fedora 27 and Ubuntu 17.10 (both of which suffer from #60 which crashes gnome on startup). LGTM

[akkartik]

Hi @cknadler, while you guys go back and forth reviewing this PR, could you please add a note to the Readme that people probably shouldn't run the install script on Linux, or something like that? It'll save people a whole lot of pain, and probably spare you some bad vibes as well. You can always remove that prose once you're confident it's not going to fuck up people's window managers in strange ways.

[akkartik]

Hi @cknadler, while you guys go back and forth reviewing this PR, could you please add a note to the Readme that people probably shouldn't run the install script on Linux, or something like that? It'll save people a whole lot of pain, and probably spare you some bad vibes as well. You can always remove that prose once you're confident it's not going to fuck up people's window managers in strange ways.

[cknadler]

@mmai Awesome, thanks Henri. I'll merge this for now. If you can accomplish the same result without shelling out to sed, I'd happily accept another pull request. I think I understand what you're doing now, but I'm afraid I'll have a tough time maintaining this if I ever need to change it.

Thanks again!

[cknadler]

@mmai Awesome, thanks Henri. I'll merge this for now. If you can accomplish the same result without shelling out to sed, I'd happily accept another pull request. I think I understand what you're doing now, but I'm afraid I'll have a tough time maintaining this if I ever need to change it.

Thanks again!