Skip to content

Add an option to suppress the FFTM table#4320

Merged
skef merged 2 commits intofontforge:masterfrom
dscorbett:no-FFTM
May 7, 2020
Merged

Add an option to suppress the FFTM table#4320
skef merged 2 commits intofontforge:masterfrom
dscorbett:no-FFTM

Conversation

@dscorbett
Copy link
Copy Markdown
Contributor

@dscorbett dscorbett commented May 4, 2020 •
edited
Loading

This redoes #4309 and closes #2389.

It seems like the only problem with #4309 was in the GUI code, so let’s restore the scripting code and I can figure out the GUI crash later. (I did test the options dialog window before opening the first PR, so it may be a while before I can even reproduce the crash.)

Type of change

  • New feature

@ctrlcctrlv
Copy link
Copy Markdown
Member

ctrlcctrlv commented May 5, 2020

Sorry, I'm not inclined to accept this without you exposing the option in the GUI. The GUI shouldn't be treated as second class

@jtanx
Copy link
Copy Markdown
Contributor

jtanx commented May 5, 2020

The crash was fixed with #4317, it was just the behaviour around it being checked/unchecked not reflecting what the true state was

@skef
Copy link
Copy Markdown
Contributor

skef commented May 5, 2020

@dscorbett If you add the GUI code from the last PR back into his one I'll attempt to get it working. I have medium-good grasp on what the problems were.

@dscorbett dscorbett changed the title Add a scripting option to suppress the FFTM table Add an option to suppress the FFTM table May 5, 2020
@skef
Copy link
Copy Markdown
Contributor

skef commented May 5, 2020

I'll take a look at this this evening and if there are remaining changes to be made I'll try to do so.

@skef
Copy link
Copy Markdown
Contributor

skef commented May 6, 2020

(My inability to distinguish GGadgetSetChecked from GGadgetSetEnabled led to confusion in my comments on the last PR.)

I've both reviewed this code as is and tested it and it seems fine in appearance and behavior now. I think the previous remaining issue was the now-correct flag check. @jtanx can you verify?

@jtanx
Copy link
Copy Markdown
Contributor

jtanx commented May 6, 2020

Yeah seems fine now

skef
skef approved these changes May 6, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@skef skef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@skef skef merged commit 21d929b into fontforge:master May 7, 2020
@dscorbett dscorbett deleted the no-FFTM branch May 7, 2020 11:41
@ctrlcctrlv ctrlcctrlv mentioned this pull request Dec 13, 2020
Open
Omnikron13 pushed a commit to Omnikron13/fontforge that referenced this pull request May 31, 2022
@dscorbett
Add an option to suppress the FFTM table (fontforge#4320)
a611108 
* Add a scripting option to suppress the FFTM table

* Expose a GUI option to suppress the FFTM table
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skef skef skef approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Generate: Don't include FFTM

4 participants

@dscorbett @ctrlcctrlv @jtanx @skef