Display date added and modified for audio contributions on sentence page #3117

cblanken · 2024-04-09T22:06:12Z

This PR addresses issue #2275.
To keep it simple, the created date and last modified date are both displayed. However, I think it might be better to only display the modified date if it differs from the creation date. It might also be better to calculate the days/months/years "ago" that the audio recording was created or modified to match the Logs section below it.

Let me know what you think. I'm happy to make any necessary changes.

Audio contributions

Logs section for reference

Yorwba · 2024-04-14T17:37:01Z

Thank you for the PR!

For the dates I would use the ago function from the DateHelper, which is used in the logs section. (getDateLabel is also an option but doesn't show a full sentence if creation and modification dates are the same.)

The CSS needs some protection against overflow if the localized string gets too long:

- Note this also modifies the DateHelper ago function to embed a non-breaking space as Unicode codepoint in the output instead of embedding it as an   HTML entity

cblanken · 2024-04-15T21:12:19Z

I've updated the CSS to handle overflows and setup the date display to use the DateHelper's ago function.

One note though. Since the audio section rendering is handled in Angular. This makes it a bit awkward to use the DateHelper, so I added a couple fields to the audios array before it's encoded into JSON and handed off to the client. However, the existing version of ago() embeds the   HTML entity as part of the output which, as far as I know, can only be used if we adapt the audios section to be rendered server-side with PHP.

Instead, I modified the ago() function to embed the non-breaking spaces as a Unicode codepoint so that it can be encoded into JSON without breaking the rendering client-side.

I don't think that should be a problem for rendering elsewhere, but I just wanted to point it out as it does cause a few tests to fail which seem to expect the  .

Screenshot

jiru · 2024-04-22T14:24:41Z

src/Model/Table/SentencesTable.php

@@ -665,7 +665,7 @@ public function fields($what = [])
    public function contain($what = [])
    {
        $audioContainment = function (Query $q) use ($what) {
-            $q = $q->select(['id', 'external', 'sentence_id']);


I think this should be moved to the if (isset($what['sentenceDetails'])) { block below in a way that adds created and modified fields to the select operation. This way we only retrieve these extra fields on sentence pages (as opposed to retrieving them everywhere a sentence block is displayed).

See commit ba8e93d

jiru · 2024-04-22T14:28:18Z

src/Template/Element/sentences/audio.ctp

+                <div class="since"><?= format(__('{date}'), [ 'date' => '{{ audio.created_ago }}' ]) ?></div>
+            </div>
+            <div class="timestamp">
+                <div><?= format(__('Last modified')) ?></div>
+                <div class="since"><?= format(__('{date}'), [ 'date' => '{{ audio.modified_ago }}' ]) ?></div>


These two calls to format() look unnecessary, how about writing {{audio.created_ago}} directly?

See commit dec4e68

jiru · 2024-04-22T14:35:32Z

src/Template/Element/sentences/audio.ctp

+                <div><?= format(__('Added')) ?></div>
+                <div class="since"><?= format(__('{date}'), [ 'date' => '{{ audio.created_ago }}' ]) ?></div>
+            </div>
+            <div class="timestamp">
+                <div><?= format(__('Last modified')) ?></div>


These two translatable strings "Added" and "Last modified" could be hard for translators to translate because of the lack of context. You can add @translators notes in a code comment on the line above the __() and these will appear on our Transifex, see src/Template/Element/advanced_search_form.ctp for some examples.

See commit dec4e68

jiru · 2024-04-22T14:56:21Z

src/View/Helper/DateHelper.php

-            return format(__n('yesterday', '{n}&nbsp;days ago', $diff->days), array('n' => $diff->days));
+            return format(__n('yesterday', "{n}\u{00A0}days ago", $diff->days), array('n' => $diff->days));
        } elseif ($diff->h > 0) {
-            return format(__n('an hour ago', '{n}&nbsp;hours ago', $diff->h), array('n' => $diff->h));
+            return format(__n('an hour ago', "{n}\u{00A0}hours ago", $diff->h), array('n' => $diff->h));
        } else {
            // we stop at minute accuracy
            $minutes = max($diff->i, 1);
-            return format(__n('a minute ago', '{n}&nbsp;minutes ago', $minutes), array('n' => $minutes));


Could you elaborate why this change is necessary?

In the audio.ctp template, the audio data is encoded into JSON, so it can be passed on to AngularJS for the client-side rendering. If the the DateHelper embeds the HTML entity non-breaking space   then it will be decoded literally and output to the HTML like so:

It's possible there's some way to make AngularJS consider the HTML safe and to interpret the   properly (see https://stackoverflow.com/a/10971756), but I'm not very familiar with it. Using the Unicode non-breaking space seemed like the least intrusive way to get it working.

The browser should interpret them the same way. You can see even when using the Unicode non-breaking space, my browser (Chromium) replaces it with an  .

Thanks, I see! The existing code base mostly assumes translations can contain HTML, as it is sometimes necessary to include formatting tags such as <a>, <em> or <strong>.

So I am concerned that this change would invalidate a lot of translations that won’t be added back before long in many languages since we don’t have a dedicated team of UI translators. For example, let’s say a contributor translated the UI up to 100% in Korean (just an example right) and then does not actively maintain the translation, so every time some new translatable strings are added or replaced, the translation coverage decreases to 99%, 98%, 95% etc. These date strings are quite visible because they are used in a lot of places, so updating them will have a substantial impact on Tatoeba’s outlook in many UI languages for some time.

I am also concerned about the discrepancy it creates. For example some translators may nevertheless add some   in their translation of "{n} days ago" and see that it works it one place, such as the Wall, and conclude it’s fine, whereas it’s not displaying correctly in the audio block.

From a security standpoint it’s not very secure to allow HTML from translations, so what AngularJS is doing by default is actually the right thing to do. But this is not the way we deal with translations for now on Tatoeba, so I suggest using AngularJS’s trustAsHtml() instead to keep things consistent between AngularJS code and PHP code.

Ah, okay. I wasn't aware it would break translations like that. I'm not sure how I feel about blindly trusting the translations with trustAsHtml(). I feel like it's overkill since the only issue is a single html entity. Since this is an isolated incident, what if the   entity is instead replaced with the Unicode NBSP just before Angular renders them. Like this.

audio-details.ctrl.js

function init(audios, audioLicenses) { vm.audios = audios.map((audio) => { if (audio.created_ago) { audio.created_ago = audio.created_ago.replace(" ", "\u00A0") } if (audio.modified_ago) { audio.modified_ago = audio.modified_ago.replace(" ", "\u00A0") } return audio }) vm.audioLicenses = audioLicenses; }

This way we don't have to worry about the security implications of using trustAsHtml() for any translations.

src/Template/Element/sentences/audio.ctp

jiru · 2024-04-22T15:08:53Z

The display of the dates look really nice! I could be made even nicer if you check how it looks when you login as admin. There are some extra information below the dates that could benefit from having the same look (with sections separated by a horizontal bar).

The extra divider helps delineate extra controls displayed for admins

cblanken · 2024-04-24T17:32:35Z

Screenshots as of ba8e93d

Contributor

Admin

src/Model/Table/SentencesTable.php

Display date added and modified for audio contributions on sentence page

a0768ea

Handle overflow and display dates with DateHelper

378b189

- Note this also modifies the DateHelper ago function to embed a non-breaking space as Unicode codepoint in the output instead of embedding it as an   HTML entity

Update DateHelper tests to use the Unicode non-breaking space

64d6057

jiru reviewed Apr 22, 2024

View reviewed changes

src/Template/Element/sentences/audio.ctp Show resolved Hide resolved

cblanken added 3 commits April 24, 2024 10:13

Add timestamp divider to bottom of audio section

bcb74a0

The extra divider helps delineate extra controls displayed for admins

Remove unnecessary format() calls and add translator context

dec4e68

Retrieve created/modified audio fields only on sentence pages

ba8e93d

jiru reviewed Apr 29, 2024

View reviewed changes

src/Model/Table/SentencesTable.php Outdated Show resolved Hide resolved

Remove unnecessary array_merge()

9d0aedd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display date added and modified for audio contributions on sentence page #3117

Display date added and modified for audio contributions on sentence page #3117

cblanken commented Apr 9, 2024

Yorwba commented Apr 14, 2024

cblanken commented Apr 15, 2024 •

edited

jiru Apr 22, 2024

cblanken Apr 24, 2024

jiru Apr 22, 2024 •

edited

cblanken Apr 24, 2024

jiru Apr 22, 2024 •

edited

cblanken Apr 24, 2024

jiru Apr 22, 2024

cblanken Apr 24, 2024 •

edited

jiru Apr 29, 2024

cblanken Apr 29, 2024

jiru commented Apr 22, 2024

cblanken commented Apr 24, 2024

Display date added and modified for audio contributions on sentence page #3117

Are you sure you want to change the base?

Display date added and modified for audio contributions on sentence page #3117

Conversation

cblanken commented Apr 9, 2024

Audio contributions

Logs section for reference

Yorwba commented Apr 14, 2024

cblanken commented Apr 15, 2024 • edited

Screenshot

jiru Apr 22, 2024

Choose a reason for hiding this comment

cblanken Apr 24, 2024

Choose a reason for hiding this comment

jiru Apr 22, 2024 • edited

Choose a reason for hiding this comment

cblanken Apr 24, 2024

Choose a reason for hiding this comment

jiru Apr 22, 2024 • edited

Choose a reason for hiding this comment

cblanken Apr 24, 2024

Choose a reason for hiding this comment

jiru Apr 22, 2024

Choose a reason for hiding this comment

cblanken Apr 24, 2024 • edited

Choose a reason for hiding this comment

jiru Apr 29, 2024

Choose a reason for hiding this comment

cblanken Apr 29, 2024

Choose a reason for hiding this comment

audio-details.ctrl.js

jiru commented Apr 22, 2024

cblanken commented Apr 24, 2024

Contributor

Admin

cblanken commented Apr 15, 2024 •

edited

jiru Apr 22, 2024 •

edited

jiru Apr 22, 2024 •

edited

cblanken Apr 24, 2024 •

edited