dbordak / telephone-line

A new implementation of Powerline for Emacs
GNU General Public License v3.0
550 stars 51 forks source link

[bug] `airline-position-segment` not displaying `mode-line-position`, important for some major modes #143

Open leonardo-cavegliacurtil opened 4 months ago

leonardo-cavegliacurtil commented 4 months ago

Some major modes, such as PDFView, use mode-line-position to communicate the buffer position aside from lines and columns. When using telephone-line-airline-position-segment, such values are not displayed.

The following screenshot depicts telephone-line-airline-position-segment beside telephone-line-position-segment, which is properly using mode-line-position.

Screenshot_20240327_112420


My proposed possible solutions would be:


Related code from telephone-line-segments.el follows.

(telephone-line-defsegment* telephone-line-position-segment ()
  (telephone-line-raw
   (if (eq major-mode 'paradox-menu-mode)
       ;;Paradox fills this with position info.
       mode-line-front-space
     mode-line-position) t))

(telephone-line-defsegment* telephone-line-airline-position-segment (&optional lines columns)
  "Position segment imitating vim-airline's appearance. Optional args set padding on lines/columns."
  (let* ((l (number-to-string (if lines lines 4)))
         (c (number-to-string (if columns columns 3))))
    (if (eq major-mode 'paradox-menu-mode)
        (telephone-line-raw mode-line-front-space t)
      `((-3 "%p") ,(concat " %" l "l"
                           ":%" c (if (bound-and-true-p column-number-indicator-zero-based) "c" "C"))))))
leonardo-cavegliacurtil commented 4 months ago

Here is my proposed version: if it matches the regex /L\d+\s*$/, it uses the previous "airline" code; otherwise, it uses mode-line-position.

Ideally, I'd use a smarted regex like /^\s*\w+\s*L\d+\s*$/, which should match only the patterns seen in text buffers (e.g. Top L40 or 5% L40), but I can't get them to work properly.

Tested and apparently working, both with text buffers and with PDFView buffers. Can someone check that this code isn't garbage? I have no idea what I'm doing with elisp. :)

(telephone-line-defsegment* telephone-line-airline-position-segment (&optional lines columns)
  "Position segment imitating vim-airline's appearance. Optional args set padding on lines/columns."
  (let* ((l (number-to-string (if lines lines 4)))
         (c (number-to-string (if columns columns 3))))
    (if (eq major-mode 'paradox-menu-mode)
        (telephone-line-raw mode-line-front-space t)
      (if (string-match-p "L[0-9]+\s*$" (telephone-line-raw mode-line-position t))
      `((-3 "%p") ,(concat " %" l "l"
                   ":%" c (if (bound-and-true-p column-number-indicator-zero-based) "c" "C")))
    mode-line-position)
)))
leonardo-cavegliacurtil commented 4 months ago

I got it to work with this regex /^[\w\s%]*L\d+\s*$/, which should match all standard positions in text buffers.

(telephone-line-defsegment* telephone-line-airline-position-segment (&optional lines columns)
  "Position segment imitating vim-airline's appearance. Optional args set padding on lines/columns."
  (let* ((l (number-to-string (if lines lines 4)))
         (c (number-to-string (if columns columns 3))))
    (if (eq major-mode 'paradox-menu-mode)
        (telephone-line-raw mode-line-front-space t)
      (if (string-match-p "^[[:alnum:]\s%]*L[0-9]+\s*$" (telephone-line-raw mode-line-position t))
      `((-3 "%p") ,(concat " %" l "l"
                   ":%" c (if (bound-and-true-p column-number-indicator-zero-based) "c" "C")))
    mode-line-position)
)))

I'm not sure if it's a clean solution to match against (telephone-line-raw mode-line-position t). But in case the code is sound, I have a branch ready for a pull.

dbordak commented 3 months ago

This sounds correct, but I don't remember best practices on elisp regex...

leonardo-cavegliacurtil commented 2 months ago

Any updates on this? What is exactly that is missing?

dbordak commented 2 months ago

I think selecting on major mode would be a cleaner solution, even if it means that it needs to be expanded in the future. I can't tell conclusively all the text formats that various modes use (i.e. it's possible that some of them would match the given regex, somehow), but on the other hand, modes which overwrite this value are probably going to be relatively few (likely only the ones which aren't rendering text, like pdfview).

EDIT: also another argument for, there's already a major-mode check in this function. Also, it'd be immediately obvious to anyone looking at the function why this check is necessary, vs the regex probably needing a line comment at least.

Something like this?

(telephone-line-defsegment* telephone-line-airline-position-segment (&optional lines columns)
  "Position segment imitating vim-airline's appearance. Optional args set padding on lines/columns."
  (let* ((l (number-to-string (if lines lines 4)))
         (c (number-to-string (if columns columns 3))))
    (cond
      ((eq major-mode 'paradox-menu-mode) (telephone-line-raw mode-line-front-space t))
      ((eq major-mode 'pdf-view-mode) (telephone-line-raw mode-line-position t))
      (t `((-3 "%p") ,(concat " %" l "l"
                              ":%" c (if (bound-and-true-p column-number-indicator-zero-based) "c" "C")))))))
leonardo-cavegliacurtil commented 2 months ago

That works just fine, and yeah, the regex does seem like a bit of a workaround. The reason why I originally said that a major mode check seems "less clean" is that it's a hardcoded switch, that will inevitably end up growing. I also found another mode that requires an exception, scad-preview-mode, from the scad-mode package. I tested your code, added the following line, and it works as expected.

      ((eq major-mode 'scad-preview-mode) (telephone-line-raw mode-line-position t))

Screenshot_20240507_184715


I'd propose to expose a customizable list of major modes, defaulting to those we know so far. While not necessary, it would be a more permanent solution; although, at that point, it would also call for many other customizable lists and strings throughout the package. I don't want to perturb the project too much, so it's up to you on whether that's worth considering.

That aside, the issue is to be considered solved, when this is patch is on MELPA.