Skip to content
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

Update MM2SpaceCenter.py #18

Closed
wants to merge 1 commit into from

Conversation

weiweihuanghuang
Copy link

  • Added "Any language" option
  • Deduplicated variable names pair2charString string vs pair2char function
  • Added function to search a string despite suffixes and add suffix to word list (e.g. for a pair such as "l.ss01" "o", convert to "l" and "o" then return list of words and replace "lo" with "/l.ss01/o "

- Added "Any language" option
- Deduplicated variable names pair2charString string vs pair2char function
- Added function to search a string despite suffixes and add suffix to word list (e.g. for a pair such as "l.ss01" "o", convert to "l" and "o" then return list of words and replace "lo" with "/l.ss01/o "
@cjdunn cjdunn changed the base branch from master to next-master March 16, 2022 23:08
@cjdunn cjdunn changed the base branch from next-master to master March 16, 2022 23:08
@cjdunn
Copy link
Owner

cjdunn commented Mar 16, 2022

Hey @weiweihuanghuang thanks so much for these updates! I've been (slowly) making updates to this extension on the NextMaster branch, so I just added your changes on a branch off of that code so it will merge without a conflict. Please take a look at the NextMaster branch, with your changes added:
https://github.com/cjdunn/MM2SpaceCenter/tree/next-master

Unless you see any issues, I'll plan to merge your updates along with the previous ones I've made in to the Master branch. Glad to see you like the extension and thank for the motivation to keep working on this!

@arrowtype
Copy link
Contributor

This is awesome! Thanks for solving this feature, @weiweihuanghuang. The "Any Language" addition is also really nice.

I have just found a small issue, though:

The "Show mirrored pair (LRL)" option seems to not be working in the latest next-master version.

This feature is pretty important to me... I suppose I could probably try to make a PR into next-master to fix it! Unless, Wei, since the code is fresh in your mind, do you have an idea why the mirrored pair might currently be unresponsive? Is it working on your end?

@arrowtype
Copy link
Contributor

...sorry, false alarm! When I clone the branch and run the script directly, the mirror pair works fine. Don’t know why it wasn’t working initially... maybe I needed to restart RoboFont or something after re-installing the extension.

(Clever to collapse the mirrored pair into just one option, too!)

@weiweihuanghuang
Copy link
Author

Awesome thanks for merging this! I’ll run the latest one here and see if there’s any issues!

@arrowtype
Copy link
Contributor

Ahhh okay, I was too quick to dismiss the issue. The issue happens if I try to show both the open+close context and the mirrored pair. I’m not yet sure if it’s a regression in this specific set of changes, but it seems as if the open+close context is somehow overriding the mirrored pair. I’ll dig a little bit in the code to see if there’s an obvious fix!

@arrowtype
Copy link
Contributor

Aha! I think I got it: you’ve handled the spacingString in a nice way, with open+close context in the middle of the general text, but those lines exclude the mirroredPair.

text = spacingString + previousText

A not very pretty, but functional way of handling this is

if self.w.mirroredPair.get() == True: #if "start with mirrored pair" is checked, add this to text
    text = self.pairMirrored(self.pair) + spacingString + previousText
else:
    text = spacingString + previousText

I’ll make a quick PR with that adjustment, as it seems to be working now.

@arrowtype arrowtype mentioned this pull request Mar 17, 2022
@weiweihuanghuang
Copy link
Author

weiweihuanghuang commented Mar 17, 2022

The new one seems to double it up for me?

Previously:
Screen Shot 2022-03-17 at 2 12 15 pm

Now:
Screen Shot 2022-03-17 at 2 12 32 pm

Edit: Oh I see this is intentional

@cjdunn
Copy link
Owner

cjdunn commented Mar 17, 2022

I honestly don't remember the thought process behind this. At some point I want to make it so you can just type in the context that you want, something like: /$LEFT/$RIGHT/$LEFT HH/$LEFT/$RIGHTHH or /$LEFT/$RIGHT/$LEFT/$RIGHT HH/$RIGHTHH or whatever. But I haven't figured that out yet.

@arrowtype Do you think the mirrored pair should change back to the old way? /$LEFT/$RIGHT/$LEFT Or stay as it is now? /$LEFT/$RIGHT/$LEFT/$RIGHT ? I have no strong feeling either way, I think you were the one that wanted this feature originally

@arrowtype
Copy link
Contributor

arrowtype commented Mar 17, 2022

Wait, huh? No, the doubled mirrored pairs is not intentional, but it does show I didn’t test my solution enough.

Okay, this is a deeper issue that I realized. In my version (#22) I can get at least three states:

  1. One mirrored pair, for pairs containing lowercase or figures, e.g. e a, slash six.hiviz, bracketleft a2, or for pairs not containing punctuation, e.g. A T or R e
  2. Doubled mirrored pairs, for pairs containing punctuation and uppercase, e.g. bracketleft T, or uppercase and figure, e.g. R 6.
  3. A strange case in which slash O.RECT breaks the space center string entirely:

/slash /slash/O.RECT .RECT /backslash /slash /slash/O.RECT .RECT /slash /slash/O.RECT .RECT BID/slash/O.RECT FFER SPREAD

(I would post a screenshot, but GitHub is giving me an error "You can’t comment at this time" when I do. -_-)

I can literally only trigger the last case with slash O.RECT. The font I’m trying this on has other similar suffixes, like R.RECT and Q.RECT, but those don’t cause the issue.

@arrowtype
Copy link
Contributor

Do you think the mirrored pair should change back to the old way? /$LEFT/$RIGHT/$LEFT Or stay as it is now? /$LEFT/$RIGHT/$LEFT/$RIGHT ? I have no strong feeling either way, I think you were the one that wanted this feature originally

I could go either way. It feels a little simpler to understand as just LRL, but I can imagine that it might be helpful to have LRLR, to see not only how R fits between L, but how L sits between R. But then again, maybe that is redundant / unnecessary?

@arrowtype
Copy link
Contributor

arrowtype commented Mar 17, 2022

Probably, the script can be slightly refactored to eliminate a few if/else blocks that might be causing some of the issues.

Thinking about it last night, I realized we could probably do something like:

# set mirrored pair if option is check, else set as empty string
mirroredPair = self.pairMirrored(self.pair) if self.w.mirroredPair.get() else ""

# set open close context if option is check, else set as empty string
openCloseContext =  openCloseContextReturn(self.pair) if self.w.openCloseContext.get() else ""

# form the text, with options in front if they are set
text = f"{mirroredPair} {openCloseContext} {self.getSpacingString(pairstring)}"

I might give this a go, and replace this PR if I can get it working well...

@arrowtype
Copy link
Contributor

Hmm, okay, with Wei’s comment, which shows a comparison of putting the open/close context in front ([T] HHT]HOT]OO) vs inside (HH[T]HO[T]OO), I do still feel like it’s best to have out front, not inside. Yes, it is more compact to have inside, but then is loses the context of the spacing string. When I look at it, I want to check: "is T in the middle of H and ]?"

@arrowtype
Copy link
Contributor

Okay, so actually, to avoid confusion, I will wait until #23 is merged/closed before trying to debug further. I’ll file my above issue into an actual issue, so the conversation is in a more logical place.

@cjdunn
Copy link
Owner

cjdunn commented Mar 17, 2022

@arrowtype thanks for looking into this! I just merged the UI changes, based on #23 but with my own take on it:
#24

If you pull the latest version of next-master into your branch your changes should be able to merge in. I'm going to close this PR now since I think the initial work that @weiweihuanghuang did is now integrated into next-master. Any new additions would be great on a separate issue/PR. Thank you both!

@cjdunn cjdunn closed this Mar 17, 2022
@arrowtype
Copy link
Contributor

Awesome, thanks CJ!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants