nex3 / perspective-el

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

Merge and unmerge perspective buffer lists. #174

Closed NicholasBHubbard closed 2 years ago

NicholasBHubbard commented 2 years ago

This PR adds functionality for intelligently merging and unmerging the buffer lists of two perspectives. I have wanted this ability since I started using perspective but when I tried to implement it I quickly found that naive solutions to this problem did not provide adequate functionality. I decided to implement the exact behavior that I envisioned and I believe that this could be useful to other people.

This PR implements two interactive functions:

Here are some important edge cases that I have addressed:

gcv commented 2 years ago

Very interesting! I'll have to review the code in more detail later. Can you give me an example situation in which you use this functionality (especially the unmerge part)?

NicholasBHubbard commented 2 years ago

Sure! Say I am writing a library for my project. I have two perspectives open, one of the main project (the current perspective) and one for the library. Say all of a sudden I need to change something in the library I can quickly merge in the libraries perspective, change some code, then unmerge the library so the its buffers don't get in the way of the main project.

gcv commented 2 years ago

That sounds a lot like the yak shave example in the readme. What's the benefit of doing merge-unmerge in your example rather than just switching to the library perspective? It sounds like you still want to reference buffers from the app project..?

NicholasBHubbard commented 2 years ago

In case it wasn't clear merging is one directional meaning if I merge A into B, B's buffers are NOT available in A (thought I would clear that up).

Yak shaving is useful for working on multiple projects that are largely unrelated. Merging is useful for working on multiple projects that are very much related, to the point that you want to be able to look at buffers from both perspectives at the same time.

The example I gave is actually exactly what I have been using merging for. I am working on a project that has lots of auxiliary functionality factored out into libraries. I want to be able to work on these libraries individually in their own perspectives but I also frequently need to reference them from the main project so I merge them in while I work for a few hours/days and then when I am done I unmerge them. I find that switching back and forth between the libraries perspective and the main project perspective is taxing because I am constantly needing to switch contexts, when really I am currently working on the library in the context of the main project perspective.

Alternatively you could just persp-add-buffer the buffers you need access to and then persp-remove-buffer them later when you are done, but I have found that in practice this is inconvenient especially when you start adding buffers from multiple perspectives. When you start adding buffers from multiple perspectives you lose track of what buffers belong to what perspective and it becomes a mess.

To sum up I would say that perspective merging/unmerging is useful for combining perspective buffer lists while keeping a clear distinction between which buffers belong to which perspectives.

NicholasBHubbard commented 2 years ago

I made some code style changes to better match perspectives style before I submitted the PR and I found some minor typo related bugs that I fixed with e98eb8f and c04bdcc.

gcv commented 2 years ago

Makes sense. Please add that example to the Sample Use Cases section in the README.

I haven’t had time to review the code yet, will try to do it in the next couple of days. Heads up: I’ll be looking out for compatibility problems with existing persp-state-* files. Since you changed that data structure, I want to make sure existing save files load cleanly.

NicholasBHubbard commented 2 years ago

Thanks for the feedback. I will work on these fixes in the coming days.

gcv commented 2 years ago

Upon reflection, I think using a frame parameter for persp-merge-list will be the least painful approach. It simplifies cleanup when a frame is deleted.

NicholasBHubbard commented 2 years ago

I implemented persp-merge-list as a frame parameter and undid my changes to perspective states. I did not add functionality for merging perspectives from different frames. I personally only use 1 frame so I don't know how multi-frame users work with perspective and if they would want this feature.

I believe that it is valuable to save the merge state and I would not be comfortable merging (no pun intended) this PR as is. It seems that the implementation of persp-state-{save,load} prohibits a way to implement saving the merge state without breaking backwards compatibility. At least I can not figure out a way. Is there a way you can think of?

If it is not possible then how would you feel about creating functions persp-state-{save,load}-with-merge-state and a variable persp-state-save-save-merge-state that persp-state-{save,load} checks so the user has the option to break backwards compatibility? This seems like a good idea to me but I am not sure what your views are on this kind of change.

Otherwise I can come up with a solution for unmerging all merges before saving the state which I think is better then putting the user in a situation where they cannot undo merges when loading a previous state.

gcv commented 2 years ago

You’re right, I never took into account versioning the persp-state-* on-disk data structure. I wish I did. :(

The only solution I can think of is to catch the error at read-time (in persp-state-load where state-complete is bound), then alter the serialized string so it reads cleanly. One approach: define a struct called persp--state-frame-v1 with the old slots, replace the definition string to use it before calling read, and then transform the data structure to the new version as needed.

I’m also fine with unmerging all perspectives before saving. That seems like a reasonable way to avoid dirty backwards-compatibility code, as long as it’s documented in the README and with a comment in the code explaining why unmerging happens before save. That has the additional advantage of maintaining read-compatibility with older versions of Perspective.

NicholasBHubbard commented 2 years ago

I added support for saving the merge list with persp-state-{save,load} that maintains backwards compatibility.

I did this by creating a new struct type called persp--state-frame-v2 that has a slot for the merge-list. I then wrote a function persp--state-complete-v2 that takes a persp--state-complete and if it is compatible with the new V2 state it does nothing but if it get the old type of state it coerces it into a V2 compatible state. This function is then called on the data structure read from the state file in persp-state-load.

This passed all my checks but let me know what you think. Do note that I am not experienced with working with multiple frames so I may have missed something in this aspect.

NicholasBHubbard commented 2 years ago

I have had trouble getting the state to load with multiple frames. It seems that wherever I try to reset the old persp-merge-list frame parameter in persp-state-load it does not properly load. I am just letting you know so you don't bother testing the state loading at this point until I can take more time to figure this out.

The state saving on the other hand does work!

gcv commented 2 years ago

Okay, sounds good. FWIW, I tried loading state with one frame, and it worked fine. The backwards compatibility for older versions of the state file also looks good. Nice work! Let me know when you want me to take another look.

NicholasBHubbard commented 2 years ago

This should be good to go now.

gcv commented 2 years ago

Looks good. I made one change, to the the default persp-unmerge keybinding. C-m is the same as RET, which (1) is kind of strange, and (2) looks bad in which-key. I made it u instead.

Thank you for making such a nice contribution to Perspective!