Hi Ihor,
Thanks for the review. I've pushed a new version, hoping to decrease the number of dislikes ;-) Ihor Radchenko <yanta...@posteo.net> writes: > Bruno Barbier <brubar...@gmail.com> writes: > >>> I have a further request on interaction with penreg objects. >>> I feel that it is not ideal that overlays associated with penreg objects >>> cannot be fully controlled by the callers. >> >> I'm trying to limit the public API surface. I don't think we should >> leak that we are currently using a mix of overlays and text >> properties. > > Let me rephrase my concern - I do not like that after reglock is no > longer live (got success/failure signal), there is no way to clean up the > visual hints associated with this particular reglock. [....] For the org-pending library, "live" means "locked". Once the outcome is known, it can't be "live" anymore (it's unlocked); as it's not reusable, it's "dead". As the region is not locked anymore, the lock properties/fields can't be trusted anymore. But see below about removing the visual outcome hints of a given reglock. >>> 2. Act on the outcome overlays - there is currently no way to remove >>> them using penreg object. >> >> I've added a funcion `org-pending-delete-outcome-marks' to manually >> delete outcome marks that are in a given region. >> >> Else, everything is handled automatically. Once the outcome is known, >> the reglock is dead (not live-p). org-pending may leave outcome marks >> about the outcomes (outcome marks are optional). The outcome marks >> automatically disappear if the user remove the section, or, if a new >> lock is created for the same region. > > I do not like this. > I'd like the Elisp program that creates the reglock to be able to > clean up any visual hints associated with it. > A function doing it for a > given region cannot do this AFAIU. ok. I've added the function `org-pending-reglock-delete-outcome-marks, that will delete the outcome visual hints for a given reglock, if there are some. I updated how the lock is described to the user (org-pending-describe-reglock): I added a button "Forget" (if the lock is dead, that removes the outcome marks), and I added a "Cancel" button if the lock is still live. >>> Maybe :cancel signal? Canceled penreg >>> objects can then be garbage-collected from the manager. >> >> Cancel is handled by sending a failure message (see >> `org-pending-cancel'). It's customizable using the reglock field >> ~org-pending-reglock-user-cancel-function~, which can decide what to >> do (like kill a process) and which can send a better outcome. >> Standard 'cancel' leaves a failure outcome mark. > > Note that this function is not documented anywhere other than in reglock > class documentation. Thanks. I've improved the documentation of `org-pending' to mention that one may want to customize the following fields of a reglock: before-kill-function, user-cancel-function and insert-details-function. And, also, I added that one can attach custom properties using the "properties" field. > In general, I am confused about your overall design > of the user interaction with the locks. > The updated top commentary explains well how Elisp programs can send > data to the locks, but it does not say anything about how Elisp programs > can receive the data. An elisp program, that uses org-pending, must update the locks using `org-pending-send-update'. That program does not receive any data from the lock; it may customize Emacs behavior using the reglock fields mentioned above: before-kill-function, user-cancel-function and insert-details-function. Hopefully, it's clearer now with the improved documentation of the org-pending function. Just let me know if you still think that the top commentary should explain this. Thanks. > Also, I'd like to see more information in the top commentary about what > are the "visual hints" displayed to the user and how to configure them. If you think the current "visual hints" are good enough and could be shipped as-is, in a first version (indirect buffers, etc.); I could work on documenting them better. What kind of configuration are you thinking about ? just the faces ? or more advanced configurations ? [...] >>> Is there any reason why you hide the extra information behind :-alist >>> filed? Why not directly adding extra fields with proper documentation? >> >> To hide them, indeed :) > >> The API for 'get-status and 'get-live-p are >> `org-pending-reglock-status' and `org-pending-reglock-live-p' (they >> are read-only). The API for the new `useless-p' is >> `org-pending-reglock-useless-p' (it's read-only too). > > We usually "hide" fields by declaring them private. > Hiding them from the type docs is not a good idea because it defeats the > purpose of type documentation in general. > >> The fields anchor-ovl, region-ovl, on-outcome, set-status and >> creation-point are the dump of the closure context, so that >> org-pending doesn't rely anymore on a closure to handle updates; I've >> rewritten that recently. Nobody is supposed to use or change those >> values, except the update process. >> >> IMHO, dumping those as fields in the lock structure would be more >> confusing and fragile than keeping those out of sight. I could add >> comments when they are created/used in the code to help understand how >> they are used. > > I disagree. In particular, I dislike the fact that they are not > documented anywhere and one has to read the internals of the code to > understand their purpose. Done. I hope the minimal documentation is enough. Thanks again for your reviews and your comments, Bruno