alphapapa / burly.el

Save and restore frames and windows with their buffers in Emacs
GNU General Public License v3.0
301 stars 14 forks source link

Doesn't restore single-window frames correctly #15

Closed tpeacock19 closed 3 years ago

tpeacock19 commented 3 years ago

Burly does not seem to open any buffers that are derived from prog-mode unless the file is already open. I'm unsure how to debug further because there are no errors thrown, it just takes me to the scratch buffer.

The unhexed burly url is:

;; => "(((min-height . 4) (min-width . 10) (min-height-ignore . 3)
;; (min-width-ignore . 4) (min-height-safe . 1) (min-width-safe . 2)
;; (min-pixel-height . 84) (min-pixel-width . 90)
;; (min-pixel-height-ignore . 45) (min-pixel-width-ignore . 34)
;; (min-pixel-height-safe . 21) (min-pixel-width-safe . 18)) leaf
;; (pixel-width . 1860) (pixel-height . 997) (total-width . 207)
;; (total-height . 47) (normal-height . 1.0) (normal-width . 1.0)
;; (parameters (better-jumper-struct . #s(better-jumper-jump-list-struct
;; (0 0 . [nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil
;; nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil
;; nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil
;; nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil
;; nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil
;; nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil])
;; -1)) (burly-url
;; . \"emacs+burly+bookmark://main.go?filename=%22~%2Fsrc%2Fdev%2Fgo%2Fexercises%2Fmain.go%22&front-context-string=%22%0A%2F%2F%20See%20page%2043.%22&rear-context-string=%22s%2Fby-nc-sa%2F4.0%2F%0A%22&position=125&defaults=%28%22main.go%22%29\")) (buffer \"main.go\" (selected . t) (hscroll . 0) (fringes 8 8 nil nil) (margins nil) (scroll-bars nil 0 t nil 0 t nil) (vscroll . 0) (dedicated) (point . 125) (start . 1)) (prev-buffers (\"*scratch*\" 1 1) (\"authors::notes.org\" 2429 2431)))"

The burly--bufferize-window-state output is:

;; =>
;; (((min-height . 4)
;;   (min-width . 10)
;;   (min-height-ignore . 3)
;;   (min-width-ignore . 4)
;;   (min-height-safe . 1)
;;   (min-width-safe . 2)
;;   (min-pixel-height . 84)
;;   (min-pixel-width . 90)
;;   (min-pixel-height-ignore . 45)
;;   (min-pixel-width-ignore . 34)
;;   (min-pixel-height-safe . 21)
;;   (min-pixel-width-safe . 18))
;;  leaf
;;  (pixel-width . 1860)
;;  (pixel-height . 997)
;;  (total-width . 207)
;;  (total-height . 47)
;;  (normal-height . 1.0)
;;  (normal-width . 1.0)
;;  (parameters
;;   (better-jumper-struct . #s(better-jumper-jump-list-struct (0 0
;;   . [nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil
;;   nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil
;;   nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil
;;   nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil
;;   nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil
;;   nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil nil])
;;   -1))
;;   (burly-url . "emacs+burly+bookmark://main.go?filename=%22~%2Fsrc%2Fdev%2Fgo%2Fexercises%2Fmain.go%22&front-context-string=%22%0A%2F%2F%20See%20page%2043.%22&rear-context-string=%22s%2Fby-nc-sa%2F4.0%2F%0A%22&position=125&defaults=%28%22main.go%22%29"))
;;  (buffer
;;   "main.go"
;;   (selected . t)
;;   (hscroll . 0)
;;   (fringes 8 8 nil nil)
;;   (margins nil)
;;   (scroll-bars
;;    nil
;;    0
;;    t
;;    nil
;;    0
;;    t
;;    nil)
;;   (vscroll . 0)
;;   (dedicated)
;;   (point . 125)
;;   (start . 1))
;;  (prev-buffers
;;   ("*scratch*" 1 1)
;;   ("authors::notes.org"
;;    2429
;;    2431)))
alphapapa commented 3 years ago

I can't reproduce this problem. Here's what I did:

  1. Run Emacs in a clean configuration with makem.sh: ./makem.sh --sandbox interactive.
  2. Install go-mode.
  3. M-x find-file RET /tmp/test.go RET.
  4. Type // foo bar.
  5. C-x C-s.
  6. C-x d RET to open a Dired buffer.
  7. C-x 3 to split the window.
  8. C-x b RET to switch the new window back to test.go.
  9. M-x burly-bookmark-windows RET test RET to save the new Burly bookmark.
  10. Kill both buffers, test.go and the Dired buffer.
  11. C-x 1 to show only the *scratch* buffer.
  12. C-x r b TAB RET to open the Burly: test bookmark.

Result: Both test.go and the Dired buffer were restored side-by-side.

I'm guessing that something in your configuration is affecting this, or perhaps a change in your newer Emacs version (IIRC you're running a snapshot version?).

Thanks.

tpeacock19 commented 3 years ago

I think it must be something. I experience it intermittently but probably premature to create an issue. I'll keep digging and see if I can get more information.

Thanks for taking a look.

alphapapa commented 3 years ago

Thanks. Please let me know if you find a way to reproduce it.

tpeacock19 commented 3 years ago

Hello again. I think I have determined what is missing in the links that do not work for me.

(burly--bufferize-window-state state)
;; =>
(...
hc
 (pixel-width . 1860)
 (pixel-height . 997)
 (total-width . 207)
 (total-height . 47)
 (normal-height . 1.0)
 (normal-width . 1.0)
 (combination-limit)
...)

I'm having trouble determining what this refers to in the burly code. Do you have any idea what it is?

tpeacock19 commented 3 years ago

Actually. I believe this may even be simpler than that. I think its a result of attempting to save a window configuration with a single window. Which I imagine is not the use case for burly, but would be nice to have.

alphapapa commented 3 years ago

Actually. I believe this may even be simpler than that. I think its a result of attempting to save a window configuration with a single window. Which I imagine is not the use case for burly, but would be nice to have.

Ah, I guess this is a lack of foresight on my part. Burly isn't necessary to bookmark single windows/buffers, because plain Emacs bookmarks do that (i.e. C-x r m), so I didn't think about what would happen if a user used burly-bookmark-windows when the frame only has one window.

In that case, I should probably either show a message directing users to use a normal bookmark, or call the command bookmark-set directly, perhaps showing an advisory message afterward.

What do you think? Thanks.

tpeacock19 commented 3 years ago

Well the only reason I came across this was because I was attempting to save windows in each of my tabs before shutting emacs down

(defun burly-save-tab-workspaces ()
    (interactive)
    (when (< 1 (length (tab-bar-tabs)))
      (let ((current-tab (alist-get 'name (tab-bar--current-tab))))
        (burly-bookmark-windows (format "Burly: %s" current-tab))
        (cl-loop for i in (-map (lambda (tab)
                                  (alist-get 'name tab ))
                                (tab-bar--tabs-recent))
                 do (tab-bar-switch-to-tab i)
                 (burly-bookmark-windows (format "Burly: %s" i)))
        (tab-bar-switch-to-tab current-tab))))

However, I dont think it will often be the case that burly-bookmark-windows will be called in a non-interactive fashion. So in that case a warning/suggestion to use `bookmark-set directly should suffice.

alphapapa commented 3 years ago

Hm, that's very interesting. I haven't upgraded to Emacs 27.1 yet for my main config, so I haven't experimented much with tab bars, other than Bufler's workspace-tabs mode. Maybe Burly should have explicit support for tabs, although I'm not sure yet what that would entail.

However, I dont think it will often be the case that burly-bookmark-windows will be called in a non-interactive fashion.

Probably not, but it is intended to support non-interactive calls, for this very reason, so users can build on it.

So in that case a warning/suggestion to use `bookmark-set directly should suffice.

If that were the case, wouldn't this function of yours still fail to work properly (or the single-window bookmarks created by it would fail to restore properly)? If so, I guess that warning would be insufficient, and either it needs to call bookmark-set directly, or it needs some other fix to handle single-window frames.

WDYT? Thanks.

tpeacock19 commented 3 years ago

If that were the case, wouldn't this function of yours still fail to work properly (or the single-window bookmarks created by it would fail to restore properly)?

Yes it would still fail. I've just updated it to exclude single-window tabs.

(defun baal-burly-save-tab-workspaces ()
    (interactive)
    (when (< 1 (length (tab-bar-tabs)))
      (let ((current-tab (alist-get 'name (tab-bar--current-tab))))
        (when (< 1 (count-windows))
          (burly-bookmark-windows (format "Burly: %s" current-tab)))
        (cl-loop for i in (-map (lambda (tab)
                                  (alist-get 'name tab ))
                                (tab-bar--tabs-recent))
                 do (tab-bar-switch-to-tab i)
                 (when (< 1 (count-windows))
                   (burly-bookmark-windows (format "Burly: %s" i))))
        (tab-bar-switch-to-tab current-tab))))

However, the main reason I suggested the warning is because it would have given me notification that it does not work with single-window configurations. If it called bookmark-set, I don't think the user would notice. Though I guess that could be satisfied with both. Largely it depends on how much of "workspace manager" or a "window-config manager" you want Burly to be.

Regarding explicit support, I do think an option like burly-enable-workspaces and burly-workspace-function defaulting to the built-in tab-bar mode for Emacs 27.1 could be useful. Also would provide customization for other workspace managers without forcing you to integrate them in Burly itself.

alphapapa commented 3 years ago

However, the main reason I suggested the warning is because it would have given me notification that it does not work with single-window configurations. If it called bookmark-set, I don't think the user would notice. Though I guess that could be satisfied with both. Largely it depends on how much of "workspace manager" or a "window-config manager" you want Burly to be.

Yeah. I'll have to give it a little thought, and maybe experiment with single-window frames. Maybe there's a simple fix that I can make to the code.

Regarding explicit support, I do think an option like burly-enable-workspaces and burly-workspace-function defaulting to the built-in tab-bar mode for Emacs 27.1 could be useful. Also would provide customization for other workspace managers without forcing you to integrate them in Burly itself.

Could you elaborate on this idea? What would burly-enable-workspaces and burly-workspace-function do? How would other workspace managers use them?

tpeacock19 commented 3 years ago

I'm thinking of something like the below. I do not have in depth experience with other managers aside from a short stint with persp-mode but I imagine all of them have a create-workspace function that may or may not take a name.

(defvar burly-enable-workspaces t)

(defun burly-workspace-function #'burly-default-tab-bar-function)

(defun burly-default-tab-bar-function (bookmark)
  ;; maybe a confirmation here with y-or-n-p
  (tab-bar-new-tab)
  (tab-bar-rename-tab (string-remove-prefix "Burly: " bookmark)))
(defun burly-open-bookmark (bookmark)
  "Restore a window configuration to the current frame from a Burly BOOKMARK."
  (interactive
   (let ((bookmark-names (cl-loop for bookmark in bookmark-alist
                                  for (_name . params) = bookmark
                                  when (equal #'burly-bookmark-handler (alist-get 'handler params))
                                  collect (car bookmark))))
     (list (completing-read "Open Burly bookmark: " bookmark-names
                            nil nil burly-bookmark-prefix))))
  (cl-assert (and bookmark (not (string-empty-p bookmark))) nil
             "(burly-open-bookmark): Invalid Burly bookmark: '%s'" bookmark)
+  (when burl-enable-workspaces
+   (funcall burly-workspace-function bookmark))
  (bookmark-jump bookmark))
alphapapa commented 3 years ago

Thanks, that helps. So, basically, I think it boils down to restoring a set of windows, not just into the current frame, but also into a new tab, and naming that tab after the bookmark, right?

And maybe some kind of "hibernate tab" command (naming things is hard) that would bookmark the tab's windows and buffers and then close them. That's a feature I've been thinking about, though not in terms of tabs. Sort of like how Web browsers can bookmark all of the tabs in a window and then restore them.

tpeacock19 commented 3 years ago

So, basically, I think it boils down to restoring a set of windows, not just into the current frame, but also into a new tab, and naming that tab after the bookmark, right?

Yes that's right. Although it might also be worth checking if a tab with that name exists and warn of overwriting and use that instead.

And maybe some kind of "hibernate tab" command (naming things is hard) that would bookmark the tab's windows and buffers and then close them

hibernate, even stow-tab I think could work, regardless I think this would be a neat feature. You may run into some issues if there is overlap of buffers between tabs/workspaces.

alphapapa commented 3 years ago

Sounds good. Thanks for your help thinking about this idea.

alphapapa commented 3 years ago

@tpeacock19 The bug with restoring one-window frames should be fixed now. For the other ideas discussed here, I'll copy or link them in the relevant issue. Thanks for your help!

FYI, the bug is fixed by making Burly correctly handle one-window frames (which is necessary for restoring one-window frames in the frameset commands). It does not call or warn about other bookmark commands.

tpeacock19 commented 3 years ago

This is great! I'll have to take a look at the code to see if I can really understand it and maybe I can contribute to projects rather than just identify bugs. Thanks for handling this