font-patcher: Copy selection instead of continuously regenerating #736
Conversation
[why] This is a TODO item. Well, two in fact. The symbolFont's selection is used for two things - the main loop to iterate over all glyphs to insert - to select the one glyph that is actually copied over Because the main loop uses iterators on the selection. The iterator is not 'stable' but invalidates if the selection is changed. The current code therefor restores the old selection before the loop jumps to the head again. This design is not very robust. [how] We need the selection to copy the symbol glyph. But we can rewrite the loop that it does not need the selection at every iteration, but that the selection is copied into a list, and we loop over that list - which is independent on a later selection state. Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why] The selection is never used. Later in the code we use a sourceFont selection to paste the glyphs into it, but that is selected just there, before, and locally not totally. Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why] In the patch loop we use two methods to do something with the glyphs - the glyph object directly (i.e. font[codepoint]) - a selection of glyphs in the font Using the selection is a bit anonymous and depends on lines far away (where the selection is set); direct glyph access seems to be easier understood and is used almost everywhere. Direct glyph access can not be used for copy() and paste(), but for everything else. [how] The direct glyph addressing is already in use almost for everything, but not for the transform() calls. This is changed. Now we transform a specific glyph and not 'all selected glyphs' (with selection = that specific glyph). Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
|
Add two unrelated code simplifications. Cherry pick whatever you like ;-) These are just items I needed to overcome in my mind when started working on |
|
Another oddity... We check if the hex-string in
Both will never happen, because we fill it with a generated string Edit: Well, added 'not' to first bullet point |
[why] The code around `currentSourceFontGlyph` and `copiedToSlot` is needlessly complex and checks for conditions that are impossible to occur (possibly because the algorithm was different in the past). It becomes rather hard to follow which variable holds what kind of value and when. [how] Drop all the string-that-contain-hex-numbers stuff and use a regular integer list/variable. Format the string when it is output. Drop obsolete checks. Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
| update_progress(round(float(index + 1) / glyphSetLength, 2)) | ||
| else: | ||
| progressText = "\nUpdating glyph: " + str(sym_glyph) + " " + str(sym_glyph.glyphname) + " putting at: " + copiedToSlot | ||
| progressText = "\nUpdating glyph: {} {} putting at: {:X}".format(sym_glyph, sym_glyph.glyphname, currentSourceFontGlyph) |
There was a problem hiding this comment.
I believe we should drop sym_glyph.glyphname from the output, as that is already in the to-string-cast sym_glyph.
But well, I did not want to change the outside-visible behavior.
|
Maybe the code could also benefit from a style unification. CamelCase versus snail_case? "string" versus 'string'? print() versus sys.write()? etc. |
ryanoasis
left a comment
There was a problem hiding this comment.
Great, thank you!
Actually all of the changes look good to me. really cleans/simplifies things too.
| if int(copiedToSlot, 16) < 0: | ||
| print("Found invalid glyph slot number. Skipping.") | ||
| continue | ||
|
|
There was a problem hiding this comment.
custom still works which is when this was added. probably safe and unneeded so this seems okay to remove 👍🏻
…ion-todo font-patcher: Copy selection instead of continuously regenerating
[why]
This is a TODO item. Well, two in fact.
The
symbolFont's selection is used for two thingsBecause the main loop uses iterators on the selection.
The iterator is not 'stable' but invalidates if the selection
is changed.
The current code therefor restores the old selection before the loop
jumps to the head again.
This design is not very robust.
[how]
We need the selection to copy the symbol glyph.
But we can rewrite the loop that it does not need the selection at every
iteration, but that the selection is copied into a list, and we loop
over that list - which is independent on a later selection state.
Requirements / Checklist
What does this Pull Request (PR) do?
Remove two
TODOitems.How should this be manually tested?
Patch one font with and one without this PR. Compare if symbols that we patch in are there and have same size as before.
Any background context you can provide?
What are the relevant tickets (if any)?
Screenshots (if appropriate or helpful)
Edit: Fill how-to-test paragraph