bug#20921: 25.0.50; `tabulated-list-print' bad logic

bug#20921: 25.0.50; `tabulated-list-print' bad logic

[Drew Adams]

This new code seems quite wrong to me:

(and remember-pos
     (when (eq (window-buffer) (current-buffer))
       (setq window-line (count-screen-lines (window-start) (point))))
     (setq entry-id (tabulated-list-get-id))
     (setq saved-col (current-column)))

That's equivalent to this (which is clearer):

(and remember-pos
     (eq (window-buffer) (current-buffer))
     (setq window-line (count-screen-lines (window-start) (point)))
     (setq entry-id (tabulated-list-get-id))
     (setq saved-col (current-column)))

Previously, the code was this:

(and remember-pos
     (setq entry-id (tabulated-list-get-id))
     (setq saved-col (current-column)))

When I use the new code, `window-buffer' is the original buffer from
which I call my command that uses tabulated-list-mode. `current-buffer'
is the buffer that will contain the tabulated list, and it is not yet
displayed.  So that test fails, and none of the variables get assigned
values.  This makes no sense to me.

When code calls `tabulated-list-print', the current buffer must pretty
much always be the buffer that will get the tabulated list.  Why the
comparison with `window-buffer', which could be anything and which in
most cases will *not* be the tabulated-list buffer?

This new code looks like it might be a misguided hack to try to deal
with the new parameter UPDATE.  Not ready for prime time, I think.

In GNU Emacs 25.0.50.1 (i686-pc-mingw32)
 of 2015-05-29 on LEG570
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=/mingw32 --host=i686-pc-mingw32
 --enable-checking=yes,glyphs'

bug#20921: 25.0.50; `tabulated-list-print' bad logic

[Artur Malabarba]

[-- Attachment #1: Type: text/plain, Size: 1238 bytes --]

On Jun 28, 2015 9:46 PM, "Drew Adams" <drew.adams@oracle.com> wrote:
>
> This new code seems quite wrong to me:
>
> (and remember-pos
>      (when (eq (window-buffer) (current-buffer))
>        (setq window-line (count-screen-lines (window-start) (point))))
>      (setq entry-id (tabulated-list-get-id))
>      (setq saved-col (current-column)))
> So that test fails, and none of the variables get assigned
> values.  This makes no sense to me.

Yes, that's wrong. The window logic should come last, so as to not prevent
the other variables.

> When code calls `tabulated-list-print', the current buffer must pretty
> much always be the buffer that will get the tabulated list.  Why the
> comparison with `window-buffer', which could be anything and which in
> most cases will *not* be the tabulated-list buffer?
>
> This new code looks like it might be a misguided hack to try to deal
> with the new parameter UPDATE.  Not ready for prime time, I think.

No.
IIRC, it was necessary because of async refreshing in package.el. It makes
no sense to store the screen line if current window is not displaying that
buffer.
As I said above, blocking the other two variables was unintentional.

I can do that tomorrow if nobody beats me to it.

[-- Attachment #2: Type: text/html, Size: 1563 bytes --]

bug#20921: 25.0.50; `tabulated-list-print' bad logic

[Artur Malabarba]

Should be fixed now.

2015-06-29 1:18 GMT+01:00 Artur Malabarba <bruce.connor.am@gmail.com>:
>
> On Jun 28, 2015 9:46 PM, "Drew Adams" <drew.adams@oracle.com> wrote:
>>
>> This new code seems quite wrong to me:
>>
>> (and remember-pos
>>      (when (eq (window-buffer) (current-buffer))
>>        (setq window-line (count-screen-lines (window-start) (point))))
>>      (setq entry-id (tabulated-list-get-id))
>>      (setq saved-col (current-column)))
>> So that test fails, and none of the variables get assigned
>> values.  This makes no sense to me.
>
> Yes, that's wrong. The window logic should come last, so as to not prevent
> the other variables.
>
>> When code calls `tabulated-list-print', the current buffer must pretty
>> much always be the buffer that will get the tabulated list.  Why the
>> comparison with `window-buffer', which could be anything and which in
>> most cases will *not* be the tabulated-list buffer?
>>
>> This new code looks like it might be a misguided hack to try to deal
>> with the new parameter UPDATE.  Not ready for prime time, I think.
>
> No.
> IIRC, it was necessary because of async refreshing in package.el. It makes
> no sense to store the screen line if current window is not displaying that
> buffer.
> As I said above, blocking the other two variables was unintentional.
>
> I can do that tomorrow if nobody beats me to it.