GNU bug report logs - #61726
[PATCH] Eglot: Support positionEncoding capability

Previous Next

Full log

Message #83 received at 61726 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61726 <at> debbugs.gnu.org, Augusto Stoffel <arstoffel <at> gmail.com>
Subject: Re: bug#61726: [PATCH] Eglot: Support positionEncoding capability
Date: Fri, 24 Feb 2023 11:43:18 +0000

On Fri, Feb 24, 2023 at 11:27 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > From: Augusto Stoffel <arstoffel <at> gmail.com>
> > Cc: joaotavora <at> gmail.com,  61726 <at> debbugs.gnu.org
> > Date: Fri, 24 Feb 2023 10:15:48 +0100
> >
> > >> -(defun eglot-current-column () (- (point) (line-beginning-position)))
> > >> +(defun eglot-current-column ()
> > >> +  "Calculate current column, counting Unicode codepoints."
> > >> +  (- (point) (line-beginning-position)))
> > >
> > > Can we please take this opportunity to get rid of the confusing
> > > "column" terminology?  As became evident from this discussion, we are
> > > not talking columns here, we are talking offsets in characters from
> > > BOL.  So something like "pos" or "linepos" or "line-offset" should be
> > > better.
> > >
> > > João, are you okay with such a sweeping change in all of eglot.el?
> >
> > I like linepos, if João is fine with not making the absolute minimal
> > amount of changes to the code.
>
> João?

eglot-current-column is a user-visible function.  would need
obsoletion aliases.  Are you sure it isn't better just to add some
clarifying comments?  I fear for my ability to recall details about this
code with such a sweeping rename, accuracy-improving as it may be.
So it's a balancing act, your call.

>
> > >> +              (tab-width 1)
> > >                   ^^^^^^^^^^^
> > > This last part shouldn't be necessary: we should move by characters,
> > > not by columns.  Why is it necessary?
> >
> > Maybe João can clarify, but I'm pretty sure this is there to support the
> > UTF-16 way of counting offsets, so this ideally should move to
> > eglot-move-to-lsp-abiding-column.
>
> Then perhaps the UTF-16 way of counting offsets should be changed as
> well.

I've vc-region-history'ed it to:

commit 2cf7905887f2137869f44c3383a55636e38b4b81
Author: Michal Krzywkowski <k.michal <at> zoho.com>
Date:   Mon Nov 19 21:22:14 2018 +0100

    Treat tab characters as 1 column wide in position conversion functions

    Fixes https://github.com/joaotavora/eglot/issues/158.

    * eglot.el (eglot--pos-to-lsp-position): Call
      eglot-current-column-function with tab-width bound to 1.
    (eglot--lsp-position-to-point): Call eglot-move-to-column-function
    with tab-width bound to 1.

Following the link, I read this there:

  "This is because move-to-column and current-column count each tab
character as
   tab-width chars."

And, as far as I remember, at the time we were indeed using move-to-column and
current-column.  But now we aren't anymore.

So maybe, just maybe, this can be removed.  And the full test suite must
run afterwards.  And then probably more tests should be added.

João

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.