-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option to show open+close context for punctuation #5
Conversation
Thanks so much for adding this feature! This is mostly working for me, and I think it’s a great addition. Only a few open+close pairs so far that I'd expect to work which are not working for me (I'm listing them here using AGL glyph names just so they’re easier to read): More questions: –Should the open+close context continue into the spacing string, eg: HH[A]HOHO[A]OO ? I would expect for this to happen, but currently it does not. What do you think? Thanks! |
Thanks for your review!
🤦♂️ Oops, haha, those were all working until I tried to be smart and add a comprehensive list.
You’re right, a "start with mirrored" option should start with mirrored context. However, 1) the open+close context is more important for actual usage, and 2) the open+close context only shows up sometimes and I really want to see it when it does. So, I have moved open+close to be first and adjusted the option labels to not include "start with." Do you think this works, or do you feel that it actually is better to put the mirrored context first?
I could be convinced, but at first, I actually don’t think so. For me, the purpose of the spacing string is to make sure that for pair Questions:
|
Awesome, this looks great. I just made one change to the order of the pairs, so the /quoteright /quoteright and /quotedblright /quotedblright pairs come later than the /quoteleft /quoteright and /quotedblleft /quotedblright. So now when both show up the left right pair comes first, which makes more sense for the way I kern things, hope that’s OK with you.
Yes, I think this is great.
OK, that’s fine to leave as is for the time being. I’ve got to use it more to know if this would be important to me or not, and if it would maybe warrant another check box, if it’s not the default. Hmm.
This was indeed a typo, I’ve fixed it now.
This seems to be working for me and I’m not seeing any performance issues on my end, so I’m fine with having it in there unless we encounter any issues.
This is an excellent question, and something I’ve been thinking about. I'll move this to a new issue here: I'll add my small changes and I think this could be good for a next release, unless you have any other changes/additions for the open+close feature. Thanks again! |
Awesome, thanks!
Good thinking! Same here. Thanks.
Yeah, that might be a good idea. I have also wondered if there might be an easy way to allow people to control their context strings. Then again, I am unsure at what point we might be pointlessly duplicating features from MM. It looks like this is what you posted about in issue #6, so I’ll comment there.
I think this is at a good spot to release! Thanks for your quick reviews and help. |
Adds option to start the spacing string with open+close punctuation, if either the
left
orright
character would logically have an associated open/close character. For example:I got what I think is a full list of open/close punctuation from here and here, then did some editing to make these into strings, and some simple python to make them into dictionary entries:
non-collapsed list to show open+close matches (Click to expand)
(I’m guessing there might be a more-clever way to do this with the Python Unicode module, but as it was, these had to be manually sorted a bit to make the correct open+close pairs.)
To be honest, I’m unsure of whether the vertical-specific characters should be included in here – I think someone would need a custom Space Center for vertical type, anyway. But, I figure it’s probably better to ere on the side of inclusivity rather than leaving things out, so I’ve left them in. Let me know if you feel that the list should be different!
Also, this PR removes the following unneeded code (the checkbox is false by default):
PS: Nice improvement for adding the context option text! I tried to carry that forward with this addition. Feel free to fix anything here you see fit, obviously, or let me know what could be done better. Thank you!