regression in mode-line-percent-position detection

[bymoz089]

With commit https://github.com/jdtsmith/mlscroll/commit/ee6d1f801f7defc78553ec79c461bf02e00e20d2 there is a regression in detecting the mode-line-percent-position.

Following code fails, when starting a fresh Emacs instance, when mlscroll.el is byte compiled (or native compiled). It does not fail, if mlscroll.el is plain elisp. The reason is unknown to me, I located this issue through print debugging the code.

       (catch 'find-in-tree ; search inside car of mode-line-position
         (cl-subst t t (car mode-line-position)
               :key (lambda (el)
                  (when (eq el 'mode-line-percent-position)
                (throw 'find-in-tree t))))
         nil)

I propose to substitute above code through a simpler one:

(cl-find 'mode-line-percent-position (car mode-line-position))

this one does not search recursive in sublists, but searching in sublists is imho not necessary.

[jdtsmith]

It sadly is necessary for Emacs 29; see #13. I think cl-subst is unstable to non-local exit, depending on whether compiled or interpreted. Too bad there is no cl-find-tree.

I've pushed a simpler recursive find method to branch mode-line-alter2 and it seems to work for me (28.2). Can you please test? @qingshuizheng can you also test on Emacs 29?

[qingshuizheng]

It sadly is necessary for Emacs 29; see #13. I think cl-subst is unstable to non-local exit, depending on whether compiled or interpreted. Too bad there is no cl-find-tree.

I've pushed a simpler recursive find method to branch mode-line-alter2 and it seems to work for me (28.2). Can you please test? @qingshuizheng can you also test on Emacs 29?

sure, will get back to you later.

And actually I'm now on 30, but since emacs 29 is just cut two days ago, there may not be too much difference, between 29/30.

[qingshuizheng]

@jdtsmith After some test, it works fine on emacs 30.

env:

1. both plain elisp and byte-compiled by straight.el, i dont have native-comp,
2. non Emacs -Q test
3. env: GNU Emacs 30.0.50 (build 1, x86_64-apple-darwin20.6.0, NS appkit-2022.60 Version 11.6 (Build 20G165)) of 2022-12-01

test code:

(use-package mlscroll
  :straight (:host github :repo "jdtsmith/mlscroll"
             :branch "mode-line-alter2")    ;;; <-- test branch
  :config
  (setopt
   mlscroll-right-align nil ;;; <--- both t/nil tested
   mlscroll-alter-percent-position t  ;;; <--- both t/nil/'replace tested
   mlscroll-in-color "red"
   mlscroll-out-color "green"
   mlscroll-width-chars 12
   mlscroll-border nil
   mlscroll-shortfun-min-width nil)

  
  ;; NOTE: (setq mode-line-compact t) will swallow mlscroll, use code:
  ;; SRC 2022-12-02: https://github.com/jdtsmith/mlscroll#other-tips
  (setq-default
   mode-line-format
   (cl-nsubst-if
    " "
    (lambda (x) (and (stringp x) (string-blank-p x) (> (length x) 1)))
        (remove 'mode-line-mule-info mode-line-format))))

There's a caveat there though, topic unrelated, below two snippets behaves the same way, not sure whether expected or not. If you want, i can open another issue:

   (setopt
   mlscroll-right-align nil
   mlscroll-alter-percent-position t)

   (setopt
   mlscroll-right-align nil
   mlscroll-alter-percent-position 'replace)

[jdtsmith]

Thanks for the tests. You can't have both 'replace and 'right-align set, since right-align means "at the right side of the mode line". You should get a message to this effect if you set both.

But specifying neither is also a problem: where should the scrollbar go if not at right or replacing percent-position? I guess this could mean "at the end, but not right-aligned" (your green arrow). Not sure who would want that, since which-function etc. will move it around a lot. If you think this is valuable, happy to look at a PR.

[bymoz089]

@jdtsmith your fix works, thank you.

Anyway, how about this search code?:

       (cl-block found 
         (cl-labels ((in-tree-p (tree val)
                    (cond ((atom tree) (when (eq val tree)
                             (cl-return-from found t)))
                      (t (in-tree-p (car tree) val)
                         (in-tree-p (cdr tree) val)))))
           (in-tree-p (car mode-line-position) 'mode-line-percent-position)))

[qingshuizheng]

Ah you are right. And I only use mlscroll to replace the percent position. All set. Thanks for the great work!! On Dec 3, 2022, 09:26 -0500, JD Smith @.***>, wrote:

Thanks for the tests. You can't have both 'replace and 'right-align set, since right-align means "at the right side of the mode line". You should get a message to this effect if you set both. But specifying neither is also a problem: where should the scrollbar go if not at right or replacing percent-position? I guess this could mean "at the end, but not right-aligned" (your green arrow). Not sure who would want that, since which-function etc. will move it around a lot. If you think this is valuable, happy to look at a PR. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[jdtsmith]

Thanks @bymoz089. Your test also looks interesting, but it's practically the same: cl-block/cl-return-from simply expands to catch/throw. Usually I try to avoid macros unless they are a line or conceptual savings. Mine also avoids searching the implicit nil cdr at the end of every list, which, if you wanted to actually find a nil car, would be important (not that we do here, but future proofing). I do like using atom so simplified with it, thanks.

[bymoz089]

but it's practically the same:

Yeah, because cl-labels in elisp does not implicitly create a block, like labels in Common Lisp does.
But I posted it anyway, to show a slightly different approach, nice that you had a look even though the issue was fixed. :)