Skip to content

Conversation

@diegomura
Copy link
Collaborator

No description provided.

@diegomura diegomura self-assigned this Mar 25, 2018
@diegomura diegomura requested a review from devongovett March 25, 2018 23:11
@diegomura diegomura changed the title Fix insert glyph Fix insert glyph (WIP) Mar 25, 2018
@diegomura
Copy link
Collaborator Author

diegomura commented Mar 26, 2018

Hi @devongovett !
As you can see I've been working with this a little bit 😄

To make the stringIndices and glyphIndices work, I had to change how some models work. I'm still trying to cover all cases, which wasn't easy, specially considering that these concepts are new to me.

While doing it, I had this doubt that I would really appreciate if you can address it 😄:

  • GlyphRun start and end fields are related to the string indices or glyph indices?. Based on the glyph generator I would say that its related to glyph indices, but some other things in the code suggested the opposite. I think I made the mistake to make my tests considering these attributes as string indices, and changing the code accordingly in this PR, but I would like to double check with you this point, and also ask you what do you think it's best in this case.

Thanks in advance!

@diegomura
Copy link
Collaborator Author

@devongovett Also, when we do GlyphString.slice(start, end), start and end refers to string indices or glyph indices?
I'm interested to know how you thought it from the beginning, so I have to change the less as possible. I think I miss-understood some concepts related to this that made me waste some time 😄

@devongovett
Copy link
Member

@diegomura yeah, GlyphRun start and end are related to glyph indices. stringIndices is an array of string indices for each glyph.

slicing also refers to glyph indices.

@devongovett
Copy link
Member

I'd definitely recommend leaving it as glyph indices by default. It is much more likely that the rest of the code will deal with glyphs rather than characters, so it should be faster and easier to map back to string indices only when needed rather than the other way around.

@diegomura
Copy link
Collaborator Author

Thanks @devongovett ! Yes, I thought so, and makes sense. I just pushed some fixes and now the layouting is working fine, using stringIndices and glyphIndices when needed:

screen shot 2018-03-27 at 3 45 07 am

I still got that red overline (do you know what it is for? seems like grammatical warning, but does not makes much sense), and I observed that if I copy the glyphs in the document, they are pasted as 􀀅. I would guess this is a renderer or pdfkit issue. But it's not related with this.

@diegomura
Copy link
Collaborator Author

I would say that GlyphRun and GlyphString now have a solid base of tests, which are two of the most complex models we handle, so I'm going to merge this. I'll keep working on this these next days, probably fixing some things I had to comment (such as Tabs), adding more tests and stuff.

I'll create issues for all the things I want to tackle, and I might ask for your feedback/help in some of them. I know you have lots of things going on right now (great work with Parsel btw!), so I don't want to bother you, but you clearly have more experience than me on these topics, so it might be very helpful 😄

@devongovett
Copy link
Member

@diegomura Awesome 🎉! Glad you got it working.

The red wavy underline is just an attribute in the example: https://github.com/devongovett/textkit/blob/master/temp.js#L37-L38 haha no idea why I implemented that 😉

Copying the glyphs as text from the document doesn't work yet because we don't pass the original string values for each glyph: https://github.com/devongovett/textkit/blob/master/src/renderers/TextRenderer.js#L104. PDFKit will need to be updated to support that.

@diegomura diegomura changed the title Fix insert glyph (WIP) Fix text layout and breaking Mar 27, 2018
@diegomura diegomura merged commit 96149d9 into master Mar 27, 2018
@devongovett
Copy link
Member

Totally, feel free to send questions whenever and I'll do my best to answer them quickly. Thanks for working on this! 😄

@diegomura diegomura deleted the fix-insert-glyph branch March 27, 2018 06:55
@diegomura
Copy link
Collaborator Author

@devongovett thanks!!
Haha didn't noticed that attribute. Glad that's something easy to get rid of.
I'll need to tackle the copy paste issue eventually, and for that I might also need your help and support on the pdfkit side. But not a problem for today 😄. Will create a ticket for that, and resume the discussion there eventually.

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