Re: feature/improved-locked-narrowing 3bf19c417f: Merge master into feature/improved-locked-narrowing.

[Stefan Monnier]

Hi Gregory,

Here are some comments about the code on this branch.

> +(defmacro with-locked-narrowing (start end tag &rest body)
> +  "Execute BODY with restrictions set to START and END and locked with TAG.
> +
> +Inside BODY, `narrow-to-region' and `widen' can be used only
> +within the START and END limits, unless the restrictions are
> +unlocked by calling `narrowing-unlock' with TAG.  See
> +`narrowing-lock' for a more detailed description.  The current
> +restrictions, if any, are restored upon return."
> +  `(save-restriction
> +     (unwind-protect
> +         (progn
> +           (narrow-to-region ,start ,end)
> +           (narrowing-lock ,tag)
> +           ,@body)
> +       (narrowing-unlock ,tag)
> +       (widen))))

I think the final is redundant because of the surrounding `save-restriction`.

Based on past experience, I recommend you split it into a macro and
a function, where the function does the bulk of the work.  I.e.

    (defmacro with-locked-narrowing (start end tag &rest body)
      ...
      `(with-locked-narrowing-1 ,start ,end ,tag (lambda () ,@body)))

    (defun with-locked-narrowing-1 (start end tag body-fun)
      ...)

Also, I'd suggest you rename this to `with-narrowing` and make `tag`
optional (e.g. by providing it with a `:locked <TAG>` at the beginning
of `body`) such that if it's absent/nil then we skip the lock/unlock.

BTW, maybe the `narrowing-unlock` at the end should be made redundant as
well by making `save-restriction` automatically save&restore the locks?

>  DEFUN ("widen", Fwiden, Swiden, 0, 0, "",
>         doc: /* Remove restrictions (narrowing) from current buffer.
> -This allows the buffer's full text to be seen and edited.
>  
> -Note that, when the current buffer contains one or more lines whose
> -length is above `long-line-threshold', Emacs may decide to leave, for
> -performance reasons, the accessible portion of the buffer unchanged
> -after this function is called from low-level hooks, such as
> -`jit-lock-functions' or `post-command-hook'.  */)
> +This allows the buffer's full text to be seen and edited, unless
> +restrictions have been locked with `narrowing-lock', which see, in
> +which case the restrictions that were current when `narrowing-lock'
> +was called are restored.  */)

The doc should be more explicit about what happens when there's a lock
(it currently only says that it does not allow to see the full text but
doesn't say if it just does nothing or if it widens to the locked
narrowing or what).

>    (void)
>  {
> -  if (! NILP (Vrestrictions_locked))
> -    return Qnil;
> -  if (BEG != BEGV || Z != ZV)
> -    current_buffer->clip_changed = 1;
> -  BEGV = BEG;
> -  BEGV_BYTE = BEG_BYTE;
> -  SET_BUF_ZV_BOTH (current_buffer, Z, Z_BYTE);
> +  Fset (Qoutermost_narrowing, Qnil);

I must say, I still haven't understood how `Qoutermost_narrowing`
and `Qnarrowing_locks` work/interact, so I suggest you document their
contents more fully (either in their docstring or in comments next to
it).

> +  if (NILP (Vnarrowing_locks))
> +    {
> +      if (BEG != BEGV || Z != ZV)
> +     current_buffer->clip_changed = 1;
> +      BEGV = BEG;
> +      BEGV_BYTE = BEG_BYTE;
> +      SET_BUF_ZV_BOTH (current_buffer, Z, Z_BYTE);
> +    }
> +  else
> +    {
> +      ptrdiff_t begv = XFIXNUM (Fcar (Fcdr (Fcar (Vnarrowing_locks))));
> +      ptrdiff_t zv = XFIXNUM (Fcdr (Fcdr (Fcar (Vnarrowing_locks))));

Since you use XFIXNUM (i.e. you assume that `Vnarrowing_locks` has
a particular shape), you can also use XCAR/XCDR here.

> +      if (begv != BEGV || zv != ZV)
> +     current_buffer->clip_changed = 1;
> +      SET_BUF_BEGV (current_buffer, begv);
> +      SET_BUF_ZV (current_buffer, zv);
> +      if (EQ (Fcar (Fcar (Vnarrowing_locks)), Qoutermost_narrowing))
> +     Fset (Qnarrowing_locks, Qnil);

I think you can `Vnarrowing_locks = Qnil` here.  But I must admit
I don't really understand why you're messing with narrowing_locks here.

> +DEFUN ("narrow-to-region", Fnarrow_to_region, Snarrow_to_region, 2, 2, "r",
> +       doc: /* Restrict editing in this buffer to the current region.
> +The rest of the text becomes temporarily invisible and untouchable
> +but is not deleted; if you save the buffer in a file, the invisible
> +text is included in the file.  \\[widen] makes all visible again.
> +See also `save-restriction'.
>  
> +When calling from Lisp, pass two arguments START and END:
> +positions (integers or markers) bounding the text that should
> +remain visible.
>  
> +When restrictions have been locked with `narrowing-lock', which see,
> +`narrow-to-region' can be used only within the limits of the
> +restrictions that were current when `narrowing-lock' was called.  If
> +the START or END arguments are outside these limits, the corresponding
> +limit of the locked restriction is used instead of the argument.  */)
> +  (Lisp_Object start, Lisp_Object end)
>  {
>    EMACS_INT s = fix_position (start), e = fix_position (end);
>  
> @@ -2731,35 +2743,29 @@ narrow_to_region_internal (Lisp_Object start, 
> Lisp_Object end, bool lock)
>        EMACS_INT tem = s; s = e; e = tem;
>      }
>  
> +  if (!(BEG <= s && s <= e && e <= Z))
> +    args_out_of_range (start, end);
>  
> +  if (! NILP (Vnarrowing_locks))
>      {
> +      ptrdiff_t begv = XFIXNUM (Fcar (Fcdr (Fcar (Vnarrowing_locks))));
> +      ptrdiff_t zv = XFIXNUM (Fcdr (Fcdr (Fcar (Vnarrowing_locks))));
> +      if (s < begv) s = begv;
> +      if (s > zv) s = zv;
> +      if (e < begv) e = begv;
> +      if (e > zv) e = zv;
> +    }

Maybe the `args_out_of_range` error should be signaled based on
`begv/zv` (when inside a lock) rather than `BEG/Z` (i.e. if trying to
`narrow-to-region` beyond the bounds of the current lock we'd get an
error rather than silently using the current lock's limit).

BTW, using XFIXNUM is wrong because the bounds need to be markers, to
account for possible buffer modifications (you could argue it's only
needed for the ZV part and not for BEGV because you can go before the
locked narrowing without undoing this lock, but it's still possible to
insert/remove text before the beginning of the current locked narrowing
by using an indirect buffer).

> +  Fset (Qoutermost_narrowing,
> +     Fcons (Fcons (Qoutermost_narrowing,
> +                   Fcons (make_fixnum (BEGV), make_fixnum (ZV))),
> +            Qnil));

This needs explaining.

> +DEFUN ("narrowing-lock", Fnarrowing_lock, Snarrowing_lock, 1, 1, 0,
[...]
> +  (Lisp_Object tag)
> +{
> +  if (NILP (Vnarrowing_locks))
> +    Fset (Qnarrowing_locks, Voutermost_narrowing);

This also.

> +  Fset (Qnarrowing_locks,
> +     Fcons (Fcons (tag, Fcons (make_fixnum (BEGV), make_fixnum (ZV))),
> +            Vnarrowing_locks));
> +  return Qnil;
> +}

As mentioned, these saved positions need to be markers.

> +DEFUN ("narrowing-unlock", Fnarrowing_unlock, Snarrowing_unlock, 1, 1, 0,
> +       doc: /* Unlock a narrowing locked with (narrowing-lock TAG).
> +
> +Unlocking restrictions locked with `narrowing-lock' should be used
> +sparingly, after carefully considering the reasons why restrictions
> +were locked.  Restrictions are typically locked around portions of
> +code that would become too slow, and make Emacs unresponsive, if they
> +were executed in a large buffer.  For example, restrictions are locked
> +by Emacs around low-level hooks such as `fontification-functions' or
> +`post-command-hook'.  */)
> +  (Lisp_Object tag)
> +{
> +  if (EQ (Fcar (Fcar (Vnarrowing_locks)), tag))
> +    Fset (Qnarrowing_locks, Fcdr (Vnarrowing_locks));
> +  return Qnil;
> +}

This allows removing a lock once and for all, but for example in
`nlinum` I'd like to just locally/temporarily escape the lock installed
by the long-lines optimization, but restore the lock when I'm done.
I don't think `narrowing-unlock` lets me do that yet currently.
It would if `save-restriction` were to save&restore
`narrowing_locks`, tho, so I could do:

    (save-restriction
      (narrowing-unlock 'long-lines)
      (narrowing-unlock 'mmm)
      (widen)
      ... compute current line number)

Also, I can imagine situations where one might like to escape from a set
of locks (e.g. the ones installed by long-lines and by mmm-mode) without
knowing in which order they may have been installed, so maybe when
overriding a lock we should be ale to provide a set (list) of tags
rather than a single tag.

Furthermore, I suspect that all such temporary override of the lock are
combined with `widen`, so maybe instead of a new function, we should
just add a `tags` arg to `widen`, so nlinum would do:

    (save-restriction
      (widen '(long-lines mmm))
      ... compute current line number)

> +static void
> +unwind_narrow_to_region_locked (Lisp_Object tag)
> +{
> +  Fnarrowing_unlock (tag);
> +  Fwiden ();
> +}
>
> +void
> +narrow_to_region_locked (Lisp_Object begv, Lisp_Object zv, Lisp_Object tag)
>  {
> +  Fnarrow_to_region (begv, zv);
> +  Fnarrowing_lock (tag);
> +  record_unwind_protect (restore_point_unwind, Fpoint_marker ());
> +  record_unwind_protect (unwind_narrow_to_region_locked, tag);
>  }

This seems to duplicate the code of `with-locked-narrowing` (just
written in C this time).

> +  DEFSYM (Qnarrowing_locks, "narrowing-locks");
> +  DEFVAR_LISP ("narrowing-locks", Vnarrowing_locks,
> +            doc: /* List of narrowing locks in the current buffer.  Internal 
> use only.  */);
> +  Vnarrowing_locks = Qnil;
> +  Fmake_variable_buffer_local (Qnarrowing_locks);
> +  Funintern (Qnarrowing_locks, Qnil);

At the very least for debugging purposes it would be very convenient to
have access to this var from ELisp, so I wouldn't unintern it (but I'd
give it a double hyphen in the name).

> +  DEFSYM (Qoutermost_narrowing, "outermost-narrowing");
> +  DEFVAR_LISP ("outermost-narrowing", Voutermost_narrowing,
> +            doc: /* Outermost narrowing bounds, if any.  Internal use only.  
> */);
> +  Voutermost_narrowing = Qnil;
> +  Fmake_variable_buffer_local (Qoutermost_narrowing);
> +  Funintern (Qoutermost_narrowing, Qnil);

As mentioned, I don't understand what this is about, so I don't have
good comments to make about the rest of the code which touches this.


        Stefan