Skip to content

[ReadingTime] Add options: Reading speed, Source metrics#300

Merged
Alkarex merged 32 commits intoFreshRSS:masterfrom
hkcomori:reading-time
Apr 5, 2025
Merged

[ReadingTime] Add options: Reading speed, Source metrics#300
Alkarex merged 32 commits intoFreshRSS:masterfrom
hkcomori:reading-time

Conversation

@hkcomori
Copy link
Contributor

fix #292

#292 (comment) suggested a policy of using letters for all languages, but my research shows that reading speed in English is usually measured by word count. So I have provided a choice in the metrics. That is, the original policy of #292.

I'm using the js_vars hook for the first time, is the usage correct?
It works fine now, but the js_vars hook is a OneToOne hook, so it might conflict if we use it in another extension too.

@Frenzie
Copy link
Member

Frenzie commented Mar 18, 2025

my research shows that reading speed in English is usually measured by word count. So I have provided a choice in the metrics.

I don't know if it applies here, but they measure typing speed in words per minute (while we do characters per minute), but a word is defined as five characters, making it a more convoluted, misleading version of the same measurement.

@hkcomori
Copy link
Contributor Author

Not smart, but I ignored the following exception to get through PHPStan.

  • FreshRSS_Context_Exception thrown by FreshRSS_Context::userConf().
  • Minz_ActionException for returning validation errors

The @throws comment (1f9070a) could not handle this, as FreshRSS doesn't expect exceptions to be thrown in Minz_Extension.init() and Minz_Extension.handleConfigureAction().

@hkcomori hkcomori changed the title [ReadingTime] Add options: Reading speed, Source metrics draft: [ReadingTime] Add options: Reading speed, Source metrics Mar 29, 2025
Because add_load_more_listener() does not called
when document.readyState === 'loading' and context was already loaded.
@hkcomori hkcomori changed the title draft: [ReadingTime] Add options: Reading speed, Source metrics [ReadingTime] Add options: Reading speed, Source metrics Mar 31, 2025
@Alkarex
Copy link
Member

Alkarex commented Apr 1, 2025

Ping @lapineige

@hkcomori
Copy link
Contributor Author

hkcomori commented Apr 1, 2025

Can I leave it to you?

@Alkarex
Copy link
Member

Alkarex commented Apr 1, 2025

@hkcomori Tests welcome FreshRSS/FreshRSS#7475

Alkarex added a commit to FreshRSS/FreshRSS that referenced this pull request Apr 1, 2025
* Catch extension exceptions in override
FreshRSS/Extensions#300 (comment)

* Fix error message
@lapineige
Copy link
Contributor

Thanks for the ping.
I'm not available to explore this in detail for a while. But overall it sounds like a massive improvement, so if you think it's good to merge, feel free to do it ;)

@hkcomori : if you like, you may add your name in the "author" field or wherever you like in the code to get credit for the additions. I don't mind how.

hkcomori added 6 commits April 2, 2025 10:11
Because the log page is more consistent in English.
Because the language setting is for correct character counts, It is not needed for word counts which are space-separated counts.
Because other extensions to FreshRSS/Extensions are not listed there, followed suit.
@hkcomori
Copy link
Contributor Author

hkcomori commented Apr 2, 2025

@lapineige Thanks, I added author.

@hkcomori
Copy link
Contributor Author

hkcomori commented Apr 2, 2025

@Alkarex Can you help me update README in French? c74fd39 is English only.

@Alkarex
Copy link
Member

Alkarex commented Apr 2, 2025

@Alkarex Can you help me update README in French? c74fd39 is English only.

I think it is so simple that it is good enough in English. We do not have a translation of everything.

Besides that, is this PR ready from your point of view?

@hkcomori
Copy link
Contributor Author

hkcomori commented Apr 2, 2025

Yes, I think there is nothing else missing.

@lapineige
Copy link
Contributor

lapineige commented Apr 3, 2025

Why removing the translation ? 🤔

I mean sure not everything is translated, and more languages would be good, but it helps accessing the content for French and non English speakers, and the content is already there 🤔

I can provide the French version of your change:

You can set reading speed and source metrics to estimate reading time.

Results in:

Vous pouvez définir la vitesse de lecture et la méthode de mesure du temps de lecture.

@Alkarex
Copy link
Member

Alkarex commented Apr 3, 2025

Why removing the translation ? 🤔

@lapineige My fault, I did not realise there was already a part translated

can provide the French version of your change

Yes please 👍🏻

@hkcomori
Copy link
Contributor Author

hkcomori commented Apr 3, 2025

Thanks for the translation.

@Alkarex Alkarex merged commit 17cfe5b into FreshRSS:master Apr 5, 2025
1 check passed
@hkcomori hkcomori deleted the reading-time branch April 7, 2025 01:50
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.

[xExtension-ReadingTime] Reading time of Japanese articles are too short

4 participants