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

Spin off hms as selectable time format #9924

Merged
merged 2 commits into from
Feb 12, 2023

Conversation

melyux
Copy link
Contributor

@melyux melyux commented Dec 16, 2022

The bare minimum changes to do this as talked about in that other PR. Mostly updating datetime.secondsToClockDuration() usages. Updated tests

The name of the hms option is up for debate. "Literal" is the stand-in


This change is Reviewable

@NiLuJe
Copy link
Member

NiLuJe commented Dec 16, 2022

"Literal" is the stand-in

How about... hear me... "hms"? :D

EDIT: Except l10n is going to piss on my parade on that one, nevermind :/.

@NiLuJe
Copy link
Member

NiLuJe commented Dec 16, 2022

Looks fine at a quick glance (all the changes from modern seem to make sense, AFAICT).

@poire-z
Copy link
Contributor

poire-z commented Dec 17, 2022

Good to go ?

@Frenzie Frenzie added this to the 2022.12 milestone Dec 18, 2022
@poire-z
Copy link
Contributor

poire-z commented Dec 30, 2022

So, all good ? I trust @melyux about the code changes and expected display results - @Frenzie : ok with the translation tweaks ?

@Frenzie
Copy link
Member

Frenzie commented Dec 30, 2022

What was literal again? I recall wanting to think about something better.

@melyux
Copy link
Contributor Author

melyux commented Dec 30, 2022

I think one of us suggested it in the other discussion, which may or may not have been a joke ("littoral" was it?). I like it cuz it's literal without using symbols. Or maybe it was meant to be "letter-al" because it uses letters... that's nice. So maybe we could call it "Letters"

My first thought "hms" is not very translation friendly, even tho seems to be what people call it if they ever call it anything. Doesn't matter that much since we give an example next to it

@poire-z
Copy link
Contributor

poire-z commented Dec 30, 2022

that must have been me, littéral in french translates to literal in english :/
So, that's what I meant, literal, as plain, obvious, reading as written.

@Frenzie
Copy link
Member

Frenzie commented Dec 30, 2022

My first thought "hms" is not very translation friendly, even tho seems to be what people call it if they ever call it anything. Doesn't matter that much since we give an example next to it

Why do you say it's not?

@melyux
Copy link
Contributor Author

melyux commented Dec 30, 2022

I guess it could be. The letters themselves are translatable after all. Do you prefer "Letters" or "HMS"?

@Frenzie
Copy link
Member

Frenzie commented Dec 30, 2022

Is there any particular reason to try to avoid hours minutes seconds?

@melyux
Copy link
Contributor Author

melyux commented Dec 30, 2022

Seems out of the pithy theme of "Classic" and "Modern" :)

@poire-z
Copy link
Contributor

poire-z commented Jan 1, 2023

So, all good (bis) ?

@Frenzie
Copy link
Member

Frenzie commented Jan 1, 2023

Well, there's a lack of screenshots and I don't really want to fetch it, so I have no idea one way or the other.

@melyux
Copy link
Contributor Author

melyux commented Jan 3, 2023

Where did we land on the exact wording instead of "Literal"?

@Frenzie
Copy link
Member

Frenzie commented Jan 3, 2023

Please provide some screenshots to figure that out. ;-)

@poire-z poire-z modified the milestones: 2023.01, 2023.02 Jan 23, 2023
@NiLuJe
Copy link
Member

NiLuJe commented Feb 2, 2023

Gentle ping ;).

@Frenzie
Copy link
Member

Frenzie commented Feb 2, 2023

I guess this would be the relevant screenshot?

As I seem to have said previously, I'd probably just call that HMS with a translator's note (or Letters). But I'd have no idea what literal means if someone said it.

@poire-z
Copy link
Contributor

poire-z commented Feb 2, 2023

But I'd have no idea what literal means if someone said it.

I said it, but I won't stand for it :):

that must have been me, littéral in french translates to literal in english :/
So, that's what I meant, literal, as plain, obvious, reading as written.

@Frenzie
Copy link
Member

Frenzie commented Feb 2, 2023

as written

"Written" might be a decent alternative.

@NiLuJe
Copy link
Member

NiLuJe commented Feb 2, 2023

"Spelled out"?

@Frenzie
Copy link
Member

Frenzie commented Feb 2, 2023

That makes me think it's currently twenty-two forty-seven o'clock.

@NiLuJe
Copy link
Member

NiLuJe commented Feb 2, 2023

True enough ;o).

@melyux
Copy link
Contributor Author

melyux commented Feb 2, 2023

I like "Written" too. Actually wait, I saw the "Letters" suggestion, and I like that even better

@Frenzie
Copy link
Member

Frenzie commented Feb 3, 2023

Letters and Written are both fine by me. Letters then?

@melyux
Copy link
Contributor Author

melyux commented Feb 4, 2023

Pushed

@poire-z poire-z merged commit 8b99c50 into koreader:master Feb 12, 2023
@@ -1872,7 +1872,7 @@ function ReaderStatistics:getDatesFromAll(sdays, ptype, book_mode)
local stop_month = os.time{year=year_end, month=month_end, day=1, hour=0, min=0 }
table.insert(results, {
date_text,
T(N_("%1 (%2 page)", "%1 (%2 pages)", tonumber(result_book[2][i])), datetime.secondsToClockDuration(user_duration_format, tonumber(result_book[3][i]), false, true), tonumber(result_book[2][i])),
T(N_("%1 (%2 page)", "%1 (%2 pages)", tonumber(result_book[2][i])), datetime.secondsToClockDuration(user_duration_format, tonumber(result_book[3][i]), false), tonumber(result_book[2][i])),
Copy link
Member

Choose a reason for hiding this comment

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

What exactly is this saying? Is the first meant to be %1 (1 page)?

Copy link
Contributor Author

@melyux melyux Feb 13, 2023

Choose a reason for hiding this comment

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

I think a mistake from when the workings of gettext were a mystery, a mystery compounded by the order of the variables, left over from the statistics PR. It should say this, I think:

T(N_("%2 (1 page)", "%2 (%1 pages)", tonumber(result_book[2][i])), datetime.secondsToClockDuration(user_duration_format, tonumber(result_book[3][i]), false), tonumber(result_book[2][i])),

Copy link
Member

Choose a reason for hiding this comment

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

Would you mind fixing it in a mini-PR if your mind's still in tune with this code?

Copy link
Contributor Author

@melyux melyux Feb 13, 2023

Choose a reason for hiding this comment

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

No problem, but as a sanity check, is the corrected line I posted actually correct? The gettext part

Copy link
Member

Choose a reason for hiding this comment

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

Not entirely, always write it sensibly, i.e., it should look like "%1 (1 page)", "%1 (%2 pages)".

It's the code that should adapt, not what's given to the translators. ^_^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always always forget that the N_() does nothing except choose one of the strings depending on the number given in the second arg

@poire-z
Copy link
Contributor

poire-z commented Feb 17, 2023

Btw, wouldn't you prefer your letters'ed duration to get a little spacing ?
Tried it, and 1d23h38m may feel a bit crowded.
I just noticed that in some private patched feature of mine, I output it as 1d 23h 38m with regular spaces.
Could look in-between these with thin/hair spaces ? Unless the usage is to see all that stuck ? (And may be it's a french thing to add space everywhere like before a question mark :)

@melyux
Copy link
Contributor Author

melyux commented Feb 17, 2023

@poire-z I made the exact same patch for myself, with the added spaces! But didn't think you guys would like it. I think it looks better with the spaces, no hairline required. I'm down to do a mini PR to add the space if everyone else is fine with it too

@NiLuJe
Copy link
Member

NiLuJe commented Feb 17, 2023

No objection here ;).

Frenzie pushed a commit that referenced this pull request Feb 20, 2023
Fix some of my early blunders in using the `N_()` gettext function. Mini-PR from #9924 (comment) (@Frenzie).

There was also one line for generating this same `%1 (%2 pages)` text that confusingly uses different ordering in the SQL query output; switched the two SELECT arguments around to make it match the other 5 usages. Works the same as before
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

4 participants