alphapapa / outshine

Org-mode for non-Org buffers
GNU General Public License v2.0
212 stars 23 forks source link

Add support for changing the ellipsis #82

Closed Maralbada closed 3 years ago

Maralbada commented 3 years ago

What

This PR adds support for changing the ellipsis text at the end of closed headings.

Why

This is not intuitive to do, so having a variable makes it cleaner and easier.

How

Changing the 'selective-display value in whatever display-map is active, and making that map local to the buffer.

Tested

Possible problems

alphapapa commented 3 years ago

I appreciate the diligence you've put into this PR. However, it's probably not a good idea. It would move Outshine toward being its own implementation of outline-mode or outline-minor-mode rather than being an extension of them. To the extent that we change Outshine, we want to simplify it and depend more on existing functionality.

Also, AFAIK this feature already exists in Org mode. For outline-mode or outline-minor-mode buffers, it would be better to extend them than put this functionality in Outshine.

What do you think? Thanks.

Maralbada commented 3 years ago

Thank you too for the work you do mantaining this awesome package. And sorry for opening a PR directly, it should have been an issue first.

If I undestand correctly, you mean that this feature should be implemented like in Org mode, and directly in outline-mode.

I did look into the Org implementation but didn't want to copy it, I can do it like that if you think it would be better.

About implementing it directly in ´outline-mode´, I personally prefer software modularized and I thought that the difference between Outline mode and Outshine mode is that Outline provides only the base and Outshine the Org-like look and feel on top of that. Now, this change is pretty isolated and cosmetic, I don't think it adds anything to the base logic of outlining, so it should be with the cosmetic part of the code. It could be implemented in the base package, but it doesn't need it to fulfill its role as a non-cosmetic outlining tool, at least in my opinion. It is also a feature that Org has, and I think a lot of people use, that I would like to have easy acces to in my pretty outlined buffers, not necessarily in any outlined buffer.

Thanks for considering the change. I'll be happy to modify it if you think it will work better in any other way.

alphapapa commented 3 years ago

Okay, I dug into the outline-mode code and the Elisp manual and, as I'm sure you did when you were working on this, tracked it down to the selective-display-ellipses option and the slot in the display table.

Having done that, it seems that the suggested justification for adding this feature to Outshine would be to consider Outshine as a kind of catch-all for outline-related features. To an extent, that may describe Outshine, and that might justify adding it. However, at the same time, that rather nebulous definition has made this package harder to maintain and develop. Even though I agreed to maintain it, I, myself, don't fully grasp all of its features, either intended ones or actual ones. Some of them are outdated, obsolete, or incompatible with changes made to Emacs and/or Org over the year. So I think it would be better if Outshine were to narrow its focus on a more specific set of features that can be more cleanly and reliably implemented. IOW, I think it should be less of a catch-all and more of a specific set of enhancements that couldn't reasonably be implemented upstream.

Regarding this PR, it seems like it could be easily implemented as its own minor-mode that modifies the display table according to user configuration. Then its minor-mode function could be added to whatever outline-related mode hook you're using. Then it could easily be used independently of Outshine.

A very simple minor mode like that would be more suitable for a kind of "grab bag" of simple, reusable code. For example, I maintain this repo that way: https://github.com/alphapapa/unpackaged.el So if you're interested in writing this as a minor mode, I'd welcome you to submit it there.

Even better would be if you were to submit a patch to outline-minor-mode that supported this feature directly. I'm guessing it would only take a few lines of code to enhance selective-display-ellipses to accept a string or character value and then set the display table accordingly when the outline mode is activated.

What do you think? Thanks.

Maralbada commented 3 years ago

I really didn't dig that deep into the problem, basically I adapted a working solution into Outshine mode, thank you however for taking the time and finding a better suited solution for implementing the change upstream.

I completely agree with the decision to keep the package focused on specific functionality, as I said, I just submitted the change here because of the focus on Org-like features.

I will look into sending the change upstream to outline-mode, this time I will get confirmation from the mantainers before sending the changes. Since the change will take time to arrive, if it happens at all, I will also submit a PR to the repo you mention as a temporary way to have the functionality easily available.

Thanks again for taking the time to respond properly and providing guiding points.

alphapapa commented 3 years ago

Sounds good, thanks.