casouri / vundo

Visualize the undo tree.
414 stars 21 forks source link

Replace vundo-ascii-mode with user option #1

Closed phikal closed 2 years ago

phikal commented 3 years ago

Hi, very interesting package!

One thing that disturbed me was that it used unicode charachters by default. Skimming through the code I noticed that you wanted this to be changed using a global minor mode. IMO this is a bit too drastic, so I changed a few things so that this can be configured using a regular user option.

I also went a bit further and rewrote some of the internals in vundo--draw-tree. I hope it is OK if I submit all of this in one pull request.

casouri commented 3 years ago

Hi, this looks cool. However, I still prefer the alist approach over many custom options plus a utility function to insert various parts of the tree.

phikal commented 3 years ago

Ok, if you say so, but just to be on the safe side, every minor mode is also a user option and I kept a function to insert parts of the tree.

bigodel commented 2 years ago

now how about making it a local minor mode, instead? defining a globalized version of it would be trivial, and it would work the same way, only buffer-wise

phikal commented 2 years ago

What would that change? The proposition here was to replace direct character mapping with a user option that maps tree structure names to characters used to construct a tree. Using a global or a local minor mode is a lesser difference.

casouri commented 2 years ago

Let make vundo-translation-alist a custom variable, how about that? Tho we probably need to change char-to-char mapping to string-to-string mapping, otherwise Customize just shows numbers.

casouri commented 2 years ago

Something like this:

Screen Shot 2021-11-24 at 9 45 06 AM
phikal commented 2 years ago

Tho we probably need to change char-to-char mapping to string-to-string mapping, otherwise Customize just shows numbers.

Are you sure, (elisp) Simple Types defines character as a customisation type that could be used to define the alist.

My objection remains that from a user-standpoint, defining a mapping from unicode to something else seems unclean. For my own sensibilities, an alist mapping symbols designating tree parts to strings or characters appears preferable, especially when you want to type it out yourself in an init.el.

casouri commented 2 years ago

Are you sure, (elisp) Simple Types defines character as a customisation type that could be used to define the alist.

I'm pretty sure. In fact, I just checked and it displays characters as numbers even if I use :type (alist character).

My objection remains that from a user-standpoint, defining a mapping from unicode to something else seems unclean. For my own sensibilities, an alist mapping symbols designating tree parts to strings or characters appears preferable, especially when you want to type it out yourself in an init.el.

I don't disagree. Ok, alist from symbols to strings then. Docstring should be enough to inform users of which symbol maps to which part of the tree.

phikal commented 2 years ago

I've updated the pull request, replacing vundo-use-symbols with vundo-symbol-alist. Using character as the did work (it might have been that you didn't specify a :value-type in your experiment?). It might be that something is still broken, because I don't have access to a Emacs 28 installation right now, but I'll test and fix anything later today, in case I did mess something up.

casouri commented 2 years ago

Thanks, before I merge it, have you assigned copyright assignment for Emacs before? If you haven't, would you like to? I might want to add vundo to ELPA in the future, so I want to make sure every contributor have assigned the copyright assignment.

phikal commented 2 years ago

Yes, don't worry about that.

casouri commented 2 years ago

Merged with some change, most notably I changed vundo-symbol-alist to vundo-glyph-alist. If you have strong opinions on that, we can discuss about it ;-) Thanks.

phikal commented 2 years ago

That is all fine, I didn't put any effort into choosing a name.