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

Spacemacs home buffer should use `bookmark-jump` for bookmarks #2431

Closed
travisbhartwell opened this Issue Jul 27, 2015 · 5 comments

Comments

Projects
None yet
4 participants
@travisbhartwell
Contributor

travisbhartwell commented Jul 27, 2015

I have a bookmark to a PDF file that bookmarks to a specific page in the file. If I use helm-filtered-bookmarks, it correctly jumps to the bookmarked page. But if I use the bookmark link from the Spacemacs home buffer, it just opens the file, which ignores the context and the specific location in the file.

I read the code for helm-filtered-bookmarks and it uses bookmark-jump, which goes to the correct location.

So instead of just using file links as in the recent files list, visiting the link should call bookmark-jump for the link.

I may try attacking this, but I thought I would submit the bug to get ideas from @syl20bnr and others. Any guidance on the cleanest way to solve this, let me know. I totally want to do a PR for this.

@klauslh

This comment has been minimized.

Show comment
Hide comment
@klauslh

klauslh Jul 29, 2015

Contributor

Hi Travis,

This has been annoying me as well for some time.

One solution is to make a new function "insert-bookmark-list" which basically does the same as "insert-file-list" but use bookmark-jump instead of find-file:

(defun spacemacs-buffer//insert-bookmark-list (list-display-name list)
  (when (car list)
    (insert list-display-name)
    (mapc (lambda (el)
            (insert "\n    ")
            (widget-create 'push-button
                           :action `(lambda (&rest ignore) (bookmark-jump ,el))
                           :mouse-face 'highlight
                           :follow-link "\C-m"
                           :button-prefix ""
                           :button-suffix ""
                           :format "%[%t%]"
                           (abbreviate-file-name el)))
          list)))

And then you need to change a line in the insert-startupify-lists:

            (when (spacemacs-buffer//insert-bookmark-list "Bookmarks:" (bookmark-all-names))

However, I'm far from sure this is the cleanest way! Duplicating the insert-*-list function doesn't feel quite right. Looking forward to see some guidance from the lisp experts :-)

Contributor

klauslh commented Jul 29, 2015

Hi Travis,

This has been annoying me as well for some time.

One solution is to make a new function "insert-bookmark-list" which basically does the same as "insert-file-list" but use bookmark-jump instead of find-file:

(defun spacemacs-buffer//insert-bookmark-list (list-display-name list)
  (when (car list)
    (insert list-display-name)
    (mapc (lambda (el)
            (insert "\n    ")
            (widget-create 'push-button
                           :action `(lambda (&rest ignore) (bookmark-jump ,el))
                           :mouse-face 'highlight
                           :follow-link "\C-m"
                           :button-prefix ""
                           :button-suffix ""
                           :format "%[%t%]"
                           (abbreviate-file-name el)))
          list)))

And then you need to change a line in the insert-startupify-lists:

            (when (spacemacs-buffer//insert-bookmark-list "Bookmarks:" (bookmark-all-names))

However, I'm far from sure this is the cleanest way! Duplicating the insert-*-list function doesn't feel quite right. Looking forward to see some guidance from the lisp experts :-)

@TheBB

This comment has been minimized.

Show comment
Hide comment
@TheBB

TheBB Jul 29, 2015

Collaborator

Well, from what I see the number of things you need to change is two:

  • The :action to use bookmark-jump instead of find-file-existing
  • The actual display value, which must be a composition of bookmark-get-filename and abbreviate-file-name, instead of just abbreviate-file-name.

You could do this with optional arguments, say, but I'm not convinced the extra complexity is worth it. Might be simpler just to make a new function.

Seems like a great entry level issue. No need for a lisp expert. :-)

Collaborator

TheBB commented Jul 29, 2015

Well, from what I see the number of things you need to change is two:

  • The :action to use bookmark-jump instead of find-file-existing
  • The actual display value, which must be a composition of bookmark-get-filename and abbreviate-file-name, instead of just abbreviate-file-name.

You could do this with optional arguments, say, but I'm not convinced the extra complexity is worth it. Might be simpler just to make a new function.

Seems like a great entry level issue. No need for a lisp expert. :-)

@travisbhartwell

This comment has been minimized.

Show comment
Hide comment
@travisbhartwell

travisbhartwell Jul 29, 2015

Contributor

@fandag I've actually got almost this exact solution sitting on my hard drive.

@TheBB Thanks!

As soon as I can do my video about how to create a Pull Request for Spacemacs, I'll do a PR for this change.

Contributor

travisbhartwell commented Jul 29, 2015

@fandag I've actually got almost this exact solution sitting on my hard drive.

@TheBB Thanks!

As soon as I can do my video about how to create a Pull Request for Spacemacs, I'll do a PR for this change.

travisbhartwell added a commit to travisbhartwell/spacemacs that referenced this issue Jul 30, 2015

Fix Spacemacs Home Buffer to jump to bookmarks.
Instead of opening the file for the bookmark, use the bookmark-jump
function to properly jump to the file and location in the file.  Also
show the bookmark name and the filename in the list.

Fixes syl20bnr/spacemacs#2431
@klauslh

This comment has been minimized.

Show comment
Hide comment
@klauslh

klauslh Jul 30, 2015

Contributor

Looking forward to getting this fix merged. Thanks for bringing it up. By the way, very nice How-to-make-a-PR guide 😄

Contributor

klauslh commented Jul 30, 2015

Looking forward to getting this fix merged. Thanks for bringing it up. By the way, very nice How-to-make-a-PR guide 😄

syl20bnr added a commit that referenced this issue Jul 31, 2015

Fix Spacemacs Home Buffer to jump to bookmarks.
Instead of opening the file for the bookmark, use the bookmark-jump
function to properly jump to the file and location in the file.  Also
show the bookmark name and the filename in the list.

Fixes syl20bnr/spacemacs#2431
@travisbhartwell

This comment has been minimized.

Show comment
Hide comment
@travisbhartwell

travisbhartwell Aug 5, 2015

Contributor

This can be closed since the fix has been merged.

Contributor

travisbhartwell commented Aug 5, 2015

This can be closed since the fix has been merged.

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