alphapapa / burly.el

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

Docs: Emphasize that Burly bookmarks are standard Emacs bookmarks and can be managed with standard tools #14

Closed leonlan closed 3 years ago

leonlan commented 3 years ago

Hi,

Thanks for the wonderful package. I just encountered the use-case where I wanted to delete some bookmark since I wont need it anymore, so I thought it would be nice to have a delete bookmark function for burly as well.

Alternatively, you could implement something similar to the native bookmark list C-x r l. Would be really useful to have that as well I think.

Kind regards, Leon

alphapapa commented 3 years ago

Hi Leon,

Thanks for the kind words. I'm glad it's useful to you.

The bookmarks created by Burly are standard Emacs bookmarks, so you can manage them with standard bookmark commands like list-bookmarks and bookmark-delete. Packages like Helm and Ivy tend to make commands like bookmark-delete easier to use with enhanced completion.

Since standard bookmark commands can be used, I'd generally prefer to not add more bookmark commands to this package.

What do you think? Thanks.

leonlan commented 3 years ago

I think you could make a case for both.

I'm still an inexperienced Emacs user, so I couldn't directly infer from the README that I could use list-bookmarks or bookmark-delete to list/delete my burly bookmarks. It just happened to be that I occasionally use bookmarks and saw that burly bookmarks were also listed, which made me realize I could delete burly urls in list-bookmarks. Elaborating on this use case in the README could be potential solution, especially for users who are not familiar with the native bookmarks implementation.

My arguments for implementing a burly-list-bookmarks would also be a separation of concerns. My suggestion would then be: list-bookmarks should only contain (native) buffer configurations and burly-list-bookmarks should contain the burly window/frame configurations. In my limited experience using the package, I have yet to encounter a situation where I would need both buffer and frame/window configurations at the same time. Honestly I am not familiar with how much time it would be needed to extend Helm and Ivy to implement such commands, so maybe this would take too much time and then the first option would be much more feasible.

A third option would be to replace the native bookmarks altogether with burly. So then burly-list-bookmarks could contain buffer/window/frame configurations, and there could be a burly-bookmark-buffer option. Just a thought!

alphapapa commented 3 years ago

I'm still an inexperienced Emacs user, so I couldn't directly infer from the README that I could use list-bookmarks or bookmark-delete to list/delete my burly bookmarks.

This is mentioned in the readme: "Burly bookmarks can also be opened with standard Emacs bookmark commands." I suppose I could change "opened with" to something like "managed with" to clarify that.

Elaborating on this use case in the README could be potential solution, especially for users who are not familiar with the native bookmarks implementation.

I should probably emphasize that "Burly bookmarks" are just Emacs bookmarks and should be managed with Emacs bookmark commands. I don't want to implement Burly-specific commands for every kind of action that can be taken on a bookmark.

What's really needed is for the built-in list-bookmarks command to be extended to allow filtering. Drew Adams's bookmarks+ library provides many such features, but I think the built-in one ought to have filtering without a third-party library.

My suggestion would then be: list-bookmarks should only contain (native) buffer configurations and burly-list-bookmarks should contain the burly window/frame configurations.

You mentioned that you're new to Emacs, so I should clarify: there's no such thing as "buffer configurations." There are "window configurations", which refer to the layout of windows within a frame, and there are "framesets", which refer to a set of frames and the window configurations within them.

My suggestion would then be: list-bookmarks should only contain (native) buffer configurations and burly-list-bookmarks should contain the burly window/frame configurations.

It could be useful to have a way to list only Burly bookmarks. I don't want to reimplement list-bookmarks, but maybe we could rebind bookmarks-alist around the call to it so it only displayed bookmarks of our choosing. However, we would need to ensure that, e.g. if a user saved the bookmarks file from that buffer, that it didn't effectively erase all the other bookmarks by only saving the ones displayed. (All of this is, to a certain extent, using the bookmarks system beyond what it was designed for.)

Honestly I am not familiar with how much time it would be needed to extend Helm and Ivy to implement such commands, so maybe this would take too much time and then the first option would be much more feasible.

I'm not sure what you mean. Helm, for example, already has bookmark commands that work. e.g. the command helm-filtered-bookmarks can easily be used to manage Burly bookmarks, just type Burly: and it will only display Burly bookmarks, then you can delete, rename, etc.

Or if you're using helm-mode or ivy-mode, a command like bookmark-delete can be used with their completion/filtering to easily select the desired bookmark by name.

A third option would be to replace the native bookmarks altogether with burly. So then burly-list-bookmarks could contain buffer/window/frame configurations, and there could be a burly-bookmark-buffer option. Just a thought!

I'm not sure what you mean. Anyway, Burly is not intended to replace the bookmarks system. Burly builds on the bookmarks system. The bookmarks system enables Burly to restore buffers by making use of standard bookmarks features provided by major modes. Without the bookmarks system, Burly would have to implement support for every major mode on its own.

alphapapa commented 3 years ago

maybe we could rebind bookmarks-alist around the call to it so it only displayed bookmarks of our choosing. However, we would need to ensure that, e.g. if a user saved the bookmarks file from that buffer, that it didn't effectively erase all the other bookmarks by only saving the ones displayed. (All of this is, to a certain extent, using the bookmarks system beyond what it was designed for.)

Rebinding bookmarks-alist seems to work, and it doesn't affect the saving of the rest of the bookmarks in the global value of bookmarks-alist. However, after running a command, e.g. deleting a bookmark, when the bookmarks list is updated to reflect that, all bookmarks are displayed, not just the Burly ones. I've tried using buffer-local function advice to override bookmarks-bmenu-list so that only Burly bookmarks are redisplayed, but that doesn't work. At this point, it begins to feel very hacky, so it's probably not a good idea to try that route.

Probably a better idea would be to propose to upstream that list-bookmarks have an optional filter function that could be set buffer-locally, so the list could be filtered again upon refresh.

leonlan commented 3 years ago

I'm still an inexperienced Emacs user, so I couldn't directly infer from the README that I could use list-bookmarks or bookmark-delete to list/delete my burly bookmarks.

This is mentioned in the readme: "Burly bookmarks can also be opened with standard Emacs bookmark commands." I suppose I could change "opened with" to something like "managed with" to clarify that.

Elaborating on this use case in the README could be potential solution, especially for users who are not familiar with the native bookmarks implementation.

I should probably emphasize that "Burly bookmarks" are just Emacs bookmarks and should be managed with Emacs bookmark commands. I don't want to implement Burly-specific commands for every kind of action that can be taken on a bookmark.

Sounds good to me.

What's really needed is for the built-in list-bookmarks command to be extended to allow filtering. Drew Adams's bookmarks+ library provides many such features, but I think the built-in one ought to have filtering without a third-party library.

My suggestion would then be: list-bookmarks should only contain (native) buffer configurations and burly-list-bookmarks should contain the burly window/frame configurations.

You mentioned that you're new to Emacs, so I should clarify: there's no such thing as "buffer configurations." There are "window configurations", which refer to the layout of windows within a frame, and there are "framesets", which refer to a set of frames and the window configurations within them.

Thanks, when writing my message I wasn't really sure about the definitions so I just went along with 'buffer' configurations. I think I should have said 'single file bookmarks' instead of 'buffer configurations'.

My suggestion would then be: list-bookmarks should only contain (native) buffer configurations and burly-list-bookmarks should contain the burly window/frame configurations.

It could be useful to have a way to list only Burly bookmarks. I don't want to reimplement list-bookmarks, but maybe we could rebind bookmarks-alist around the call to it so it only displayed bookmarks of our choosing. However, we would need to ensure that, e.g. if a user saved the bookmarks file from that buffer, that it didn't effectively erase all the other bookmarks by only saving the ones displayed. (All of this is, to a certain extent, using the bookmarks system beyond what it was designed for.)

I don't really understand the technical details of this haha. But again, I would enjoy having a way to list only Burly bookmarks.

Honestly I am not familiar with how much time it would be needed to extend Helm and Ivy to implement such commands, so maybe this would take too much time and then the first option would be much more feasible.

I'm not sure what you mean. Helm, for example, already has bookmark commands that work. e.g. the command helm-filtered-bookmarks can easily be used to manage Burly bookmarks, just type Burly: and it will only display Burly bookmarks, then you can delete, rename, etc.

Sorry, I did not understand what you meant with your first comment on Helm/Ivy. But I do understand it now and it makes much more sense.

Or if you're using helm-mode or ivy-mode, a command like bookmark-delete can be used with their completion/filtering to easily select the desired bookmark by name.

A third option would be to replace the native bookmarks altogether with burly. So then burly-list-bookmarks could contain buffer/window/frame configurations, and there could be a burly-bookmark-buffer option. Just a thought!

I'm not sure what you mean. Anyway, Burly is not intended to replace the bookmarks system. Burly builds on the bookmarks system. The bookmarks system enables Burly to restore buffers by making use of standard bookmarks features provided by major modes. Without the bookmarks system, Burly would have to implement support for every major mode on its own.

Okay, makes sense! I think the point is clear that there's no real need for implementing a burly-specific bookmarks listing, but it would be a nice-to-have. A little bit elaboration on the README goes a long way (e.g. explicitly stating that Burly bookmarks can be managed with Emacs bookmark commands).

maybe we could rebind bookmarks-alist around the call to it so it only displayed bookmarks of our choosing. However, we would need to ensure that, e.g. if a user saved the bookmarks file from that buffer, that it didn't effectively erase all the other bookmarks by only saving the ones displayed. (All of this is, to a certain extent, using the bookmarks system beyond what it was designed for.)

Rebinding bookmarks-alist seems to work, and it doesn't affect the saving of the rest of the bookmarks in the global value of bookmarks-alist. However, after running a command, e.g. deleting a bookmark, when the bookmarks list is updated to reflect that, all bookmarks are displayed, not just the Burly ones. I've tried using buffer-local function advice to override bookmarks-bmenu-list so that only Burly bookmarks are redisplayed, but that doesn't work. At this point, it begins to feel very hacky, so it's probably not a good idea to try that route.

Probably a better idea would be to propose to upstream that list-bookmarks have an optional filter function that could be set buffer-locally, so the list could be filtered again upon refresh.

Okay that's unfortunate to hear, but thanks for trying! Do you think that proposing such optional filter has any chance of going through?

How about only implementing a burly-delete-bookmark?

alphapapa commented 3 years ago

Do you think that proposing such optional filter has any chance of going through?

It's hard to say. I think there's a lot of potential to extend and improve the bookmarks system. It would probably take a lot of back-and-forth with the maintainers to work out the enhancements in a satisfactory way.

If one were interested in that now, I'd probably suggest to first reimplement list-bookmarks using tabulated-list-mode. They'd probably consider that a worthy improvement. From there, it would probably be easier to make enhancements to that command.

I think I should have said 'single file bookmarks' instead of 'buffer configurations'.

Right, and to be clear, Emacs bookmarks are designed to be single-buffer bookmarks, anyway. This package repurposes them with a minor hack to allow them to restore more than a single buffer.

That's another potential improvement for the bookmarks system: removing the need for that little hack.

How about only implementing a burly-delete-bookmark?

I'd strongly prefer not to go down that route. If I do one such command, someone else will ask for a rename command, etc. And I don't want users to think of Burly bookmarks as if they were separate from Emacs bookmarks. I want to emphasize that they are regular Emacs bookmarks, so the whole world of existing bookmarks tools work with them.

And I don't think adding such a command would be that useful, anyway. If you really need such a command, here, I replaced bookmark-jump with bookmark-delete. But I don't want to add it to this package.

(defun leon/burly-delete-bookmark (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 "Delete Burly bookmark: " bookmark-names
                nil nil burly-bookmark-prefix))))
  (cl-assert (and bookmark (not (string-empty-p bookmark))) nil
             "(burly-delete-bookmark): Invalid Burly bookmark: '%s'" bookmark)
  (bookmark-delete bookmark))

I think the point is clear that there's no real need for implementing a burly-specific bookmarks listing, but it would be a nice-to-have.

Yeah, I'll probably not implement that, because I know if I do, I'll get bug reports about the list not being filtered after being updated, etc. I'd rather not do it if I can't do it properly.

A little bit elaboration on the README goes a long way (e.g. explicitly stating that Burly bookmarks can be managed with Emacs bookmark commands).

I'll see if I can improve that.

Thanks for your feedback!

alphapapa commented 3 years ago

If one were interested in that now, I'd probably suggest to first reimplement list-bookmarks using tabulated-list-mode. They'd probably consider that a worthy improvement. From there, it would probably be easier to make enhancements to that command.

FYI, they actually did that recently: https://lists.gnu.org/archive/html/emacs-devel/2020-11/msg00148.html

alphapapa commented 3 years ago

@leonlan Please let me know what you think about the documentation now. Thanks for your help.

leonlan commented 3 years ago

Looks good! Thank you :smile: