search

syl20bnr / spacemacs

A community-driven Emacs distribution - The best editor is neither Emacs nor Vim, it's Emacs *and* Vim!
http://spacemacs.org
GNU General Public License v3.0
23.66k stars 4.89k forks source link

Treemacs assigning window number, in both layer and package #9865

Closed duianto closed 5 years ago

duianto commented 6 years ago

Description :octocat:

The treemacs--window-number-ten function seems to be added to the winum-assign-functions variable twice.

In the Treemacs layer: https://github.com/syl20bnr/spacemacs/blob/36c15b79f6d490a4733cfca3278c0c837fdc2cff/layers/%2Bfiletree/treemacs/packages.el#L71-L74

And in the Treemacs package: https://github.com/Alexander-Miller/treemacs/blob/fd5c893ce920082b4fcfc764feecc47096500ae2/treemacs-compatibility.el#L38-L39

This is what the variable looks like when Spacemacs starts:

winum-assign-functions is a variable defined in ‘winum.el’.
Its value is
(treemacs--window-number-ten treemacs--window-number-ten)
Original value was nil

Pinging the Treemacs author: @Alexander-Miller

Reproduction guide :beetle:

Observed behaviour: :eyes: :broken_heart: The messages buffer shows:

Winum conflict - window #<window 21 on *Treemacs*> was assigned a number by multiple custom assign functions: ’(treemacs--window-number-ten -> 10 treemacs--window-number-ten -> 10)’ [2 times]

Expected behaviour: :heart: :smile: The winum conflict should be resolved.

System Info :computer:

Alexander-Miller commented 6 years ago

Thanks for the ping. Fix should be easy enough. I'll replace the push with add-to-list later today.

Alexander-Miller commented 6 years ago

Replacement is pushed. Conflict should be resolved now.

duianto commented 6 years ago

The Winum conflict message doesn't appear anymore, and the variable only contains one instance of the treemacs--window-number-ten function name (as expected) 👍

But I happened to have opened Spacemacs in Emacs 25.3.1 (the official version emacs-25.3_1-x86_64.zip, from https://www.gnu.org/software/emacs/). I updated the packages (including Treemacs).

Now when Treemacs has been opened at least once, so that the buffers list SPC b b shows an entry for *Treemacs*. Then if the Treemacs window is either open or closed, when SPC q r is pressed to call restart-emacs-resume-layouts.

When Spacemacs starts, then a side window with the same size as Treemacs, opens and closes for a split second. I managed to see that it shows window number 10, so it's probably the Treemacs window.

The messages buffer shows this message: Blocking call to accept-process-output with quit inhibited!! [4 times]

And the buffers list still shows an entry for *Treemacs*.

I then closed Emacs 25.3.1 and started the pre-release version 26.0.50 (emacs-bin-w64-26.0.50.7z from https://github.com/zklhp/emacs-w64/releases/tag/192342a). I use it most of the time, unless I'm comparing an issue with the latest stable version 25.3.1. I updated the packages (including Treemacs).

But in 26.0.50, none of the things mentioned above occur, when Treemacs has been opened one or more times, and it's open or closed when restart-emacs-resume-layouts SPC q r is called.

When Spacemacs starts then there's:

I remember having seen the Blocking call messages before, but I didn't have clear reproduction steps. This is the note I had written down:

These messages sometimes appear when navigating in the treemacs window: Blocking call to accept-process-output with quit inhibited!! [18 times] [Treemacs] Refresh complete.

This page might contain some useful info: http://emacs.1067599.n8.nabble.com/Blocking-call-to-accept-process-output-with-quit-inhibited-11-times-td149496.html

Unfortunately I hadn't added a date when it was written, but I suspect that it might have been shortly after the Treemacs layer was added to Spacemacs. But I'm sure that it was before the winum conflict messages started appearing, so it might not be related to that being fixed.

I haven't tried Treemacs in 25.3.1 a lot, so this issue might have been occurring all along, and I just happened to notice it now.

System Info :computer:

Alexander-Miller commented 6 years ago

When Spacemacs starts, then a side window with the same size as Treemacs, opens and closes for a split second. I managed to see that it shows window number 10, so it's probably the Treemacs window.

I have just pushed a massive update to master (all treemacs buffers are frame-local now). Part of that update was removing the persp restoration integration since persp does not touch frames. Try again when the new update is built. If things go as intended persp should make no attempt to restore any treemacs buffers.

Blocking call to accept-process-output with quit inhibited!! [4 times]

Annoying, but also harmless. The external processes I use are git status and a simple python script. I tried to prevent these in treemacs in some places, but now that I think about it all the async parts are done with my own pfuture module. Updating that should do the trick.

duianto commented 6 years ago

With the latest versions of both: treemacs-20171114.1047 and treemacs-projectile-20171114.1009 In Emacs 25.3.1:

However, when the buffer list SPC b b is opened the first time, then it only shows the 3 standard buffers:

*scratch*
*Messages*
*spacemacs*

But when the buffer list is closed and reopened, then there's a 4th entry *Compile-Log*, containing following text in both Emacs 25.3.1 and 26.0.50

~/.emacs.d/elpa/25.3/develop/treemacs-20171114.1047/treemacs-compatibility.elc:Warning:
    reference to free variable ‘treemacs--buffer-name’
~/.emacs.d/elpa/25.3/develop/treemacs-20171114.1047/treemacs-compatibility.elc:Warning:
    reference to free variable ‘treemacs--buffer-name’
~/.emacs.d/elpa/26.0/develop/treemacs-20171114.1047/treemacs-compatibility.elc:Warning:
    reference to free variable ‘treemacs--buffer-name’
~/.emacs.d/elpa/26.0/develop/treemacs-20171114.1047/treemacs-compatibility.elc:Warning:
    reference to free variable ‘treemacs--buffer-name’

and after Treemacs has been opened, then the *Treemacs* entry in the buffer list has the frame with the title appended to the name.

*Treemacs-#<frame Windows 10.0.14393  Emacs 25.3.1  Spacemacs 0.300.0 (develop: 36c15b79)  Project (-) 00000004008d7820>*
*Treemacs-#<frame Windows 10.0.14393  Emacs 26.0.50  Spacemacs 0.300.0 (develop: 36c15b79)  Project (-) 0000000400a12210>*
Alexander-Miller commented 6 years ago

However, when the buffer list SPC b b is opened the first time, then it only shows the 3 standard buffers:

When you say "only" does it mean you were expecting something else?

But when the buffer list is closed and reopened, then there's a 4th entry Compile-Log, containing following text in both Emacs 25.3.1 and 26.0.50

Don't know the compile log shows up late, but the warning is easy to remove. The entire popwin compatibility block will be deleted since it's only needed to get around window-size-fixed being t, which treemacs does not use anymore.

and after Treemacs has been opened, then the Treemacs entry in the buffer list has the frame with the title appended to the name.

Working as inteded. Since there may now be multiple treemacs buffers I needed to come up with a naming scheme and my solution is (format "*Treemacs-%s* (selected-frame)), though for me frame strings look like #<frame emacs@am-laptop 0x120cc30>. I'll shorten this to using a number, since I give each frame a unique number parameter for desktop-mode persistence anyway.

duianto commented 6 years ago

When you say "only" does it mean you were expecting something else?

It's the expected behaviour. I see now, that it sounds like I meant that it wasn't.

I'll shorten this to using a number, since I give each frame a unique number parameter for desktop-mode persistence anyway.

That's probably better, since the frame number isn't visible in the UI, so it isn't helpful for identifying which frame it's referring to:

*Treemacs-#<frame Windows 10.0.14393  Emacs 26.0.50  Spacemacs 0.300.0 (develop: 36c15b79)  Project (-) 0000000400a12210>*
*Treemacs-#<frame Windows 10.0.14393  Emacs 26.0.50  Spacemacs 0.300.0 (develop: 36c15b79)  Project (-) 0000000007b24210>*

One thing that might help, is if the Treemacs windows buffer name (on the mode line) and the buffer list entry had the same name and number.

There's a window number 10 conflict, when Treemacs is opened in a second frame:

Number 10 already assigned to #<window 45 on *Treemacs-#<frame Windows 10.0.14393  Emacs 26.0.50  Spacemacs 0.300.0 (develop: 36c15b79)  Project (-) 0000000400a12210>*>, can’t assign to #<window 54 on *Treemacs-#<frame Windows 10.0.14393  Emacs 26.0.50  Spacemacs 0.300.0 (develop: 36c15b79)  Project (-) 000000000c930e20>*> [2 times]

The Treemacs window in the second frame gets the next available window number as expected. But there probably should be a check, so that it won't try to assign number 10 again if a Treemacs window already is open.

Questions

What are the use cases for having Treemacs entries in the buffer list? If one selects one of them, then the current window opens that Treemacs buffer. I guess I thought of filetree windows as being shown on the side, but that's probably only one way of displaying a filetree.

What was the reason for assigning the Treemacs window as number 10 instead of 0, like Neotree does? When there are 10 windows, and Treemacs is opened, then Treemacs takes over the 10th window number, and any window number above 9 gets incremented when Treemacs opens and decremented when Treemacs closes. It's seems like an unnecessary operation. But I might be unaware of other benefits with using 10 instead of 0.

Alexander-Miller commented 6 years ago

One thing that might help, is if the Treemacs windows buffer name (on the mode line) and the buffer list entry had the same name and number.

Good idea.

There's a window number 10 conflict, when Treemacs is opened in a second frame:

Will take a look, thanks.

What are the use cases for having Treemacs entries in the buffer list?

The use case is that I don't know how to get rid of them, and it hasn't really been a priority until now. If you know how to make these buffers disappear let me know.

What was the reason for assigning the Treemacs window as number 10 instead of 0, like Neotree does?

Spaceline does not show a unicode symbol for 0. Easiest solution was to go with 10 instead.

When there are 10 windows, and Treemacs is opened, then Treemacs takes over the 10th window number, and any window number above 9 gets incremented

I image that does not happen very often. For the cases when it does I can introduce a config var to determine which number treemacs takes. Side benefit: setting it to null will also mean treemacs' window number is unmanaged.

duianto commented 6 years ago

If you know how to make these buffers disappear let me know.

Since Neotree doesn't show a buffer list entry, I started looking through the source code and I noticed the space before the name:

(defconst neo-buffer-name " *NeoTree*"
  "Name of the buffer where neotree shows directory contents.")

and when I checked (current-buffer) on the file and the Neotree window:

#<buffer neotree.el>
#<buffer  *NeoTree*>

That made me think that the space might be relevant, and I found this:

Buffers that are ephemeral and generally uninteresting to the user have names starting with a space, so that the list-buffers and buffer-menu commands don't mention them (but if such a buffer visits a file, it is mentioned). A name starting with space also initially disables recording undo information; see Undo.

source: http://www.gnu.org/software/emacs/manual/html_node/elisp/Buffer-Names.html

If that works to hide the buffer list entry, then maybe the Treemacs buffer name doesn't need to be renamed with a number.

Alexander-Miller commented 6 years ago

Did I forget anything?

duianto commented 6 years ago

That seems to fix the issues discussed above. 👍

But with treemacs-20171116.1011, the Treemacs window gets assigned number 1, even though the treemacs-winum-number has the default value 10.

Alexander-Miller commented 6 years ago

Make sure you winum-scope is set to frame-local. If it is tell me which of the following conditions fails:

(with-selected-window (treemacs--is-visible?)
  (when (and (eq (selected-window) (frame-first-window))
             (treemacs--is-treemacs-window-selected?)
             (boundp 'winum-scope)
             (eq winum-scope 'frame-local))
    treemacs-winum-number))
duianto commented 6 years ago

That's the problem, winum-scope is global, therefore (eq winum-scope 'frame-local) returns nil. I have no memory of having changed it, so maybe global is the default value?

PS: I just read the Treemacs readme compability section:

Using winum-mode with a scope other than frame-local means that potentially multiple treemacs windows can be part of the same window numbering scheme, resulting in a conflict since by default all treemacs windows are assigned treemacs-winum-number as their window number. To prevent this treemacs will only be assigned a custom number when the value of winum-scope is frame-local.

So it's by design. 👍

duianto commented 6 years ago

I'll close this issue, since the original issue has been solved, including several others, and everything seems to be working as expected. Thanks @Alexander-Miller

Alexander-Miller commented 6 years ago

Better keep this open, we've another window number problem to discuss: the default winum scope is global, this effectively disables treemacs' window number assignment. AFAIK neotree has always taken window #0 in spacemacs. Treemacs should do the same, or we'll be breaking workflows.

Removing the scope=local check is a bad option, since it exposes treemacs to number conflicts. My vote therefore goes towards setting the scope to frame-local in spacemacs, at least when treemacs is loaded and optionally enabled/disabled with a config option. This is making things rather overly complicated, but I cannot see a less complex option that can guarantee full compatibility while keeping the #10 assignment.

duianto commented 6 years ago

Is it possible to only assign number 10, to the Treemacs window in the frame that has window number 1 in it?

And if it's possible to detect the Treemacs window in the current frame, then maybe it could be selectable with SPC 0 (in addition to the window number it currently has). Maybe it should be optional. Since it prevents one from directly selecting the Treemacs window in the frame with window number 1.

Alexander-Miller commented 6 years ago

Might be possible, though it falls under the "I am uncomfortable with how complex this simple feature is becoming" category. We'd have to make our way through half of winum's private implementations details.

I may have found a reasonable compromise: assign number 10 only in the selected frame. It will still shift around numbers when frame selection switches, but so far it's the smallest evil I see. I can make this apply only when there is more than 1 treemacs buffer to lessen the impact.

I'll go about implementing this, though I still remain open for better ideas.

Alexander-Miller commented 6 years ago

Nope, idea doesn't work.

The next best thing I see is to refactor winum, such that some window types are allowed get the same number multiple times. Trying to select such a window by number would then look for that number in the local frame, then globally.

ping @deb0ch What do you think?

quicknir commented 6 years ago

Curious, where are we with this? An alternative (IMHO) is simply to get rid of the numbering for the file tree completely, and simply have a custom shortcut for it. SPC 0 is not particularly convenient anyhow. Some alternatives: SPC ' (can change select window by number, which I guess is rarely used, to SPC 0), SPC w T, SPC S, etc.

I think the main thing should be for the file tree to not affect other numbering, but I don't think it's particularly important that the file tree be numbered 0 as long as it has some good custom shortcut.

Alexander-Miller commented 6 years ago

Curious, where are we with this?

I was just fresh out of ideas. Luckily you've had another. AFAIK winum is not able to not give a number to some window, but it should be possible to patch that in.

simply have a custom shortcut for it. SPC 0 is not particularly convenient anyhow.

A function to select treemacs already exists, all we need is just a good keybind for it. For me selecting window 0 is M-0, I don't know if that's the standard or something I changed, but having used it nearly since treemacs' inception it definitely qualifies as convenient for me. There's also something like SPC , or SPC -. They're not taken and easily reachable.

What about neotree? Does it have its own selection option or does it also fully rely on winum?

quicknir commented 6 years ago

As far as I know it relies on winnum but writing such a function cannot be that hard, afaik the neotree buffer always has the same name, so you can write a simple function that loops over windows until it finds one with an active buffer with the right name.

duianto commented 6 years ago

For me selecting window 0 is M-0, I don't know if that's the standard or something I changed

M-0 selects window 0 or 10 by default, C-h k M-0 shows:

M-0 runs the command winum-select-window-0-or-10 (found in winum-keymap), which
is an interactive autoloaded compiled Lisp function in ‘winum.el’.

It is bound to SPC 0, M-m 0, M-0, C-x w 0.

(winum-select-window-0-or-10 &optional ARG)

Jump to window 0 if assigned or 10 if exists.
If prefix ARG is given, delete the window instead of selecting it.

[back]

What about neotree? Does it have its own selection option or does it also fully rely on winum?

This issue: Feature request: 'SPC 0' to open neotree #9313

and this PR might be relevant: neotree: Use ~SPC 0~ to open the tree #9917

quicknir commented 6 years ago

I think this may actually be already possible: https://github.com/deb0ch/emacs-winum/blob/master/winum.el. 

(defcustom winum-ignored-buffers '(" *which-key*")
  "List of buffers to ignore when assigning numbers."
  :group 'winum
  :type  '(repeat string))

I think basically what spacemacs need to do is add "neotree" and "treemacs" (or whatever) to that list, get rid of the mode line (not very useful anyway), and then make SPC 0 bound to the appropriate functions for treemacs (already exists) and neotree (to be written).

Or as a first/final step, treemacs layer could do this on its own and leave neotree as is for the time being.

Alexander-Miller commented 6 years ago

winum-ignored-buffers

Excellent. I was just about to start implementing something like this. Treemacs buffers have generated numbers in them, so I'll still need to change to match to use regex, but that's a quick and simple fix. When that patch is in I'll update treemacs and its layer accordingly.

quicknir commented 6 years ago

Sorry, just to be clear: what do you mean by "that patch"? Mostly I want to make sure that you don't feel like you are blocked by anything on the spacemacs/winum side that I could try to help out with :-).

Alexander-Miller commented 6 years ago

If you want to help out all the better. I mean the needed change in winum. The ignore check currently looks at buffer names in a list. All you need to do is change that to use a regex instead.

quicknir commented 6 years ago

Ah ok. Sure, I will take a look at this tomorrow. It's pretty trivial, the main bottleneck will be tracking down @deb0ch, I'll ping you when it's done.

quicknir commented 6 years ago

@Alexander-Miller I put 

  (add-to-list 'winum-ignored-buffers " *Treemacs-Framebuffer-1*")
  (spacemacs/set-leader-keys "<escape>" 'treemacs-select-window)

In my config and it's working great. Although a regex is more ideal, if the treemacs buffer name only depends on the frame, could we just do a loop e.g. from 1-10 or something? I don't think anyone is likely to have more than 10 frames. I realize this isn't ideal but deboch hasn't been responding for a bit, so it might be nice to get a fix in that works for 99.9% of people instead of waiting an indeterminate amount of time.

duianto commented 5 years ago

Maybe this PR solved the winum ignore issue? [treemacs] Make sure treemacs' buffers are ignored by winum. #11851

I'll close this, but if there's anything left to resolve then I can reopen it. Although it's probably better to open a new issue, under a more relevant title.

Alexander-Miller commented 5 years ago

LGTM, and I haven't received any other winum-related issues since these fixes either.