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

(Upstream) Various minor picks to keep some bits in sync #450

Merged
merged 10 commits into from
Aug 31, 2021

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Aug 26, 2021

Picking what we can easily from buggins/coolreader#304 and buggins/coolreader#307:

  • methods added to base objects, that can be helpful with future devs
  • stuff that touches things we don't use (LVDrawBookCover, sentence selection fixes) and that git applied without much complain
  • some easy cleanup.

Many other things not picked as they touch mostly the UI<->Core interface and we have already diverged too much, so even if I would have liked picking buggins/coolreader@2d693aedd44c93 to keep much of that in sync, I didn't as we have diverged because of previous added things that we didn't need and I didn't picked at the time :/
Also, some top status bar refactoring that couldn't be integrated because we did some changes on our side, and upstream did too (ability to put this status bar at the bottom that we obviously didn't need, etc...)
So, well, that's how it is - with lvdocview.cpp having to be a bit of their version of our cre.cpp.

The LVDrawBookCover will need some fixes to cre.cpp, because we have its call in a method we don't use - so I'll comment out our drawCoverPage() and cursorRight().


This change is Reviewable

@poire-z
Copy link
Contributor Author

poire-z commented Aug 26, 2021

Gently reminding @Frenzie about #443 (comment) :)

@Frenzie
Copy link
Member

Frenzie commented Aug 26, 2021

@poire-z If you rebase and force push I figure it should run.

@poire-z
Copy link
Contributor Author

poire-z commented Aug 26, 2021

Nope, same issue as #451 (comment)

@poire-z poire-z force-pushed the upstream_picks branch 2 times, most recently from c2d6ce6 to c38f97c Compare August 26, 2021 19:28
@poire-z
Copy link
Contributor Author

poire-z commented Aug 26, 2021

It ran !
(Been seeing no yellow dot "in progress", but saw the green checkmark after some time...)

@poire-z poire-z force-pushed the upstream_picks branch 2 times, most recently from d46c5bd to d153b65 Compare August 27, 2021 08:49
@poire-z
Copy link
Contributor Author

poire-z commented Aug 27, 2021

Thanks for #452 (comment)

I still don't get why I dont see the running/results icons, or after a delay...

Anyway, clang-tidy complains with this that I hadn't seen before:

Error while processing /home/runner/work/crengine/crengine/crengine/src/lvtinydom.cpp.
/home/runner/work/crengine/crengine/crengine/src/../include/lvfntman.h:148:25: error: 'LVFontGlyphCacheItem' does not refer to a value [clang-diagnostic-error]
        return offsetof(LVFontGlyphCacheItem, bmp) + ((bmp_width * bmp_height) * sizeof(*bmp));
                        ^
/home/runner/work/crengine/crengine/crengine/src/../include/lvfntman.h:124:8: note: declared here
Found compiler error(s).
struct LVFontGlyphCacheItem
       ^
/home/runner/work/crengine/crengine/crengine/src/../include/lvfntman.h:152:80: error: 'LVFontGlyphCacheItem' does not refer to a value [clang-diagnostic-error]
        LVFontGlyphCacheItem * item = (LVFontGlyphCacheItem *)malloc( offsetof(LVFontGlyphCacheItem, bmp)
                                                                               ^
/home/runner/work/crengine/crengine/crengine/src/../include/lvfntman.h:124:8: note: declared here
struct LVFontGlyphCacheItem
       ^
/home/runner/work/crengine/crengine/crengine/src/../include/lvfntman.h:152:102: error: invalid use of member 'bmp' in static member function [clang-diagnostic-error]
        LVFontGlyphCacheItem * item = (LVFontGlyphCacheItem *)malloc( offsetof(LVFontGlyphCacheItem, bmp)
                                                                                                     ^

Pinging @NiLuJe if that talks to you, if it's related to the recent introduction of offsetof() in 604132a - if it's something to be solved or if we can silent these via some pragma.

@Frenzie
Copy link
Member

Frenzie commented Aug 27, 2021

I see the problem; I made the same mistake a year and a half ago. :-D

koreader/koreader-base#1058

@Frenzie
Copy link
Member

Frenzie commented Aug 27, 2021

@poire-z Will you attach the change as a separate commit in this PR (since we're rebasing it anyway), or would you prefer I stick it in a separate PR?

poire-z and others added 8 commits August 27, 2021 12:49
@poire-z
Copy link
Contributor Author

poire-z commented Aug 27, 2021

Done, and seeing that mellow yellow dot and seeing it become green, thanks !
It's quite faster that Travis - hope we can get that on frontend/base and we can get them take less time, without concurrency lilmits :)

@poire-z
Copy link
Contributor Author

poire-z commented Aug 27, 2021

So, when I fork a project that uses GH Actions, these actions will run on my fork too. When I push a PR, the action runs both on my repo (47s the last one) and on koreader's repo (40s the last one, so not the same).
Feels not cheap, should cost money to Github/Microsoft and kill some trees. Any way to prevent that ? (may be I can disable actions on my repo, but should everybody have to do that ?)

@Frenzie
Copy link
Member

Frenzie commented Aug 27, 2021

It doesn't simply regard them as the same run like CircleCI does?

@poire-z
Copy link
Contributor Author

poire-z commented Aug 27, 2021

I don't think so, it doesn't look like these 2 runs are the sames.

@Frenzie
Copy link
Member

Frenzie commented Aug 27, 2021

Perhaps https://github.com/marketplace/actions/skip-duplicate-actions can help. But at the very least that's not for this PR. ^_^

@poire-z
Copy link
Contributor Author

poire-z commented Aug 30, 2021

@NiLuJe : any thoughts about #450 (comment) ?

@NiLuJe
Copy link
Member

NiLuJe commented Aug 30, 2021

Haven't forgotten about it, but haven't had time to actually look into it yet ;p.

- Sanitizer error fixed in chmlib: chmlib/src/lzx.c:569:25:
runtime error: left shift of 65404 by 16 places cannot be
represented in type 'int'.
- Fixed incorrect parsing of content file in 'chm' files.
Details: tag auto-closing did not work.
- Fixed null pointer dereferencing when parsing 'chm' files.
Fix possible integer overflow when measuring text with
fallback fonts (HarfBuzz render).
@poire-z
Copy link
Contributor Author

poire-z commented Aug 30, 2021

Added 2 commits picked from buggins/coolreader#308.

@poire-z
Copy link
Contributor Author

poire-z commented Aug 31, 2021

Merging, as all is merged upstream.
I'll bump it after my next PR with small fixes.

@poire-z
Copy link
Contributor Author

poire-z commented Nov 27, 2021

Pinging again @NiLuJe about #450 (comment) (still happening , ie in https://github.com/koreader/crengine/runs/4340698048?check_suite_focus=true)

@NiLuJe
Copy link
Member

NiLuJe commented Nov 27, 2021

Last I checked it's because it's a class or something, but we tested the code on its own and it behaves, so, meh :/.

@poire-z
Copy link
Contributor Author

poire-z commented Nov 27, 2021

But it's not a class, it's a classic struct. May be it's because the functions that use it are defined inside the stuct, so before it's fully defined?

so, meh :/.

I know it behave, I'm just trying to (make you) kill introduced false positives so it doesn't bug us in CI output in unrelated PRs.

@poire-z
Copy link
Contributor Author

poire-z commented Nov 27, 2021

May be it's because the functions that use it are defined inside the stuct, so before it's fully defined?

Nope, that's not it.

@poire-z
Copy link
Contributor Author

poire-z commented Nov 27, 2021

I can't even silence it with // NOLINT :/

https://stackoverflow.com/questions/49206519/not-able-to-use-offsetof-with-clang-compiler-using-libc-and-c14
(in case you understand anything to it :)

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.

4 participants