[Bug]: Clicking on header to sort loses highlighting

Confirmation

[X] I have checked the documentation (README, Wiki, docstrings, etc)
[ ] I am checking these without reading them.
[X] I have searched previous issues to see if my question is a duplicate.

Elpaca Version

Elpaca 4022c34 HEAD -> master, origin/master, origin/HEAD installer: 0.7 emacs-version: GNU Emacs 29.3 (build 1, aarch64-redhat-linux-gnu, GTK+ Version 3.24.41, cairo version 1.18.0) of 2024-03-27 git --version: git version 2.44.0

Operating System

Linux (Fedora)

Description

If I click on a header line to sort by some column, all highlighting including indication of the marks is lost. This is because tabulated-list-printer is not called again so the face in elpaca-ui--apply-faces don't get applied.

It seems to be fixed by,

diff --git a/elpaca-ui.el b/elpaca-ui.el
index 0696d34..a035374 100644
--- a/elpaca-ui.el
+++ b/elpaca-ui.el
@@ -390,9 +390,7 @@ ID and COLS mandatory args to fulfill `tabulated-list-printer' API."
                             (face   (or (plist-get props :face) 'default))
                             (prefix (or (plist-get props :prefix) "*")))
                        (propertize name 'display (propertize (concat prefix " " name) 'face face))))))
-      (progn
-        (setq cols (copy-tree cols t))
-        (setf (aref cols 0) result))
+      (setf (aref cols 0) result)
     (remove-text-properties 0 (length (aref cols 0)) '(display) (aref cols 0)))
   (tabulated-list-print-entry id cols))

which mutates the entry by applying the face directly. I am not sure it won't have any undesirable consequences and the code seems to be taking care to not mutate so I assume there is a reason. But in light testing everthing seems to behave.

If mutation is undesirable I think another avenue can be a custom jit-lock-function.

If I click on a header line to sort by some column, all highlighting including indication of the marks is lost. This is because tabulated-list-printer is not called again so the face in elpaca-ui--apply-faces don't get applied.

Reverting the buffer (bound to g r by default) should re-apply those faces.

It seems to be fixed by, [PATCH OMITTED] which mutates the entry by applying the face directly. I am not sure it won't have any undesirable consequences and the code seems to be taking care to not mutate so I assume there is a reason. But in light testing everthing seems to behave.

Yes, we want to avoid mutating menu items, because they can be re-used again. That can lead to stale text properties. Unfortunately tabulated-list-mode doesn't have a good hook for this case, so I've added some more advice to ensure we're printing the entries after sorting. It's a bit of a kludge, but it should work for now.

Try the following test case:

(elpaca-test
  :ref "fix/colsort"
  :interactive t
  :early-init (setq elpaca-menu-functions nil)
  :init
  (elpaca (doct :host github :repo "progfolio/doct" :wait t))
  (with-current-buffer (elpaca-log "#unique")
    (goto-char (point-min))
    (re-search-forward "doct")
    (elpaca-ui-mark-rebuild)
    (goto-char (point-min))))

It should install Elpaca and doct, then open a log buffer, and mark doct for re-installation. You can then sort the column and the mark should persist. Let me know if that works for you and I'll merge the change.

If mutation is undesirable I think another avenue can be a custom jit-lock-function.

I'd have to look into that more. I've never written a jit-lock-function. Added to my notes. Thanks.

Reverting the buffer (bound to g r by default) should re-apply those faces.

Yes, it does, thanks.

Yes, we want to avoid mutating menu items, because they can be re-used again. That can lead to stale text properties. Unfortunately tabulated-list-mode doesn't have a good hook for this case, so I've added some more advice to ensure we're printing the entries after sorting. It's a bit of a kludge, but it should work for now.

Entries that are reused are in elpaca--menu-cache, right? If so I think copying can be moved to a different place i.e. when the tabulated-list-entries are constructed. Then these can be mutated without affecting the global cache.

Try the following test case:
(elpaca-test
  :ref "fix/colsort"
  :interactive t
  :early-init (setq elpaca-menu-functions nil)
  :init
  (elpaca (doct :host github :repo "progfolio/doct" :wait t))
  (with-current-buffer (elpaca-log "#unique")
    (goto-char (point-min))
    (re-search-forward "doct")
    (elpaca-ui-mark-rebuild)
    (goto-char (point-min))))
It should install Elpaca and doct, then open a log buffer, and mark doct for re-installation. You can then sort the column and the mark should persist. Let me know if that works for you and I'll merge the change.

I tested and now marks work as expected. Thanks a lot!

I'd have to look into that more. I've never written a jit-lock-function. Added to my notes. Thanks.

I have written a couple so I can try writing one for elpaca too. Might need some time to familiarize myself with the code though.

A basic jit lock function that works for applying marks is,

(defun elpaca-ui--jit-apply-faces (beg end)
  "Apply faces to all entries between BEG and END."
  (save-excursion
    (with-silent-modifications
      (setq end (progn (goto-char end) (line-end-position)))
      (setq beg (progn (goto-char beg) (line-beginning-position)))
      (while (< (point) end)
        (elpaca-ui--apply-face)
        (forward-line 1))
      `(jit-lock-bounds ,beg . ,end))))

It doesn't work for applying the faces showing the status of a package. The elpaca-ui--cache is only let bound so is not useful for jit-lock and I am not sure how this should interact with the processing of the queues which will change the status.

A basic jit lock function that works for applying marks is,
(defun elpaca-ui--jit-apply-faces (beg end)
  "Apply faces to all entries between BEG and END."
  (save-excursion
    (with-silent-modifications
      (setq end (progn (goto-char end) (line-end-position)))
      (setq beg (progn (goto-char beg) (line-beginning-position)))
      (while (< (point) end)
        (elpaca-ui--apply-face)
        (forward-line 1))
      `(jit-lock-bounds ,beg . ,end))))
It doesn't work for applying the faces showing the status of a package. The elpaca-ui--cache is only let bound so is not useful for jit-lock and I am not sure how this should interact with the processing of the queues which will change the status.

Cool! Thank you. I took a quick peek at the font-lock/jit machinery. Am I correct that that function would need to be added to jit-lock-functions in elpaca-ui-mode buffers for it to take effect?

I tried adding it and patching elpaca-ui--apply-face to apply only to the current line:

(defun elpaca-ui--apply-face ()
  "Apply face to current entry id."
  (when-let ((entry (tabulated-list-get-entry))
             (name  (aref entry 0))
             (id    (intern name)))
    (let* ((marked (cl-find id elpaca-ui--marked-packages :key #'car))
           (props  (nthcdr 2 marked))
           (face   (or (plist-get props :face) 'default))
           (prefix (or (plist-get props :prefix) "*"))
           (parg   (plist-get props :prefix-arg))
           (mark   (propertize (concat prefix (when parg "+") " " name) 'face face))
           (beg (line-beginning-position))
           (e (elpaca-get id)))
      (let ((inhibit-read-only t))
        (when e
          (put-text-property beg (+ beg (length name)) 'face
                             (elpaca-alist-get (elpaca--status e)
                                               elpaca-status-faces '(:weight bold))))
        (if marked (put-text-property beg (+ beg (length name)) 'display mark)
          (remove-text-properties beg (line-end-position) '(display)))))))

With that in place we can get marks and the status, but only once a line has been operated on. The initial view does not apply any of the font-locking. I tried manually calling font-lock-flush and jit-lock-refontify, but that didn't seem to do the trick. Only marking/unmarking applies the faces. If you have any insight here I'd be happy to replace what I wrote with your jit-locking function.

For now that version of elpaca--ui-apply-face completely ignores the cache because I think we could remove it considering jit-lock only operates on the visible portion of the buffer (assuming I'm correct there as well). Thoughts?

A basic jit lock function that works for applying marks is,
(defun elpaca-ui--jit-apply-faces (beg end)
  "Apply faces to all entries between BEG and END."
  (save-excursion
    (with-silent-modifications
      (setq end (progn (goto-char end) (line-end-position)))
      (setq beg (progn (goto-char beg) (line-beginning-position)))
      (while (< (point) end)
        (elpaca-ui--apply-face)
        (forward-line 1))
      `(jit-lock-bounds ,beg . ,end))))
It doesn't work for applying the faces showing the status of a package. The elpaca-ui--cache is only let bound so is not useful for jit-lock and I am not sure how this should interact with the processing of the queues which will change the status.
Cool! Thank you. I took a quick peek at the font-lock/jit machinery. Am I correct that that function would need to be added to jit-lock-functions in elpaca-ui-mode buffers for it to take effect?

It is better to add it through jit-lock-register since that will turn on the jit-lock-mode in the buffer.

I tried adding it and patching elpaca-ui--apply-face to apply only to the current line:
(defun elpaca-ui--apply-face ()
  "Apply face to current entry id."
  (when-let ((entry (tabulated-list-get-entry))
             (name  (aref entry 0))
             (id    (intern name)))
    (let* ((marked (cl-find id elpaca-ui--marked-packages :key #'car))
           (props  (nthcdr 2 marked))
           (face   (or (plist-get props :face) 'default))
           (prefix (or (plist-get props :prefix) "*"))
           (parg   (plist-get props :prefix-arg))
           (mark   (propertize (concat prefix (when parg "+") " " name) 'face face))
           (beg (line-beginning-position))
           (e (elpaca-get id)))
      (let ((inhibit-read-only t))
        (when e
          (put-text-property beg (+ beg (length name)) 'face
                             (elpaca-alist-get (elpaca--status e)
                                               elpaca-status-faces '(:weight bold))))
        (if marked (put-text-property beg (+ beg (length name)) 'display mark)
          (remove-text-properties beg (line-end-position) '(display)))))))
With that in place we can get marks and the status, but only once a line has been operated on. The initial view does not apply any of the font-locking. I tried manually calling font-lock-flush and jit-lock-refontify, but that didn't seem to do the trick. Only marking/unmarking applies the faces. If you have any insight here I'd be happy to replace what I wrote with your jit-locking function.

I tried this and it seems to work for me! I opened a new session of Emacs, did M-x load-library elpaca-ui RET, evaluated your definition of elpaca-ui--apply-face, and the following definition of elpaca-ui-mode and elpaca-ui--jit-apply,

(define-derived-mode elpaca-ui-mode tabulated-list-mode "elpaca-ui"
  "Major mode to manage packages."
  (add-hook 'minibuffer-setup-hook 'elpaca-ui--minibuffer-setup)
  (elpaca-ui-live-update-mode 1)
  (jit-lock-register #'elpaca-ui--jit-apply-faces)
  (advice-add #'tabulated-list-print :after #'elpaca-ui--print-appender)
  (hl-line-mode))

(defun elpaca-ui--jit-apply-faces (beg end)
  "Apply faces to all entries between BEG and END."
  (save-excursion
    (with-silent-modifications
      (setq end (progn (goto-char end) (line-end-position)))
      (setq beg (progn (goto-char beg) (line-beginning-position)))
      (while (< (point) end)
        (elpaca-ui--apply-face)
        (forward-line 1))
      `(jit-lock-bounds ,beg . ,end))))

Then I did M-x elpaca-manager and M-x elpaca-ui-search-installed and the faces for the status are applied. Marks also get applied but they mess up the alignment of columns. I will look into that and can do a pr once I have it under control if that seems like a good plan to you.

I think font-lock is a jit-lock client so calling font-lock-flush shouldn't do anything. jit-lock-refontify should work and I am surprised it doesn't.

For now that version of elpaca--ui-apply-face completely ignores the cache because I think we could remove it considering jit-lock only operates on the visible portion of the buffer (assuming I'm correct there as well). Thoughts?

I think they can be removed, the lazy approach of jit-lock should be fast enough. jit-lock operates on the visible portion unless the jit-lock-stealth-* variables have been customized.

It is better to add it through jit-lock-register since that will turn on the jit-lock-mode in the buffer.

Ah, that makes sense. As do your other points about font-lock-flush.

I tried this and it seems to work for me! I opened a new session of Emacs, did M-x load-library elpaca-ui RET, evaluated your definition of elpaca-ui--apply-face, and the following definition of elpaca-ui-mode and elpaca-ui--jit-apply, .... Then I did M-x elpaca-manager and M-x elpaca-ui-search-installed and the faces for the status are applied. Marks also get applied but they mess up the alignment of columns.

The column spacing was due to the way elpaca-ui--apply-face indiscriminately removed the display property to the end of line when there is no mark. tabulated-list-mode controls the display of column spacing with a display text property as well, which was removed. I've refactored elpaca-ui--apply-face to account for this and a few other aspects (retaining the status face of previous log entries, removing unnecessary caches, refontifying after unmarking/marking, etc).

I will look into that and can do a pr once I have it under control if that seems like a good plan to you.

I rebased the "fix/colsort" branch to reflect the above changes. I think that everything should be working as intended now. Give it a try and let me know what you think:

(elpaca-test :ref "fix/colsort" :interactive t)

Of course, I'll list you as co-author (unless you prefer not) on the commit once we've tested and decide it's merge-ready. Thanks again for your help on this. You've helped me learn a bit about jit-lock.el.

I rebased the "fix/colsort" branch to reflect the above changes. I think that everything should be working as intended now. Give it a try and let me know what you think:
(elpaca-test :ref "fix/colsort" :interactive t)

I tested it and everything looks good. I also took a peek at the code and the only comment I have is that in elpaca-ui-defmark, beg and end should be passed to the jit-lock-refontify so that it doesn't need to refontify the whole buffer.

Of course, I'll list you as co-author (unless you prefer not) on the commit once we've tested and decide it's merge-ready. Thanks again for your help on this. You've helped me learn a bit about jit-lock.el.

Thanks a lot for fixing this and I am happy to learn a bit about elpaca internals. About co-author, do what is convenient for you.

I tested it and everything looks good.

Great. There was one other small issue I took care of regarding showing hidden rows when the table is longer than elpaca-ui-row-limit.

I also took a peek at the code and the only comment I have is that in elpaca-ui-defmark, beg and end should be passed to the jit-lock-refontify so that it doesn't need to refontify the whole buffer.

Taken care of. Those functions are currently for interactive use in an elpaca-ui-mode buffer, so I passed (window-start) and (window-end) to limit to the current viewable portion of the buffer.

About co-author, do what is convenient for you.

I've credited you in the commit message and merged this to master.

Thanks a lot for fixing this and I am happy to learn a bit about elpaca internals.

Thank you! If you have any questions or ideas, or run into any other issues, I'm always happy to chat about it.

Taken care of. Those functions are currently for interactive use in an elpaca-ui-mode buffer, so I passed (window-start) and (window-end) to limit to the current viewable portion of the buffer.

I think this will be incorrect if the region selected is bigger than the window. This should work,

(defmacro elpaca-ui-defmark (name test)
  "Define a marking command with NAME and TEST."
  (declare (indent 1) (debug t))
  `(defun ,(intern (format "elpaca-ui-mark-%s"
                           (replace-regexp-in-string "^elpaca-" "" (symbol-name name))))
       () ,(format "Mark package at point for `%s'." name)
       (interactive)
       (if (not (use-region-p))
           (progn
             (elpaca-ui--toggle-mark ,test ',name)
             (jit-lock-refontify (line-beginning-position) (line-end-position)))
         (let ((end (region-end))
               (beg (region-beginning)))
           (save-restriction
             (goto-char beg)
             (while (not (>= (point) end))
               (condition-case _
                   (elpaca-ui--toggle-mark ,test ',name)
                 ((error) (forward-line))))
             (deactivate-mark)
             (jit-lock-refontify beg end))))))

While I think elpaca-ui-mark doesn't handle the region? it should probably be something like this:

(defun elpaca-ui-unmark ()
  "Unmark current package.
If region is active unmark all packages in region."
  (interactive)
  (let ((beg (if (use-region-p) (region-beginning) (line-beginning-position)))
        (end (if (use-region-p) (region-end) (line-end-position))))
    (goto-char beg)
    (while (< (point) end)
      (elpaca-ui--unmark (elpaca-ui-current-package))
      (forward-line))
    (when (use-region-p) (deactivate-mark))
    (jit-lock-refontify (window-start) (window-end))))

Thank you! If you have any questions or ideas, or run into any other issues, I'm always happy to chat about it.

Thanks a lot!

I think this will be incorrect if the region selected is bigger than the window. This should work,

Ah, I was under the impression that scrolling would cause jit-lock to kick in.

For the case where region is inactive, we also want the whole window because there may be multiple log-lines which pertain to the same package, which should all show as marked.

While I think elpaca-ui-mark doesn't handle the region?

I've refactored the marking to remove some duplication, allow unmarking regions, and hopefully get all cases of jit-locking correct here:

9a4d81b75e93369698916fb74c4f9dfbe5ad0153

Ah, I was under the impression that scrolling would cause jit-lock to kick in.

jit-lock fontifies lazily but once fontification is done, it puts a non-nil fontified property and doesn't do anything unless that property is removed. If buffer is modified, it takes care of removing this property. For marks nothing changes in the buffer since they are display properties. So to cause refontification, fontifified property has to be removed manually which is what jit-lock-refontify does.

For the case where region is inactive, we also want the whole window because there may be multiple log-lines which pertain to the same package, which should all show as marked.

I didn't realize this and I think your initial approach of just calling (jit-lock-refontify) was the correct one since these entries can be arbitrarily separated. I though I was suggesting a minor performance optimization but I think it was wrong. Sorry about that.

I've refactored the marking to remove some duplication, allow unmarking regions, and hopefully get all cases of jit-locking correct here:

9a4d81b

Thanks a lot but as above, I think it is best to request the refontification of the whole buffer.

progfolio / elpaca