alphapapa / dogears.el

Never lose your place in Emacs again
GNU General Public License v3.0
205 stars 8 forks source link

Enabling `switch-to-buffer-obey-display-action` seems to cause some Dogears commands to not work as expected #25

Closed chansey97 closed 4 months ago

chansey97 commented 4 months ago

Reproduce steps:

  1. Init settings
    (setq dogears-idle nil)
    (require 'dogears)
    (dogears-mode)
  2. Open Emacs
  3. Ensure you have two windows: *scratch* and *Dogears List*
  4. Move the point to Pos 146 in *scratch*, then M-x dogears-remember
  5. Switch to init.el
  6. Move the point to Pos 300 in init.el, then do M-x dogears-remember
  7. Move the point to Pos 7272 in init.el, then do M-x dogears-remember
  8. Switch to dogears.el
  9. Move the point to Pos 1280 in dogears.el, then do M-x dogears-remember
  10. Do M-x dogears-back, the cursor jump to Pos 7272 in init.el (correct)
  11. Do M-x dogears-back again, doesn't see any jumping! (wrong, it should jump to Pos 300 in init.el)
  12. Do M-x dogears-back again, the cursor jump to Pos 146 in *scratch* (correct)

Note that, the entries motion in *Dogears List* is correct, i.e. in the step 11, I could see the highlight entry moving from 1 to 2, but did not see the cursor jump! It seems that multiple dogears-remembers in the same buffer doesn't work well.

image

image

P.s. The dogears-go has a similar phenomenon.

Using Emacs 29.3 on Windows and the latest version of dogears.el https://github.com/alphapapa/dogears.el/commit/162671e66cac601f1cfd5d22f7da2671af2e9866

Thanks.

chansey97 commented 4 months ago

OK. I just tried with Emacs -Q and no problem.

The issue seems to be related to window management. I am using display-buffer-alist to manage windows. After comment all display-buffer-alist code, dogears.el can work.

Could you give some advice? Thanks.

chansey97 commented 4 months ago

Hi, I found the cause.

From L257:

(or (ignore-errors
      (bookmark-jump place))
    (when-let ((buffer (map-elt (cdr place) 'buffer)))
      (when (stringp buffer)
        (setf buffer (get-buffer buffer)))
      (if (buffer-live-p buffer)
          (switch-to-buffer buffer)
        (user-error "Buffer no longer exists: %s" buffer))))

Delete (when-let ...) can workaround the problem.

IMHO, it seems that (when-let ...) not very necessary.

  1. The ignore-errors says "Execute BODY; if an error occurs, return nil." but it is normal for (bookmark-jump place) to return nil.
  2. The bookmark-jump can switch buffer by itself, so manually switch-to-buffer is unnecessary.
  3. The (when-let ...) only switches buffer, but it does not update the point.

A possible patch:

Modify

(ignore-errors
        (bookmark-jump place))

to

(ignore-errors
        (bookmark-jump place) t)

PR.

alphapapa commented 4 months ago

OK. I just tried with Emacs -Q and no problem.

The issue seems to be related to window management. I am using display-buffer-alist to manage windows. After comment all display-buffer-alist code, dogears.el can work.

Could you give some advice? Thanks.

Please provide the display-buffer-alist value that you found to be problematic so that we may fully understand the problem you are reporting.

chansey97 commented 4 months ago

However, the change you propose here would have unintended consequences, preventing the following form from ever being executed, and that form exists for a reason.

I have no idea about the meaning of the following form (when-let ...).

I guess it should be executed only when (bookmark-jump place) fails, which means (bookmark-jump place) raised a error, and (when-let ...) is just for some fallback / robustness reasons (correct me if I am wrong).

If not appending t, the (bookmark-jump place) normally will return nil, which will cause the (when-let ...) to "almost always" be executed. I said "almost always" because bookmark-jump docs doesn't say anything about its return value. In this case, both (bookmark-jump place) and (switch-to-buffer buffer) will be called, which seems unnecessary.

Could you tell me the meaning of the following form (when-let ...)?

Please provide the display-buffer-alist value that you found to be problematic so that we may fully understand the problem you are reporting.

Further investigation revealed that this issue is not related to display-buffer-alist, but switch-to-buffer-obey-display-actions.

You can reproduce the issue via emacs -Q or --init-directory with a clean directory and clean init.el.

If (setq switch-to-buffer-obey-display-actions t), this issue can be reproduced.

If (setq switch-to-buffer-obey-display-actions nil) which is default, then everything is OK

The reproduce steps are simpler than the original one.

  1. Open init.el,
  2. Do M-x dogears-remember at some locations in init.el,
  3. Switch to dogears.el and M-x dogears-remember at one location.
  4. Do M-x dogears-back multiple times.

P.s. No need to touch display-buffer-alist. No need to open *scratch* Only one window is enough, no need to open two windows such as *scratch* and *Dogears List*.

Thanks.

alphapapa commented 4 months ago

Have you read the documentation for switch-to-buffer-obey-display-action? Why have you set that option? I had never even heard of it, having used Emacs since before it was added. Anyway:

  1. I don't know of any reason to enable that option.
  2. I don't know if setting that option can be reasonably handled by any other code that calls switch-to-buffer; it seems like a user-level override, so if a user uses it, he should probably expect to find conflicts. Dogears calls switch-to-buffer and expects it to do just that, not to have its behavior overridden.

Therefore, knowing no reason to enable that option, and not knowing a reasonable way to work around it:

  1. I can't be sure this is really a bug.
  2. I can see no reason to try to work around it.

I would recommend that you do not enable that option. Instead, if you find a "problem" that it solves, it would probably be better solved by calling display-buffer instead of switch-to-buffer.

Also, note carefully this part of the documentation:

If this results in displaying
the buffer in the selected window, window start and point are adjusted
as prescribed by the option ‘switch-to-buffer-preserve-window-point’.
chansey97 commented 4 months ago

Have you read the documentation for switch-to-buffer-obey-display-action?

Yes. I use it because I am using display-buffer-alist to manage all the window display. Some 3rd packages use switch-to-buffer directly or indirectly internally. Without enabling switch-to-buffer-obey-display-action I have no control over where these windows are displayed (I can't remember which packages they was though, probably some early packages).

Another reason of enabling switch-to-buffer-obey-display-action is that Mastering Emacs recommend:

You probably don’t want that. I recommend you set this:

;; Requires Emacs 27+
(setq switch-to-buffer-obey-display-actions t)

He also said switch-to-buffer is a user-facing command.


If this results in displaying the buffer in the selected window, window start and point are adjusted as prescribed by the option ‘switch-to-buffer-preserve-window-point’.

This is definitely relevant and I didn't know it before, thanks!

Another workaround might be (tested OK)

(or (ignore-errors
      (bookmark-jump place))  ; <--- rollback t, personally I still think it is needed though
    (when-let ((buffer (map-elt (cdr place) 'buffer)))
      (when (stringp buffer)
        (setf buffer (get-buffer buffer)))
      (if (buffer-live-p buffer)
          (let ((switch-to-buffer-preserve-window-point nil)) ; <--- disable switch-to-buffer-preserve-window-point
            (switch-to-buffer buffer))
        (user-error "Buffer no longer exists: %s" buffer))))

In addition (not important though), I still don't quite understand why you call both bookmark-jump and switch-to-buffer.

Thanks.

alphapapa commented 4 months ago

I recommend doing some research into the history of switch-to-buffer-obey-display-action. You can trace its development in Emacs.git. Then you can find the bug reports where it was discussed initially, and where the dframe package had to be modified because of it. In those discussions you can find a maintainer admitting that switch-to-buffer is used throughout the Emacs core codebase; it is not only a user-facing, interactive command. Therefore, IMHO enabling this option is not a good idea, likely to cause problems all over the place. Instead, if you find a call to switch-to-buffer that does not do what you want, it would be better to suggest calling display-buffer instead. (Note that I doubt I would be interested in making such a change in Dogears, because that's not how it's intended to work. If you want an older place to be displayed in another window, just switch to that window before calling Dogears's commands.)

In addition (not important though), I still don't quite understand why you call both bookmark-jump and switch-to-buffer.

The Bookmark system's code is complicated and antiquated. When dealing with old code like that, workarounds are often required. If you want to know more, I'd suggest forking this package and experimenting with the code until you understand how it works and why things are done as they are. Nothing here has been done without a reason. I don't generally have the time to explain every line of code; and when dealing with code written a long time ago, I don't necessarily remember the details, anyway.

Since I don't intend to make a change in Dogears with respect to this option, I'm going to close this issue. But feel free to continue discussing it in case there are a few other users who use that option, who you want to leave information for.

chansey97 commented 3 months ago

This issue might be due to an upstream bug in Emacs. I reported it on GNU Emacs: bug#71616: switch-to-buffer behaves inconsistently when switch-to-buffer

alphapapa commented 3 months ago

That would be somewhat of a relief. Thank you for reporting that and linking it here.