Correctly handle windows reserving a number larger than the amount of…

deb0ch / emacs-winum

Window numbers for Emacs - Navigate windows and frames using numbers

143 stars 13 forks source link

Correctly handle windows reserving a number larger than the amount of… #6

Closed Alexander-Miller closed 7 years ago

Alexander-Miller commented 7 years ago

One down, one to go.

Alexander-Miller commented 7 years ago

New version is up. Added an optional arg to only clear window numbers hash on update. Also retouched the conditional in get-window-by-number so n isn't compared against the window count, but the actual window-vector.

deb0ch commented 7 years ago

Hey !

I've taken the liberty of modifying a little bit your commit in my own branch, to:

enforce 80 columns
reformat commit messages
not use defsubst where it's not absolutely necessary
regroup the setter with the getter
separate the window-vector expansion into its own function
separate the winum-get-window-by-number fix into its own commit (good catch on that one :thumbsup: )
keep a compact and descriptive commit history

Let me know your thoughts !

I still have the bug from time to time in the state of this branch though, so I would like to take a little more time to understand what's going on before merging :thinking:

Alexander-Miller commented 7 years ago

Your codebase, your rules ¯\_(ツ)_/¯

I've pulled your changes locally now, but for me it's all quiet. Let me know if you run into something reprocucible. And best make sure emacs is running the updated version. Calling something like xref-find-definitions might jump you to the winum version in your elpa folder, because that's what emacs loads at start. Eval that buffer and suddenly the bug is back. It certainly happened to me at the beginning.

deb0ch commented 7 years ago

I actually copy paste the file from the repo into my elpa folder and restart Emacs.

Spacemacs is supposed to have a mechanism to load your own version of a package but it never seemed to work properly for me.

I'll try to reproduce consistently. One thing I have to check is that with the latest commit (which still gets the code closer to how it should work), one can request uninitialized entries from the window-vector. I have to make sure that that won't trigger bugs.

deb0ch commented 7 years ago

Hey hey !

This is finally merged, just made a new version with it ! :thumbsup::sparkles:

Thanks for the fix and reports !

I finally found the bug that I told you still happened from time to time (but not consistently) and just patched it (you can see it just after your PR in the commit history), it wasn't related with your PR.

Don't hesitate to send more in :construction_worker_man: