bm icon indicating copy to clipboard operation
bm copied to clipboard

New feature: bm-suggest-annotation

Open Boruch-Baum opened this issue 4 years ago • 4 comments

  • Establishes bm-suggest-annotation-alist mapping buffer major-modes to suggestion functions

  • Provides sample functions for emacs-lisp-mode and org-mode

  • Integrates the feature into functions bm-bookmark-annotate and bm-bookmark-add

  • Provide a bookmark's current annotation, if it exists.

  • When adding a bookmark, the annotation prompt to the user must happen before the bookmark is created because the user can always abort the process (eg. C-g) leaving us with only a partially configured bookmark.

  • Snuck in: Give the user feedback on successful bookmark creation and removal.

Boruch-Baum avatar Apr 30 '21 15:04 Boruch-Baum

Thank you for submitting a pull request, but it is hard to review it when you rewrite the code and add new functionality at the same time.

I like the idea of creating suggestions for annotations but part of the pull request is too specific. I think adding support for creating suggestions makes sense, but the elisp- and org-mode functions should be excluded.

joodland avatar May 05 '21 22:05 joodland

On 2021-05-05 15:32, Jo Odland wrote:

Thank you for submitting a pull request, but it is hard to review it when you rewrite the code and add new functionality at the same time.

I like the idea of creating suggestions for annotations but part of the pull request is too specific. I think adding support for creating suggestions makes sense, but the elisp- and org-mode functions should be excluded.

You can do that, I don't mind you removing it from the PR, but I think it's unwise for two reasons:

  1. It makes the feature much less discoverable. Without examples for common emacs modes, people who update your package might never realize the feature exists or what it's useful for.

  2. Examples, even if not enabled by default, give users a template of a possible common use.

The examples were chosen for being common modes that I associate with people who would be likely to use it.

-- hkp://keys.gnupg.net CA45 09B5 5351 7C11 A9D1 7286 0036 9E45 1595 8BC0

Boruch-Baum avatar May 06 '21 02:05 Boruch-Baum

On 2021-05-05 15:35, Jo Odland wrote:

@@ -633,8 +686,16 @@ when bm-next' or bm-previous' navigate to this bookmark

This code is not calling bm-bookmark-annotate() but instead duplicating parts of that code.

Sorry about the delay on this response. Yes. It isn't calling function bm-bookmark-annotate because that function expects the bookmark object to already exist at point.

My observation was that when -creating- a bookmark, the annotation request needs to be performed BEFORE creation of the bookmark object, because the use can C-g abort out of function read-from-minibuffer with the expectation that the bookmark would not be created.

However, in the code before my PR the bookmark seems to have been created anyway, missing the components of the bm object after the annotation.

-- hkp://keys.gnupg.net CA45 09B5 5351 7C11 A9D1 7286 0036 9E45 1595 8BC0

Boruch-Baum avatar May 06 '21 22:05 Boruch-Baum

Any chance of this and my other PR getting merged anytime soon? I ask because I have another PR ready, to make bm play nicely with show-paren-mode. What I mean is that when show-paren-mode is set to highlight an expression, setting a bookmark at an open-paren combines the bm-face with the show-paren-face, and in my case the result is unreadable. So I have a mitigation that others may find useful.

Boruch-Baum avatar Jun 01 '21 14:06 Boruch-Baum