Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C-i and C-o shouldn't jump between files in different workspaces #2826

Closed
thiagokokada opened this issue Apr 5, 2020 · 10 comments
Closed

C-i and C-o shouldn't jump between files in different workspaces #2826

thiagokokada opened this issue Apr 5, 2020 · 10 comments
Labels
is:bug Something isn't working as intended module:core Relevant to Doom core status:resolved Issue was addressed internally

Comments

@thiagokokada
Copy link
Contributor

thiagokokada commented Apr 5, 2020

What did you expect to happen?
Each workspace should be independent between each other, so opening a new project in a new workspace should be like opening another project in a new instance of Emacs, just in another "tab" (workspace) instead.

What actually happened?
Calling C-o (better-jumper-jump-backward) after opening a new file from a new project inside a new workspace goes back to the old file from the old project, however the workspace is not switched up.

So you actually have, say, a workspace called "Project 2" that has a file from "Project 1" opened on it (while "Project 1" workspace still exists).

Additional details:

Steps to reproduce:

  1. Open a project inside doom-emacs
  2. Open any file from this project
  3. Switch to another project using SPC p p (this will create a new workspace)
  4. Open any file from this new project
  5. Use C-o
  6. Alternating between C-i and C-o will alternate between files from the two projects (at least this is consistent)

System information:

emacs   version    27.0.90
        features   XPM JPEG TIFF GIF PNG RSVG SOUND DBUS GSETTINGS GLIB NOTIFY INOTIFY LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS LIBSYSTEMD JSON PDUMPER GMP
        build      abr 04, 2020
        buildopts  (--prefix=/nix/store/vlka0vgjgi2kz828361xllmqsrczm1gk-emacs-27.0.90 --disable-build-details --with-modules --with-x-toolkit=gtk3 --with-xft CFLAGS=-DMAC_OS_X_VERSION_MAX_ALLOWED=101200)
        windowsys  x
        daemonp    server-running
doom    version    2.0.9
        build      HEAD -> develop, origin/develop, origin/HEAD a381f5926 2020-04-03 02:12:50 -0400
        dir        ~/.dotfiles/doom-emacs/.doom.d/
system  type       gnu/linux
        config     x86_64-pc-linux-gnu
        shell      /run/current-system/sw/bin/zsh
        uname      Linux 5.4.28 #1-NixOS SMP Wed Mar 25 07:26:00 UTC 2020 x86_64
        path       (/nix/store/6snxzbkk7llrk45w6xy8y9fmaiklscr1-emacs-packages-deps/bin ~/dev/nu/nucli.py/bin ~/dev/nu/nudev/docker/forticlientvpn ~/dev/nu/ios-cli ~/dev/nu/nucli ~/dev/nu/nudev ~/.npm-packages/bin ~/.emacs.d/bin ~/.local/bin /nix/store/m1kpr61gayshnf5fs9d654kdb8c3bkh1-kitty-0.14.6/bin /nix/store/3j32yw16prmqw7mppgjbgczlcpgjgq3d-imagemagick-6.9.10-71/bin /nix/store/rax8pqvzzakz1wjnvdb8mdda7zw86w0c-xsel-unstable-2018-01-10/bin /nix/store/jwhd7yi26sfbi5djclxrr3ims6r5796i-ncurses-6.1-20190112-dev/bin ~/bin /run/wrappers/bin ~/.nix-profile/bin /etc/profiles/per-user/thiagoko/bin /nix/var/nix/profiles/default/bin /run/current-system/sw/bin /nix/store/vlka0vgjgi2kz828361xllmqsrczm1gk-emacs-27.0.90/libexec/emacs/27.0.90/x86_64-pc-linux-gnu)
config  envfile    nil
        elc-files  0
        modules    (:completion company (ivy +fuzzy +icons) :ui doom doom-dashboard hl-todo (modeline +light) nav-flash ophints (popup +defaults) treemacs vc-gutter vi-tilde-fringe window-select workspaces :editor (evil +everywhere) file-templates fold (format +onsave) multiple-cursors snippets word-wrap :emacs (dired +icons) electric vc :term vterm :checkers syntax :tools docker (eval +overlay) lookup lsp magit :lang cc (clojure +lsp) data elixir emacs-lisp go hy javascript markdown nix (org +dragndrop +present) plantuml python ruby rust scala sh :config (default +bindings +smartparens))
        packages   ((adoc-mode) (color-identifiers-mode) (evil-escape :disable t) (lispyville) (lsp-ui :disable t) (sort-words) (uuidgen) (vlf))
        unpin      (n/a)
        elpa       (n/a)
@thiagokokada thiagokokada added the is:bug Something isn't working as intended label Apr 5, 2020
@thiagokokada thiagokokada changed the title C-o shouldn't jump between workspaces C-o shouldn't jump between files in different workspaces Apr 5, 2020
@thiagokokada thiagokokada changed the title C-o shouldn't jump between files in different workspaces C-i and C-o shouldn't jump between files in different workspaces Apr 5, 2020
@ericdallo
Copy link
Contributor

This always annoyed me, but I didn't realize that was because the "queue" of C-o has ended and it was going back to the other workspace, nice catch!
I can confirm this happens to me too.

@raszi
Copy link
Contributor

raszi commented Dec 16, 2020

I can also confirm that this happens to me too, and it annoys the hell out of me! 😈

@sgleizes
Copy link

sgleizes commented Dec 16, 2020

I've also been frustrated by this issue and I came up with the following partial fix:

(after! better-jumper
  (defun doom-prevent-persp-jump (orig-fn &rest args)
    "Ensure ORIG-FN doesn't set any jump points in buffers from other perspectives."
    (unless (and (markerp (car args))
                 (not (+workspace-contains-buffer-p (marker-buffer (car args)))))
      (apply orig-fn args)))
  (add-hook! better-jumper-mode
    (defun doom-prevent-persp-jump-h ()
      (advice-add #'evil-set-jump :around #'doom-prevent-persp-jump)
      (advice-add #'better-jumper-set-jump :around #'doom-prevent-persp-jump))))

While this prevents the addition of jump points from other perspectives, there are other issues that also need to be addressed:

  • When creating a new workspace/perspective with +workspace/new, the jumplist seems to be inherited from the previous workspace, whereas when restoring a session with multiple workspaces, the jumplists are adequately independent. I'm not sure about the internal details here.
  • When creating a new window in the same workspace (say with evil-window-vsplit), the jumplist from the previous window is not copied even though better-jumper-new-window-behavior is set to copy. This is contrary to what is advertised by better-jumper.

I still have to investigate these and maybe contribute it upstream, in the meantime the above snippet reduces the pain.

@gilbertw1
Copy link
Contributor

Hey,

I'm the author of better-jumper. It does support persp-mode by storing it's jump lists in the window configuration, so it supports the mechanism underpinning "workspaces" in Doom Emacs. This is the specific reason that I originally created the project as evil-jump was indiscriminate of persp-mode boundaries.

When creating a new workspace/perspective with +workspace/new, the jumplist seems to be inherited from the previous workspace

This is working by design based on the better-jumper-new-window-behavior. The previous window's jump list is copied to the initial window of the "workspace" when it's created. The concept of a "workspace" is something specifically created by Doom Emacs and not something better-jumper itself is aware of. In this case I would recommend an update to Doom that clears the jump list when creating a new workspace is created so that the new window doesn't retain the previous window's jump list (which happens to be in a different "workspace").

When creating a new window in the same workspace (say with evil-window-vsplit), the jumplist from the previous window is not copied even though better-jumper-new-window-behavior is set to copy. This is contrary to what is advertised by better-jumper.

@sgleizes Can you please create a bug on gilbertw1/better-jumper with steps to reproduce this issue? From my testing, I'm not able to reproduce this problem.

the addition of jump points from other perspectives

@sgleizes Could you also provide a scenario that reproduces better-jumper adding jump points between different persp-mode perspectives as well? I'm not able to reproduce that one either.

@sgleizes
Copy link

Hello and thanks for your insights!

The previous window's jump list is copied to the initial window of the "workspace" when it's created.

That's also what I thought first, but in my case it seems that the window for the new workspace and the original window share the same jump list (does it mean that they are the same window?). To make sure I tried clearing the jump list as in your PR, and this results in the jump list of the original perspective's window being cleared as well, which is not intended. This happens regardless of the better-jumper-new-window-behavior setting.

Can you please create a bug on gilbertw1/better-jumper with steps to reproduce this issue?

I will, but first I will try with a minimal configuration to make sure I don't have some parts of my own config interfering here, since it seems that I get a different behavior on all mentioned aspects.

Could you also provide a scenario that reproduces better-jumper adding jump points between different persp-mode perspectives as well?

For me this was occurring when simply switching between workspaces using e.g. +workspace/switch-left or similar commands. The buffer from the previous workspace was being added to the jumplist of the current workspace. This seemed to occur with the post-command-hook of evil-jump, and so was not happening if I disabled better-jumper-use-evil-jump-advice.
I will also try to reproduce this with a minimal config.

@gilbertw1
Copy link
Contributor

Thanks for the response. The negative interactions with evil-jump definitely make sense as they made some recent changes that complicated the relationship between better-jumper and itself. I'll look into the new workspace behavior and see why the previous window configuration seems to be leaking into the new perspective as well.

@sgleizes
Copy link

sgleizes commented Dec 16, 2020

So I just tested with a default doom configuration. For the sake of clarity here are the 3 issues under investigation:

  1. +workspace/switch-to results in a jump from the previous persp being added to the new persp.
  2. +workspace/new inadequately copies the jump list from the current window into the new perspective window.
  3. evil-window-vsplit does not copy jump list from previous window if the window is focused directly.

How to reproduce

  1. On a default doom installation:
    1. Start emacs and open a file in the default workspace.
    2. Create a new workspace with +workspace/new and open another file.
    3. Switch back to the first workspace with +workspace/switch-left.
    4. Inspect the jump list with +ivy/jump-list and see the file from the second workspace.
  2. On a default doom installation, without the changes from your proposed PR:
    1. From the state of the previous test, create a new workspace with +workspace/new.
    2. Inspect the jump list with +ivy/jump-list and see that the list is not empty.
  3. On a default doom installation:
    1. Start emacs and open a file in the default workspace.
    2. Add entries to the jumplist, e.g. by starting a search and moving between matches.
    3. Eval (setq evil-vsplit-window-right t).
    4. Split window vertically with evil-window-vsplit.
    5. Inspect the jump list with +ivy/jump-list and see that the list is empty.

Solutions so far

  1. The snippet from my previous post is the best thing I have so far. This does not seem related to doom and should be addressed upstream in #6.

  2. Unfortunately, the changes from your proposed PR will also reset the jump list from the originating workspace (confirmed on a default doom installation). IIUC this is due to the fact that better-jumper-clear-jumps will use frame-selected-window, which in the context of persp-created-functions resolves to the previous window instead of the new window.

    I could not find a way to get a window from the persp argument. The solution I have currently is really an ugly hack:

    (add-hook! 'persp-created-functions
      (defun +worspaces-mark-creating (persp &rest _)
        (setq better-jumper-creating-perspective (persp-name persp))))
    (add-hook! 'persp-activated-functions
      (defun +worspaces-clear-initial-jump-list (&rest _)
        (let ((persp (get-current-persp)))
          (when (and persp (eq better-jumper-creating-perspective (persp-name persp)))
            (setq better-jumper-creating-perspective nil)
            (better-jumper-clear-jumps)))))
    

    I would say that this is a doom-related issue on the integration with the workspaces modules. Since setting better-jumper-new-window-behavior to 'empty has no effect on this behavior, it seems to indicate that the undesired copy is due to doom's workspace internals and is probably unrelated to upstream.

  3. No solution so far, I will open the issue upstream since this is about the implementation of better-jumper--window-configuration-hook.

@gilbertw1
Copy link
Contributor

Thanks, for the detailed reply. I've been pretty busy today, so I haven't had much time to look at this so far.

After updating doom to the lastest (including all packages), which I haven't don't in a while, I've noticed a regression where it appears that the window properties are being shared between perspectives as they're created, instead of the expected copy. Therefore the actual underlying jump lists are being utilized by multiple perspectives simultaneously which is causing confusing behavior with jumps crossing the perspective boundary.

I haven't yet been able to dig into where or when this regression was introduced, but I plan on looking into it tonight.

@gilbertw1
Copy link
Contributor

I'm still digging into the cause of the problem and a proper solution. It appears that the window configuration from the current window is being used whenever a new perspective is being created. This results in all the properties in the current selected window (better jumper jumps, buffer list, etc) being inherited into the new perspective instead of those properties being isolated between the new and old perspective.

A temporary workaround for this would be the following advice:

(with-eval-after-load 'persp-mode
  (defadvice persp-delete-other-windows (before better-jumper activate)
    (select-window (split-window))))

When persp-mode activates a new perspective and switches up the window configuration, it calls this function (persp-delete-other-windows) to effectively reset the window state, however it normally only deletes the other windows and leaves the current window intact. This advice effectively creates a new window and selects it before this function is called, resulting in all existing windows being deleted and only having the new window remain.

This will solve problem by preventing the existing window properties (buffer list, jumps, etc.) from leaking into the newly created perspective and result in an effectively blank window being used as the basis of the new perspective.

@gilbertw1
Copy link
Contributor

I just created a pull request that should solve this problem. Better jumper has been updated to now ensure that upon activating a new workspace, an existing jump list is never leaked into a new perspective.

I'm also contemplating creating a proposal for persp-mode that would add an option to more effectively reset window state when restoring a window configuration. I feel like this would effectively solve any other potential issues with leaking window properties into newly created perspectives, effectively making a new perspective more of a blank slate.

hlissner added a commit that referenced this issue Jan 3, 2021
@hlissner hlissner added module:core Relevant to Doom core status:resolved Issue was addressed internally labels Jan 3, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
is:bug Something isn't working as intended module:core Relevant to Doom core status:resolved Issue was addressed internally
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants