Bruno Barbier <brubar...@gmail.com> writes: >> 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.
Ok. >> 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. Sorry, that's not what I meant. I was hoping that you can make use of inherit face attribute to define faces. Using just a part of an existing face is not a good idea - faces can be changed to make the _full_ combination of foreground/background/etc legit, but not necessarily to make individual color values reusable. If you have no ideas about faces to inherit from, better keep hard-coded colors. (Also, this is not too critical; just something nice to have for better inter-operability with Emacs themes) >>> (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). I prefer to have docstrings for every function, including internal functions. This will make life easier for future contributors when diagnosing whether a given function behaves as it supposed to. Ideally, we should detail what the function is expected to do by its callers in its docstring. This way, we have a way to check if the code behaves as it should in the future, after situations like external API change or unexpected change in another internal API. > I've pushed the changes to my branch. Thanks! Next set of comments :) > (let ((map (make-sparse-keymap))) > (dolist (k `([touchscreen-down] [mouse-2] [mouse-1])) > (define-key map k 'org-pending-describe-reglock-at-point)) > (when read-only > (define-key map [13] 'org-pending-describe-reglock-at-point)) > map)) Nitpick: #'org-pending... - this will help compiler to catch problems if something happens to `org-pending-describe-reglock-at-point' (like rename). Also, [13] is not very readable. Better use ?\C-m (or what did you mean?) > (defun org-pending--make-overlay (type beg-end) > ... > (let ((overlay (make-overlay (car beg-end) (cdr beg-end))) > (read-only > (list > (lambda (&rest _) > (signal 'org-pending-error > (list "Cannot modify a region containing pending > content")))))) You can factor this lambda out into an internal constant. > (defun org-pending--make-overlay (type beg-end) > "Create a pending overlay of type TYPE between BEG-END. > The pair BEG-END contains 2 positions (BEG . END). > Create an overlay between BEGIN and END. Return it. > See `org-pending--delete-overlay' to delete it." It would be nice to have the docstring detail what kinds of properties are applied to the overlay: (1) that it is read-only; (2) org-pending--owner; (3) org-pending--before-delete; (4) keymap. It is especially important to document properties that other functions make use of. > (let ((overlay (make-overlay (car beg-end) (cdr beg-end))) > (read-only > (list > (lambda (&rest _) > (signal 'org-pending-error > (list "Cannot modify a region containing pending > content")))))) May you factor this lambda out into an internal constant? > (cl-flet ((make-read-only (ovl) > "Make the overly OVL read-only." > (overlay-put ovl 'modification-hooks read-only) > (overlay-put ovl 'insert-in-front-hooks read-only) > (overlay-put ovl 'insert-behind-hooks read-only))) Or maybe even factor out make-read-only into an internal function that can mark/unmark overlay/region with read-only text properties. > (overlay-put overlay 'org-pending type) > (unless (memq type '(:success :failure)) > (overlay-put overlay 'face 'secondary-selection) Better use a new org-pending-specific face (inheriting from secondary-selection). > (overlay-put > overlay 'help-echo > (substitute-command-keys > (concat "\\<org-pending-pending-keymap>" > "This content is pending. " > "\\[org-pending-describe-reglock-at-point]" > " to know more.")))) You set a similar help-echo string in two places. It is a good candidate to factor things out into a helper function. > ;; Hack to detect if our overlay has been copied into an other > ;; buffer. > (overlay-put overlay 'org-pending--owner (overlay-buffer overlay)) AFAIU, the sole purpose of this is > ;; Try to detect and delete overlays that have been wrongly copied > ;; from other buffers. ... but cloning buffer does not copy its overlays. Or do I miss something? Also, "ownder" buffer is reachable via the associated REGLOCK object (`org-pending-reglock-owner'). > (when (memq type '(:success :failure)) > ;; Add a link to the outcome overlay so that we may remove it > ;; from any buffer. > (with-silent-modifications > (add-text-properties (car beg-end) (cdr beg-end) > (list 'org-pending--outcome-overlay overlay))) Do I understand correctly that this is to support indirect buffers? If so, why is it not a part of `org-pending--add-overlay-projection'? > (overlay-put overlay 'org-pending--before-delete > (lambda () > (let ((inhibit-modification-hooks t) > (inhibit-read-only t)) > (overlay-put overlay 'modification-hooks nil) > (overlay-put overlay 'insert-in-front-hooks nil) > (overlay-put overlay 'insert-behind-hooks nil) > (org-pending--remove-overlay-projection overlay) > ;; Force refontification of the result > ;; (working around indirect buffers hacks). > (let ((start (overlay-start overlay)) > (end (overlay-end overlay))) > (remove-text-properties start end > (list 'fontified :not-used > 'font-lock-face > :not-used > 'font-lock-fontified > :not-used)) > (font-lock-flush start end))))) This feels like overengineering. I'd rather put the overlay removal code into `org-pending--delete-overlay'. I see no reason to increase memory used to store text/overlay properties unless we need to. Also, a more general question on `org-pending--make-overlay' design: Why also not setting 'org-pending-reglock property right within this function? Same for other places that modify the overlay in place after creating. It feels like REGLOCK, face, help-echo (or even part of it), and before-string can easily be parameters of `org-pending--make-overlay'. At least, REGLOCK. Other properties may be simply passed as some kind of (... &rest other-properties) argument for `org-pending--make-overlay'. IMHO, it would be nice to keep everything related to creating the overlay in one function rather than spreading it all over the place. > (defun org-pending-reglock-status (reglock) > "Return the status of REGLOCK. > The possible status are, in chronological order: > :scheduled => > :pending => > :success > or :failure." > (org-pending-reglock--status reglock)) The return value is a symbol, right? You need to document this fact. > (defun org-pending-reglock-live-p (reglock) > "Return non-nil if REGLOCK is still live. > A REGLOCK stays live until it receives its outcome: :success or :failure." > (funcall (org-pending-reglock--get-live-p reglock))) Here as well. I suggest marking the outcome types in the docstrings as `:success' or `:failure'. > (defun org-pending-reglock-duration (reglock) > "Return REGLOCK duration between its scheduling and its outcome. > If the outcome is not known, use the current time." > (let ((start (org-pending-reglock-scheduled-at reglock)) > (end (or (org-pending-reglock-outcome-at reglock) > (float-time)))) > (- end start))) Is the return value in seconds? Internal time representation? > (defun org-pending-reglock-set-property (reglock prop val) > "Set the value of the property PROP for this REGLOCK. > See also `org-pending-reglock-property'." > (if-let ((b (assq prop (org-pending-reglock-properties reglock)))) > (setcdr b val) > (push (cons prop val) > (org-pending-reglock-properties reglock)))) `alist-get' is setf-able as well :) > (defun org-pending--user-cancel-default (reglock) > "Send a cancel message to REGLOCK to close it. > Default value for `org-pending-reglock-user-cancel-function'." > (org-pending-send-update > reglock (list :failure (list 'org-pending-user-cancel > "Canceled")))) What is the purpose of 'org-pending-user-cancel? Also, '(:failure (org-pending-user-cancel "Canceled)) would be shorter. > (defun org-pending-reglock-delete-outcome-marks (reglock) Not very related, but I am wondering if an API function to retrieve reglock at point could be useful. WDYT? -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92>
