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

Added keybindings for custom commands 4-9 #858

Closed
wants to merge 1 commit into from
Closed

Conversation

tsahlin
Copy link
Contributor

@tsahlin tsahlin commented Jan 8, 2016

Added additional keybindings so that one can have up to 9 custom editor commands.

Split from previous pull request #792

@tsahlin
Copy link
Contributor Author

tsahlin commented Jan 8, 2016

Could you point me to some guide or info on how to update the docs?

@elextr
Copy link
Member

elextr commented Jan 8, 2016

docs/geany.txt its in restructured text format. You would need docutils if you wanted to build the html.

@eht16
Copy link
Member

eht16 commented Jan 9, 2016

Just to make it clear: you don't need to build the HTML from geany.txt for this PR. Just modify the doc/geany.txt file.
However, if you want to build it to test your changes or so, just run make -C doc geany.html in the top source directory of Geany. Ideally, do not include the modified geany.html into this PR.

@elextr
Copy link
Member

elextr commented Jan 9, 2016

It would be good if you could fix the merge conflict as well.

@b4n
Copy link
Member

b4n commented Jan 18, 2016

Apart from the missing manual update and merge conflict (easy to fix), untested but looks good.

@b4n
Copy link
Member

b4n commented Jan 18, 2016

Hum, however, do we want to have default bindings for these? Remember that a binding in Geany overrides one in a plugin, so if a user already had mapped one of those to a plugin it will override the user choice. (AFAIK)

@b4n
Copy link
Member

b4n commented Jan 18, 2016

https://github.com/b4n/geany/tree/tsahlin/more-cc-kb rebases this on top of matser (fixing the conflict), and updates the documentation

@eht16
Copy link
Member

eht16 commented Jan 20, 2016

I agree with @b4n: we should not add defaults for the new keybindings. Btw, I'm actually one of the users @b4n described: I've bound Ctrl-4 and -5 to the GeanyVC plugin.

@elextr
Copy link
Member

elextr commented Jan 20, 2016

Since we recommend plugins not set default bindings we probably should not trample all over users existing bindings with Geany itself. That of course means we can never add any default bindings to Geany evermore.

@codebrainz
Copy link
Member

It seems like a bug (if true) that adding new default keybinding overrides those set explicitly by the user, for plugins. We should be able to add new keybindings and they should only have effect if the user hasn't customized them like they does for core keybindings.

@b4n any idea where/why this bug exists for the case of plugin keybindings?

@elextr
Copy link
Member

elextr commented Jan 21, 2016

@codebrainz I believe its because the Geany keybindings are searched before the plugin ones.

@codebrainz
Copy link
Member

because the Geany keybindings are searched before the plugin ones

If you mean it searches default Geany keybindings before customized plugin ones, then yeah, that's definitively a bug then. It should do:

  1. Customized core keybindings
  2. Customized plugin keybindings
  3. Default core keybindings
  4. Default plugin keybindings

One could argue whether core or plugin keybindings should be used first, but nothing the user has customized (ie. the stuff in home dir) should ever be overridden by system defaults, IMO.

@elextr
Copy link
Member

elextr commented Jan 21, 2016

If you mean it searches default Geany keybindings before customized plugin ones, then yeah, that's definitively a bug then.

IIRC it doesn't differentiate between customised and default after startup and loading of the config files.

@eht16
Copy link
Member

eht16 commented Feb 7, 2016

Apart from the discussion about Geany's handling of core and plugin keybindings which seem to be an issue on its own, I'd like to get this one ready.

@tsahlin would you mind incoporating b4n's changes (resolved merge conflict & doc update) into this PR and remove the default values for the keybindings? Then we could merge this PR.

@tsahlin
Copy link
Contributor Author

tsahlin commented Feb 8, 2016

Sorry to say I won't have any time to fix this for another 6 weeks. I'm also really a newbie at git/github so it's not straightforward.

If anyone is willing and can spare the time, feel free to modify the PR to better fit the project.

@eht16
Copy link
Member

eht16 commented Feb 15, 2016

@b4n could you just merge your branch with the resolved conflicts and remove the defaults for the new keybindings? I guess this would least work and we would get this one done.

@b4n b4n closed this in 372d763 Feb 16, 2016
@b4n
Copy link
Member

b4n commented Feb 16, 2016

@eht16 done

@b4n b4n added this to the 1.27 milestone Feb 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants