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

Reimplement adaptive encoding #326

Merged
merged 2 commits into from
Feb 17, 2019
Merged

Reimplement adaptive encoding #326

merged 2 commits into from
Feb 17, 2019

Conversation

dennwc
Copy link

@dennwc dennwc commented Jan 19, 2019

Reimplement adaptive encoding removed in previous commits related to encoding.

This function still uses an old approach - the whole alphabet should be given upfront to create an encoding. The next change will introduce an incremental encoder.

@codecov
Copy link

codecov bot commented Jan 19, 2019

Codecov Report

Merging #326 into v3 will increase coverage by 5.54%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##               v3     #326      +/-   ##
==========================================
+ Coverage   52.61%   58.16%   +5.54%     
==========================================
  Files         145      145              
  Lines       25236    25275      +39     
==========================================
+ Hits        13278    14701    +1423     
- Misses      10170    10209      +39     
+ Partials     1788      365    -1423
Impacted Files Coverage Δ
pdf/model/font.go 61.57% <0%> (+2.44%) ⬆️
pdf/creator/division.go 89.13% <0%> (+1.08%) ⬆️
pdf/model/outline.go 86.74% <0%> (+1.2%) ⬆️
pdf/creator/invoice.go 90.07% <0%> (+1.21%) ⬆️
pdf/model/annotations.go 22.56% <0%> (+1.48%) ⬆️
pdf/creator/toc_line.go 75.39% <0%> (+1.58%) ⬆️
pdf/internal/transform/matrix.go 67.27% <0%> (+1.81%) ⬆️
pdf/internal/textencoding/truetype.go 16% <0%> (+2%) ⬆️
pdf/creator/chapters.go 64.94% <0%> (+2.06%) ⬆️
pdf/model/fonts/std.go 90% <0%> (+2.5%) ⬆️
... and 88 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23743a3...ff0e5fc. Read the comment docs.

@gunnsth
Copy link
Contributor

gunnsth commented Jan 20, 2019

@dennwc Would it be worth making a test case for NewStandard14FontWithEncoding ?
Are you planning to keep it when the adaptive/incremental encoding is introduced?

@dennwc
Copy link
Author

dennwc commented Jan 20, 2019

@gunnsth Incremental encoding will make this function useless, I guess. I consider this a "backup plan" for incremental encoding. If a good implementation of incremental encoding will take too much time or will require significant changes, this PR will at least cover the feature until it's done.

Copy link

@gunnsth-review gunnsth-review left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @dennwc)


pdf/model/font.go, line 132 at r1 (raw file):

// a TextEncoder that encodes all the runes in `alphabet`, or an error if this is not possible.
// An error can occur if `basefont` is not one the standard 14 font names.
func NewStandard14FontWithEncoding(basefont fonts.StdFontName, alphabet map[rune]int) (*PdfFont,

Is there any way to test this? E.g. given the inputalphabet, check that the encoder behaves correctly?


pdf/model/font.go, line 214 at r1 (raw file):

// GetAlphabet returns a map of the runes in `text` and their frequencies.
func GetAlphabet(text string) map[rune]int {

Add a couple of basic tests, check map output of a few input strings with 0, 1, multiple instances of runes.

@gunnsth gunnsth added this to the v3.0.0-alpha.3 milestone Feb 17, 2019
@gunnsth gunnsth merged commit 2070f8c into unidoc:v3 Feb 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants