Q about submitting fixes to crengine

[twowheels]

I've fixed a problem with the 12/24 hour clock setting on crengine, which required change to the crengine C++ code. Is my understanding correct that you have to submit the change all of the way back to the upstream on SourceForge?

[Frenzie]

Coolreader has migrated to GitHub (see https://github.com/buggins/coolreader) but you can also open a PR in https://github.com/koreader/crengine

The problem is just that we are or were at the top of the search results which led to some people thinking we were the coolreader.

[twowheels]

Originally Posted by Frenzie

Coolreader has migrated to GitHub (see https://github.com/buggins/coolreader) but you can also open a PR in https://github.com/koreader/crengine

The problem is just that we are or were at the top of the search results which led to some people thinking we were the coolreader.

Ah, thanks! You might want to update the about text on the koreader/crengine repo! That's what led me to SourceForge.

Mirror of the official crengine (Cool Reader Engine) repository at sourceforge.net http://sourceforge.net/projects/crengine/

[twowheels]

I'm a bit confused. The KOReader log says that it bumped the base crengine somewhat recently, but the code in the repo that you pointed me to is VERY different, and it seems that the 12hour clock option was removed. Given that the engine was bumped just a few days ago and the changes I'm seeing are much older than that, I must be looking in the wrong place.

[NiLuJe]

Where *were* you looking, in both cases?

I mean, the clock formatting option for *CRe* was only added two weeks ago: https://github.com/koreader/crengine...11c151db03d95b, and I'm not sure it's even landed upstream yet, so it's currently only available in KOReader nightlies.

[twowheels]

Originally Posted by NiLuJe

Where *were* you looking, in both cases?

I mean, the clock formatting option for *CRe* was only added two weeks ago: https://github.com/koreader/crengine...11c151db03d95b, and I'm not sure it's even landed upstream yet, so it's currently only available in KOReader nightlies.

Oh, that makes more sense. I assumed that the flow was:

[change]->[crengine]->bump->[koreader]

Are you saying that there's cross pollination in both directions?

That's the exact block of code that I was trying to change, but to make it more consistent w/ the formatting in KOReader:

               int const raw_hour = bt->tm_hour > 12 ? bt->tm_hour % 12 : bt->tm_hour;
               int const hour = raw_hour?raw_hour:12;
               sprintf(str, "%d:%02d %s", hour, bt->tm_min, raw_hour<12?"AM":"PM");

The check-in comment was going to be:

    Improved 12 hour clock display
    
            The primary motivation of this improvement was to make the
            format more consistent with how 12 hour clocks are generally
            displayed, and consistent with how the other clocks are
            displayed when used within KOReader.
    
               * Made the midnight to 01:00 hour display 12 instead of 0
               * Added an AM/PM indicator

[NiLuJe]

We don't control CRe's upstream, we control *KOReader's* fork of CRe (what I linked to).

@poire-z often pulls upstream's CRe changes into our fork, and @virxkane often pulls our fork's changes into CRe's upstream .

So, when I say CRe, I usually mean our fork.
In which case, it needs to be bumped in koreader-base, and base needs to be bumped in koreader for it to show up in nigthlies.

[twowheels]

Originally Posted by NiLuJe

We don't control CRe, we control *KOReader's* fork of CRe (what I linked to).

@poire-z often pulls upstream's CRe changes into our fork, and @virxkane often pulls our fork's changes into CRe's upstream .

So, when I say crengine, I mean our repo.

In which case, it needs to be bumped in koreader-base, and base needs to be bumped in koreader for it to show up in nigthlies.

So, should I submit my MR to the KOReader base repo then rather than the upstream repo?

[NiLuJe]

As far as your exact change, I can pretty confidently say that it'd be rejected, because it's not translatable.

And we can't really switch to strftime, because Windows is terrible (c.f., #410).

(But, yes, our fork is where this should go to, especially since this probably hasn't landed upstream at all yet).

[twowheels]

Good point on the l10n. I'll look into how that's handled, and if it's too much of a mess I'll just submit the 0 --> 12 part of the change as a smaller PR.

[NiLuJe]

That said, you've got a couple of things going for you:

* We don't give much of a damn about 12 hour clocks
* We give even less of a damn about CRe's header
* The sane (e.g., strftime) approach requires compiled locales for the target language, and that's a non-starter on Kobo, because they don't compile *any* locales. So it'd effectively always show AM/PM anyways.
I don't recall how well Kindles handle non-English locales on that front.
* AFAIK, there aren't many languages that actually both prefer 12h clocks *and* have a localized variant, and I'm even less sure if we actually support any of those .