nex3 / perspective-el

Perspectives for Emacs.
MIT License
880 stars 71 forks source link

Add commands to add/remove marked buffers to/from persp via ibuffer #162

Closed meliache closed 1 year ago

meliache commented 3 years ago

See discussion in #54.

This allows to easily add and remove multiple buffers to/from a perspective. These functions have to be called from an ibuffer-buffer, after some buffers have been marked (e.g. via m).

I hope I have done the declare-function, the error-raising, autoloading etc. correctly. I'm used to writing elisp for my own emacs config, but haven't done much coding for packages yet, so these are things new to me, but I just tried to do the same as the existing functions.

Potential TODO's

gcv commented 3 years ago

I tried your change, it looks good. I'm okay with shipping this without keybindings and just a README note.

With regard to updating the ibuffer buffer after removing buffers from the perspective, I tried this modification to your code, and it seems to work:

(defun persp-ibuffer-remove-marked-buffers ()
  "Remove all marked buffers within ibuffer from the current perspective."
  (interactive)
  (unless (featurep 'ibuffer)
    (user-error "IBuffer not loaded"))
  (defvar ibuffer-maybe-show-predicates)
  (declare-function ibuffer-get-marked-buffers "ibuffer.el")
  (declare-function ibuffer-update "ibuffer.el")
  (dolist (buffer (ibuffer-get-marked-buffers))
    (persp-remove-buffer buffer))
  (let ((ibuffer-maybe-show-predicates (append ibuffer-maybe-show-predicates
                                               (list #'persp-buffer-filter)
                                               ido-ignore-buffers)))
    (ibuffer-update nil t)))

It's a bit ugly because it copy-pastes the ibuffer-maybe-show-predicates binding from persp-ibuffer. Maybe a persp--ibuffer-maybe-show-predicates-helper function could be added that returns the necessary list, and then persp-ibuffer and persp-ibuffer-remove-marked-buffers could both use it for the binding. I'm open to hearing about a nicer way to refactor it.

gcv commented 3 years ago

I just realized my code will enforce persp-ibuffer behavior even when used in non-perspective-specific ibuffer. :(

There are ways to hack around this. For example: have persp-ibuffer set persp--ibuffer-called-with-persp flag in a buffer-specific variable on the *IBuffer* buffer, and then check that flag from persp-ibuffer-remove-marked-buffers.

gcv commented 3 years ago

Let's do one other thing to reorganize the code a bit. Let's take the entire "ibuffer filter group code" section along with your new code, rename it to "ibuffer integration", and move it all next to the "Helm integration section" (either before or after, doesn't matter). That way all "integration" code sits together. Then move the persp-ibuffer function into the new section. Feel free to do that as part of this PR.

Let me know when you finish any other refactoring you plan to tackle for the purposes of this PR, and I'll take another look and merge.

meliache commented 1 year ago

Sorry for ghosting this PR for so long, back then I got busy with other things and then also stopped using perspective.el because I found it to not be necessary for my workflow. If you still feel like the functionality that I suggested in this PR as a way to address #54 via ibuffer is useful, please implement it in a separate PR, though feel free to reuse the code. Though looking at the diff the so far added code in this PR was very minor anyway.