New feature: bm-suggest-annotation #38
Conversation
+ 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 function bm-bookmark-annotate + 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.
|
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. |
| (buffer-substring (match-beginning 0) (match-end 0)) | ||
| "")) | ||
|
|
||
| (defun bm--suggest-annotation (&optional bm) |
There was a problem hiding this comment.
This should be an extension point rather than a specific function. Then users can add the specifics themselves.
| bookmark) | ||
| (when (and (not annotation) bm-annotate-on-create) | ||
| (setq annotation | ||
| (read-from-minibuffer "Annotation: " |
There was a problem hiding this comment.
This code is not calling bm-bookmark-annotate() but instead duplicating parts of that code.
|
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
|
|
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
|
|
Any chance of this and my other PR getting merged anytime soon? I ask because I have another PR ready, to make |
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.