Hi Ihor,
Ihor Radchenko <yanta...@posteo.net> writes: > Bruno Barbier <brubar...@gmail.com> writes: > >>>> I added a function 'org-pending-unlock-NOW!' which unlock the region >>>> immediately. The uppercase "NOW!" emphasizes that it's not the >>>> "safe" way to unlock a region. >>> >>> I expect to see this function called by some kind of button in the >>> details buffer, so that users can actually call it. >> >> This function should not be used, not even in code, except to work >> around bugs. I would prefer not to provide a button for it. > > Then, there is no point in this function - users will never know about > it. Maybe you do expose it as a button, but also supply a yes/no prompt > asking for confirmation? > The function 'org-pending-unlock-NOW!' is part of the API, it's not a command Emacs end users. If we make it a command and display that button by default, we'll need also an option to not display it by default, and, probably an other option to not ask for confirmation. Let see later if we really need to provide all this. >>> We should provide a user option to suppress the query. Something like >>> what `confirm-kill-processes' does. Maybe even support something akin >>> `process-query-on-exit-flag', but for reglocks. >> >> IMHO, it's the package that uses org-pending that should provide such a >> feature: I may not care about loosing data by ignoring on-going code >> block executions, but, I'll probably care if Emacs aborts some money >> transfers. > > I kindly disagree. I am not proposing anything dramatically different > from what Emacs already does. > > If you look into `confirm-kill-process' docstring, you will see that > Emacs does, in fact, have mechanisms for Elisp packages to tell whether a > given process is important or not (`process-query-on-exit-flag'). > > Yet, `confirm-kill-processes' overrides that mechanism if Emacs user > wishes to and sets the value to non-default nil. > > There is nothing principally different in org-pending users compared to > processes. So, in order to conform with the rest of Emacs API, we do > need to provide an equivalent user customization. > We will not suppress queries by default, but we ought to give users an > option to suppress these queries. Maybe even follow > `confirm-kill-process' setting in how org-pending behaves. In my view, reglocks are more about regions that are being updated, than processes; they are kind of _planed_ buffer modifications. But, anyway, I added a user option `org-pending-confirm-ignore-reglocks-on-exit' that allows to ignore pending locks when exiting. >>> Please describe what the default value of ON-OUTCOME (when ON-OUTCOME is >>> not explicitly provided) does right in the docstring. >> >> Done. > > Thanks! > There is one typo in the relevant commit: > > + The default ON-OUTCOME > +function replaces the region on success and ignore failures; in all > ^ignores > Thanks. >> (defface org-pending-outcome-failure >> ... >> (defface org-pending-outcome-success > > If possible, please derive the faces from either org-faces.el or from > built-in faces like the ones listed in `modus-themes-faces' variable. > > Ideally, we should have no hard-coded color names. They were derived from built-in ones (error, success, org-tag, etc.). I've redesigned the faces and put them all in org-pending, so that org-pending is indenpendent of Org. I'm now computing some colours from built-in faces to avoid colour names. I'm not sure it's what you meant, as even Org itself doesn't do this. >> :version "30.1" > Please, use :package-version instead of :version. I'm now using: :package-version '(Org . "9.7") >> (defun org-pending--status-still-pending-p (status) > > This function is trivial and used only once. Maybe you can just inline > it. Right. But it also defines what "still pending" means. I prefer to keep it. > >> (defvar org-pending--outcome-keymap >> (defvar org-pending--pending-keymap > > Why are these keymaps internal? May we better expose them to users for > modification? They are now part of the public API. > It may also be a good idea to add some actual keyboard binding to this > map. Right. I've improved the keymaps, trying to work like a button when the text is read-only. >> (defun org-pending--popup-failure-details (exc) > > This _internal_ function is unused in org-pending.el, but instead used > in ob-core.el. I am not yet looking into the changes outside > org-pending, but it is generally not ideal when library users need to > call an internal function from the library. > Also, this particular function does not look relevant to org-pending > library per se. It is rather generic and may be inlined. Indeed. I moved it into ob-core, where it is used. >> (defun org-pending-reglock-live-p (reglock) > > You introduce this API function, but yet using > `org-pending-reglock--get-live-p' directly in other places in the > library. It will be more readable to use `org-pending-reglock-live-p' > everywhere. > Done. Thanks. >> (defun org-pending-reglock-duration (reglock) > > The docstring does not mention REGLOCK argument. Done. >> (defun org-pending-reglock-property (reglock prop) > > ... PROP argument. Done. >> (defun org-pending-reglock-set-property (reglock prop val) >> "See `org-pending-reglock-property'." > > May as well write the full docstring. Done. >> (defun org-pending-reglock-delete-outcome-marks (reglock) >> "Delete visual hints of the outcome for this REGLOCK, if any. >> Do nothing if the outcome is not known. Do nothing if there are no > > We use double space between sentences in the docstrings. Oops. Sorry. > >> (cl-defun org-pending (region >> &key anchor name >> (on-outcome #'org-pending-on-outcome-replace)) > > NAME key is not documented in the docstring. > (and, more generally, please do M-x checkdoc on org-pending.el) Done. Thanks. > >> (user-error "This region lock has been destroyed.")))) > > Error messages should not end with period. Oops. Sorry. >> (let ((buffer (get-buffer-create "*Region Lock*"))) > > Please avoid using constant strings. Instead, declare them as defvar. Done. >> (defun org-pending-describe-reglock (reglock) >> "Describe REGLOCK in a buffer. >> >> Describe position REGLOCK. >> The information is displayed in new buffer. > > in *a* new buffer... except that it is not really a new buffer, when > another REGLOCK buffer is already being displayed. Thanks. And I've fixed the doc to more accurately describe what's happening. > May users want to see info about multiple reglocks at the same time? The function `org-pending-describe-reglock' works like `describe-variable' and other Emacs related functions: the user needs to rename the buffer manually if he wants to keep it. >> (setq-local header-line-format >> (format "Lock info (at %s), hit 'g' to update." >> (format-time-string "%T"))) > > Please, avoid using hard-coded bindings. Instead, see > `substitute-command-keys' (also see how org-capture.el sets up header > line) Done. Thanks for the references to the command and to one example. >> (defun org-pending--describe-reglock-at-point () > > Why internal? It is a command users may want to call via M-x. OK. >> (defun org-pending-pre-insert-outcome-default (_lock _message) >> "Default value for `org-pending-pre-insert-outcome-function'.") > > May as well just use #'ignore OK. Done. > >> (defvar org-pending-pre-insert-outcome-function >> (defvar org-pending-post-insert-outcome-function > > Should these be defcustom ? No. This is part of the developer API, not for the end user. > >> (overlay-put outcome-ovl >> 'help-echo >> "Last lock outcome, click to popup its full description.") > > This assumes fixed key bindings in the lock overlay keymap. Which may or > may not be the case. Please compute 'help-echo programmatically, based on > the key map. (using `substitute-command-keys'). Done. >> (cl-defun org-pending--update (reglock status data) > > No docstring. Please, add. I added some basic documentation; I hope this is enough (this is an internal function). > >> (let* ((reg (org-pending-reglock-region reglock)) >> (start (car reg)) >> (end (cdr reg)) >> (buf (marker-buffer start))) >> (when (buffer-live-p buf) >> (with-current-buffer buf >> (save-excursion >> (if (> (- end start) 1) > > What if the buffer is narrowed and reglock is outside the narrowing? Good point. I'm now using an inlined without-restriction. >> (with-current-buffer buffer >> (without-restriction > > `without-restriction' is only available since Emacs 29. However, we > still support Emacs 27 and will support Emacs 28 for quite a while. > So, please avoid `without-restriction' if possible. Done. I've inlined its definition. I've pushed the changes to my branch. Thanks again for your review and time, Bruno
