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

Add separate marker ring for goto-definition calls #44

Merged
merged 4 commits into from Apr 25, 2013

Conversation

Projects
None yet
3 participants
@immerrr
Collaborator

immerrr commented Apr 23, 2013

It's made to to facilitate popping back to where C-. was invoked.
Default keybinding is M-*.

It wasn't too hard, actually.

Feel free to ping me if you insist on having M-* key customized like the rest of the keybindings. Feel free to close the issue in favor of the pull-request (issue #42).

Add separate marker ring for goto-definition calls
It's made to to facilitate popping back to where `C-.` was invoked.
Default keybinding is `M-*`.
jedi.el Outdated
@@ -334,6 +338,7 @@ toolitp when inside of function call.
(define-key map jedi:key-complete 'jedi:complete)
(define-key map jedi:key-goto-definition 'jedi:goto-definition)
(define-key map jedi:key-show-doc 'jedi:show-doc)
(define-key map (kbd "M-*") 'jedi:goto-definition-pop-marker)

This comment has been minimized.

@tkf

tkf Apr 23, 2013

Owner

Can you add jedi:key-goto-definition-pop-marker so that it is configurable?

Also please document it in jedi:setup-keys and manual (doc/source/tail.rest).

This comment has been minimized.

@yyr

yyr Apr 23, 2013

FWIW, I agree it should be configurable, more over, M-* has a global binding

This comment has been minimized.

@tkf

tkf Apr 23, 2013

Owner

@yyr You are right, thank for the info.

How about C-, or C-*? Both are empty in plain Emacs. I took C-. from M-. of SLIME. SLIME binds M-, (and M-*) for pop so I think it makes sense.

(Stricktly speaking we should bind C-c + punctuation for minor mode but that's why I am not enabling it by default anyway.)

This comment has been minimized.

@yyr

yyr Apr 23, 2013

C-, , C-* are good as we have already C-.

This comment has been minimized.

@tkf

tkf Apr 23, 2013

Owner

@immerrr let's use C-, then

This comment has been minimized.

@immerrr

immerrr Apr 23, 2013

Collaborator

@yyr, that's actually intentional. I wanted to provide intuitive transition from tags-based browsing to jedi:goto-definition. And you always can do (define-key jedi-mode-map (kbd "M-*") nil) if you miss those tags and don't like the idea. But if you insist... sure.

@tkf, M-. of SLIME was probably an exact same idea: they rebound find-tag function key (also a global binding) to avoid confusion :)

This comment has been minimized.

@yyr

yyr Apr 23, 2013

@immerrr does this totally replace tag based navitaion.? if so I can vote for such keybinding, But after all nothing wrong in making configurable with a defcustom.

This comment has been minimized.

@tkf

tkf Apr 23, 2013

Owner

@immerrr If we want jedi:goto-definition-pop-marker to override pop-tag-mark, we should change jedi:goto-definition too. As we discussed in #42 (comment), we may change the default keybind and/or how they are configured soon. For the time being, I want make it consistent. What do you think?

This comment has been minimized.

@immerrr

immerrr Apr 24, 2013

Collaborator

@tkf sure, no problem

@@ -287,6 +287,10 @@ avoid collision by something like this::
"Automatically import setting from python.el variables."
:group 'jedi)
(defcustom jedi:goto-definition-marker-ring-length 16

This comment has been minimized.

@tkf

tkf Apr 23, 2013

Owner

Please document this in the manual (doc/source/tail.rest).

jedi.el Outdated
(interactive)
(ring-insert jedi:goto-definition--marker-ring (point-marker)))
(defun jedi:goto-definition-pop-marker ()

This comment has been minimized.

@tkf

tkf Apr 23, 2013

Owner

Please add both jedi:goto-definition-push-marker and jedi:goto-definition-pop-marker in the manual (doc/source/tail.rest) as well.

This comment has been minimized.

@immerrr

immerrr Apr 24, 2013

Collaborator

Done, but I think push-marker should be an internal function, like goto-definition--nth. Feel free to remove it from the doc if you agree.

This comment has been minimized.

@tkf

tkf Apr 24, 2013

Owner

Ah, I thought you have some interactive use jedi:goto-definition-push-marker was in mind, as it is defined as a command. Let's remove it from the manual and remove the line (interactive).

This comment has been minimized.

@immerrr

immerrr Apr 25, 2013

Collaborator

Aw, that was an error, I shouldn't have left it there after testing manually.

@tkf

This comment has been minimized.

Owner

tkf commented Apr 23, 2013

Thank you, except the minor points I mentioned it looks fine. I need to check travis failure before merging, though. Probably some package start using functions that are not in 23.

@immerrr

This comment has been minimized.

Collaborator

immerrr commented Apr 24, 2013

Done.

Btw, speaking of key binding conventions, C-c <LETTER> should be reserved for user bindings, AFAIR.

@tkf

This comment has been minimized.

Owner

tkf commented Apr 24, 2013

That's one of the reason why I am not activating it by default. And the reason why I was using that keybinding was to make it compatible with ropemacs. Probably I shouldn't take that keybinding in the first place.

tkf added a commit that referenced this pull request Apr 25, 2013

Merge pull request #44 from immerrr/add-separate-marker-ring
Add separate marker ring for goto-definition calls

@tkf tkf merged commit 40c51a6 into tkf:master Apr 25, 2013

1 check failed

default The Travis build failed
Details
@tkf

This comment has been minimized.

Owner

tkf commented Apr 25, 2013

Merged. Thanks for patience!

@immerrr immerrr deleted the immerrr:add-separate-marker-ring branch Apr 25, 2013

@tkf tkf referenced this pull request May 18, 2013

Closed

Add separate mark-ring #42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment