Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

Fix missing thumbnail on MacOS #20

Closed
wants to merge 1 commit into from
Closed

Fix missing thumbnail on MacOS #20

wants to merge 1 commit into from

Conversation

gonejack
Copy link
Contributor

Hi, author.

This PR addresses both #19 and #18

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 81.926% when pulling 29bfce9 on gonejack:fix/thumbnail into 2a8f9fd on bmaupin:master.

@bmaupin
Copy link
Owner

bmaupin commented Apr 14, 2021

@gonejack Thanks for the PR!

Unfortunately it seems to be failing the EPUBCheck test (I'm using v4.2.2).

To see the results for yourself, you can either:

I'll try to document this a bit better for contributors to make it easier to submit PRs.

Here is the output I'm getting:

$ go test
Validating using EPUB version 3.2 rules.
ERROR(RSC-005): My EPUB.epub/EPUB/package.opf(10,63): Error while parsing file: value of attribute "property" is invalid; must be an XML NMTOKEN
ERROR(RSC-005): My EPUB.epub/EPUB/package.opf(10,63): Error while parsing file: attribute "name" not allowed here; expected attribute "dir", "id", "refines", "scheme" or "xml:lang"
ERROR(RSC-005): My EPUB.epub/EPUB/package.opf(10,63): Error while parsing file: attribute "content" not allowed here; expected attribute "dir", "id", "refines", "scheme" or "xml:lang"
ERROR(RSC-005): My EPUB.epub/EPUB/package.opf(10,70): Error while parsing file: character content of element "meta" invalid; must be a string with length at least 1 (actual length was 0)

Check finished with errors
Messages: 0 fatals / 4 errors / 0 warnings / 0 infos

EPUBCheck completed

--- FAIL: TestEpubValidity (3.62s)
    epub_test.go:652: EPUB validation failed
FAIL
exit status 1
FAIL	github.com/bmaupin/go-epub	4.153s

If you want to send a separate PR just for #19 I can merge that now, or if you'd like you can just fix this one.

Thanks!

@bmaupin
Copy link
Owner

bmaupin commented Apr 14, 2021

@gonejack I just added more information on EPUBCheck to the readme. Hope that helps!

@bmaupin
Copy link
Owner

bmaupin commented Apr 14, 2021

@gonejack I just updated the CI/CD tests in Travis to run EPUBCheck so it should help catch this sooner.

@gonejack
Copy link
Contributor Author

I notice that sigil sets cover meta the same way and O'reilly books contain this meta as well.

It's interesting why sigil and macOS support something not following the specification.

@bmaupin bmaupin mentioned this pull request Jun 8, 2021
@bmaupin
Copy link
Owner

bmaupin commented Jun 13, 2021

@gonejack I'll try to take a closer look at this PR this coming week. If you're able to split the fixes for #18 and #19 into separate PRs it might be a little bit easier. But if not I'll see what I can do. Thanks!

@gonejack
Copy link
Contributor Author

@coveralls it's my fault that mixed them, fix for #19 is too short for a PR, could you include it in your later commits?

@bmaupin bmaupin self-assigned this Jun 15, 2021
@bmaupin bmaupin closed this in #38 Jun 15, 2021
bmaupin added a commit that referenced this pull request Jun 15, 2021
Fix missing thumbnail on MacOS

Closes #18, closes #20
@bmaupin
Copy link
Owner

bmaupin commented Jun 15, 2021

@gonejack

I did some research regarding the thumbnail issue and the new element you added, and it seems that this is an old EPUB 2 element that a lot of companies still use. This page in particular was helpful: http://idpf.org/forum/topic-715

This was the example given on that page:

<meta name="cover" content="My_Cover_ID" />

According to that page, this should still be valid even in EPUB 3 (which this library generates), so I compared it to the meta element from the change you made:

<meta property="" name="cover" content="testfromfile.png"></meta>

The biggest difference seems to be that they left out the property attribute.

So I made one small tweak in pkg.go, and it passes validation now!

	Property string `xml:"property,attr,omitempty"`

Since I couldn't modify this PR, I created a new one (#38) and rebased it on your commit so you'll be listed as a contributor.

Thanks for your help and I'm sorry it took me so long to look into this. I don't have a Mac that's easily accessible now, but hopefully this fixes #18. Feel free to reopen it if it doesn't.

@gonejack
Copy link
Contributor Author

Good done, I will try it later.

bmaupin added a commit to bmaupin/epub-samples that referenced this pull request Jun 16, 2021
@gonejack gonejack deleted the fix/thumbnail branch August 14, 2021 05:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants