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

Various picks from crengine-ng #504

Merged
merged 10 commits into from Jan 21, 2023
Merged

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Jan 5, 2023

Various picks from @virxkane 's https://gitlab.com/coolreader-ng/crengine-ng .

(crengine-ng) Various small picks: unused variables, cast & types

Remove unused variables, fix types, more proper casts.

From crengine-ng commits:
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/259e8e7a9951d74fa81b268c7117c24070d8b7cb
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/a90ac8cc26fd1aefa9a33d11edecd6696578cbd5
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/3ed32dd05d1d26c79ec3cc725e8a04f28e7ad984
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/f9095b8aa773c1bcafa5ec0fb3b0cd4587feadbe
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/64ce6c2a8135841374f0f9338c29b753657a25c6
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/146fdd59a46958cda6a1cbce5e14c8f0a1bee845
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/2791906f7b59b9eb897f40b46c0053064f7ec9e1
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/116e5cf5d8e6fcf7233d939c501d009719f630af

(crengine-ng) LVStream: minor fixes

  • ZIP64 minor fixes: when finding 'End of central directory record' don't try to lseek outside of file size.
  • LVPumpStream() function improved: added check result of write operation.
  • LVDeleteDirectory(): use rmdir instead of unlink.
  • Give a name to typedef struct ZipLocalFileHdr to avoir clang warning

From crengine-ng commits:
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/6e6655432e510f1ef7973e4923de974d7916e3d5
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/8e024f68b5ff6a17c4717539b0b7b1d63b831830
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/750751bbe67dc9d61c6d245c4b2fbe50eb92eddc
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/5c7db1c550ffcea9f8f296bac8f678d513202c88

(crengine-ng) LVStream: cleanup zip processing

ZIP processing: replaced hardcoded maximum entry name length and maximum extra entry data length with a new defines.
It is allowed to skip extra entry data if it is too long without stopping the processing of the zip archive.

From crengine-ng commit:
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/5473ebed8ae8b300ca5f1138a5d6ad41b8155723

(crengine-ng) ldomDataStorageManager: fix cast & types

From crengine-ng commit:
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/3ed32dd05d1d26c79ec3cc725e8a04f28e7ad984

(crengine-ng) ldomDataStorageManager: fix possible segfault

  • SIGSEGV fixed in ldomDataStorageManager class, AddressSanitizer: heap-use-after-free.
  • Fix cache emptying function.

From crengine-ng commits:
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/eaee261107221c46b759a68b527515c39b986c0f
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/1f3dc6b5395b29cc63297a6b5a6191ad0f1102fd

(crengine-ng) DOM/MathML: more currNode/parentNode sanity checks

From crengine-ng commit:
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/9f1032becb4f6ae765e30c356525227e24fe61a8

(crengine-ng) Default style: use cr3.ini fg & bg colors

ldomDocument default style: use text color and background color from document properties instead of black and white.
This fixes incorrect colors for mathml elements (fraction bar, sqrt symbol...) with non-standard color settings.

From crengine-ng commit:
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/aed17364d766a92b619e92d9a9da9b84cff2fd29

(crengine-ng) Support regular documents inside zip

LVDocView::loadDocumentInt(): allow to open '.fb3', '.epub', '.doc', '.docx', '*.odt' inside zip-archives.

From crengine-ng commit:
https://gitlab.com/coolreader-ng/crengine-ng/-/commit/d0be59c7a24063c6037e8a3bd25fe9f099553a9b

Silence some clang warnings

New warnings on old code that got to be checked by clang-tidy on this PR.

Stylesheets: support percent-encoded stylesheet links

Should allow closing koreader/koreader#10021


This change is Reviewable

@poire-z
Copy link
Contributor Author

poire-z commented Jan 5, 2023

I'll merge and bump this later, after KOReader v2023.01 is released (this weekend, right? :), and after a bump of crengine with @benoit-pierre 's recent commits, so we get 2 nightlies to help discriminate issues in case of any.

@Frenzie
Copy link
Member

Frenzie commented Jan 5, 2023

This weekend is the current plan. :-)

@virxkane
Copy link
Contributor

virxkane commented Jan 5, 2023

@poire-z
Why didn't you use cherry-pick? So the authorship of commits is lost.
I've always moved your commits carefully, while saving your authorship. I hope you fix this.
Thank you.

@Frenzie
I hope I don't have to remind you as it was before?
buggins/coolreader#274 (comment)
But if @poire-z lost the authorship of the commits, will you remember who the real author is?
Thank you.

The copyright is copyright. And it must be respected.

I can't help but add, you ignored the license issue buggins/coolreader#338
I do not understand this buggins/coolreader#338 (comment)

If you develop software under a certain license, you need to know it, you need to comply with the requirements that it imposes and preferably recommendations.
All you need to do is change the wording of the license to GPLv2+ and make a complete list of authors. If there is a desire for each source file, but this is not necessary.
Thank you.

Happy New Year!

@poire-z
Copy link
Contributor Author

poire-z commented Jan 5, 2023

Why didn't you use cherry-pick? So the authorship of commits is lost.

Because for most of the commits, I couldn't cherry pick, because our sources diverged and because of your ongoing split towards "one-file-per-class" - so I indeed did not git cherry-pick at all, and reported all your changes manually (and sometimes not all the changes contained in a single commit, just the ones that were interesting and were easy to apply).

Is it ok if I just change the owner of all these commits here to be you (even if it's not all your original commits and code, and possibly some adjustments by me - and that I merged some small ones?)

If you develop software under a certain license, you need to know it, you need to comply with the requirements that it imposes and preferably recommendations.

Well, we worked together for years without bothering a single time about that. The fact that you now bother is good for you, but let me code for fun in good spirit - and let my fellow @Frenzie and @NiLuJe guide me and tell me what to do about the legal stuff.

@virxkane
Copy link
Contributor

virxkane commented Jan 5, 2023

@poire-z

Because for most of the commits, I couldn't cherry pick, because our sources diverged

It's not a problem. You do a cherry-pick, it ends in a conflict, you manually make changes just like you did here, then you do git cherry-pick --continue.

Is it ok if I just change the owner of all these commits here to be you

Or so, I don't mind.

Well, we worked together for years without bothering a single time about that.

Yes, sometimes we make mistakes, but it's still good to fix them.

@NiLuJe
Copy link
Member

NiLuJe commented Jan 5, 2023

It's not a problem. You do a cherry-pick, it ends in a conflict, you manually make changes just like you did here, then you do git cherry-pick --continue.

Depending on the extent of the divergences, that can be considerably more annoying and error-prone work than just starting from scratch manually ;).

(Even with a good 3-way merge diff tool (i.e., Perforce's, as pretty much everything else pales in comparison (pun intended ;p)).

Comment on lines 1342 to 1347
if (words_in_file == 0) {
free(buf);
return USER_HYPH_DICT_MALFORMED;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This will only happen, if the user dict is empty (could happen, if a user deletes all entries) or only has one line without a trailing NL (which is not allowed by design and will also be considered as empty).

In both cases the frontend would try to load a user-dictionary (which is considered empty) and crengine should honor that attempt. That means release a previous loaded user-dict, free(buf) and report that the user-dict has been reloaded (even this means with an empty dictionary).

Therefore I would use:

    if (words_in_file == 0) {
        free(buf);
        release();
        return USER_HYPH_DICT_RELOAD;
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Done.

virxkane and others added 10 commits January 21, 2023 18:14
Remove unused variables, fix types, more proper casts.

From crengine-ng commits:
259e8e7a9951d74fa81b268c7117c24070d8b7cb
a90ac8cc26fd1aefa9a33d11edecd6696578cbd5
3ed32dd05d1d26c79ec3cc725e8a04f28e7ad984
f9095b8aa773c1bcafa5ec0fb3b0cd4587feadbe
64ce6c2a8135841374f0f9338c29b753657a25c6
146fdd59a46958cda6a1cbce5e14c8f0a1bee845
2791906f7b59b9eb897f40b46c0053064f7ec9e1
116e5cf5d8e6fcf7233d939c501d009719f630af
at https://gitlab.com/coolreader-ng/crengine-ng
- ZIP64 minor fixes: when finding 'End of central directory
  record' don't try to lseek outside of file size.
- LVPumpStream() function improved: added check result of
  write operation.
- LVDeleteDirectory(): use rmdir instead of unlink.
- Give a name to typedef struct ZipLocalFileHdr to avoir
  clang warning

From crengine-ng commits:
6e6655432e510f1ef7973e4923de974d7916e3d5
8e024f68b5ff6a17c4717539b0b7b1d63b831830
750751bbe67dc9d61c6d245c4b2fbe50eb92eddc
5c7db1c550ffcea9f8f296bac8f678d513202c88
ZIP processing: replaced hardcoded maximum entry name length and
maximum extra entry data length with a new defines.
It is allowed to skip extra entry data if it is too long without
stopping the processing of the zip archive.

From crengine-ng commit:
5473ebed8ae8b300ca5f1138a5d6ad41b8155723
From crengine-ng commit:
3ed32dd05d1d26c79ec3cc725e8a04f28e7ad984
- SIGSEGV fixed in ldomDataStorageManager class,
  AddressSanitizer: heap-use-after-free.
- Fix cache emptying function.

From crengine-ng commits:
eaee261107221c46b759a68b527515c39b986c0f
1f3dc6b5395b29cc63297a6b5a6191ad0f1102fd
From crengine-ng commit:
9f1032becb4f6ae765e30c356525227e24fe61a8
ldomDocument default style: use text color and background color from
document properties instead of black and white.
This fixes incorrect colors for mathml elements (fraction bar, sqrt
symbol...) with non-standard color settings.

From crengine-ng commit:
aed17364d766a92b619e92d9a9da9b84cff2fd29
LVDocView::loadDocumentInt(): allow to open '*.fb3', '*.epub',
'*.doc', '*.docx', '*.odt' inside zip-archives.

From crengine-ng commit:
d0be59c7a24063c6037e8a3bd25fe9f099553a9b
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.

None yet

5 participants