emacs-citar / citar

Emacs package to quickly find and act on bibliographic references, and edit org, markdown, and latex academic documents.
GNU General Public License v3.0
502 stars 54 forks source link

Review `citar--make-indicator-symbols` #764

Closed bdarcus closed 1 year ago

bdarcus commented 1 year ago

@aikrahguzar - you originally wrote citar--symbol-strings, which I adapted to this new function.

When you get a chance, can you please review to ensure I have the details right?

It works, but I don't quite understand the alignment stuff.

You can see what it's doing in terms of output like:

ELISP> (citar--make-indicator-symbols "abc has:notes")

Perhaps we can add some more code comments to clarify for the next time?


https://github.com/emacs-citar/citar/blob/0c6a8038e9b7f72c2e6837bd3342eab942c08d0d/citar.el#L868-L888

aikrahguzar commented 1 year ago

The problem is that icons are not fixed width and generally that makes alignment very hard. The way around that is to use specified space in the display text property. The details are in the info node (elisp) Specified Space.

But basically we say that the last character of the padding should be replaced by a space which stretches to the position we want to. So for example if the icon has string width 1 but occupies 0.9 the space will stretch to occupy 1.1 and if the icon occupies 1.5 it will shrink to occupy 0.5.

The caveats are: 1) One will get a surprise if the last character of padding is not a space. 2) There will again be a surprise if the padding is an empty string. 3) If the icon has string width n but occupies more than n+1 cells the alignment will persist but I have only seen n to be 1 or 2 so I think being off by more than one cell is unlikely.

Emacs 29 has vtable which should allow us to do alignment properly but before that this seems like an acceptable hack.

bdarcus commented 1 year ago

So is my adaptation correct (for now)?

No rush if you haven't had time to determine.

You might also look at the citar-indicator defstruct. I made some assumptions that are probably wrong, and so may need to tweak that a little.

I do know that both default text indicators, and all-the-icons ones align correctly for at least three of us!

bdarcus commented 1 year ago

Oh, I also submitted a doom biblio PR based on this :-)

aikrahguzar commented 1 year ago

So is my adaptation correct?

It looks as correct to me as the last version I wrote which is only correct up to the caveats I mentioned.

bdarcus commented 1 year ago

Thank you!

bdarcus commented 1 year ago

I added an extensive code comment, just an edited version of your first message, in this commit.

https://github.com/emacs-citar/citar/pull/763/commits/3bb0c403eade0aa04b95f453474a05166bd797fe

bdarcus commented 1 year ago

On vtable, here's a really cool post!

https://lars.ingebrigtsen.no/2022/04/13/more-vtable-fun/