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

l10n: Align notification messages for server version and apps #35768

Closed
rakekniven opened this issue Dec 14, 2022 · 7 comments · Fixed by #35769
Closed

l10n: Align notification messages for server version and apps #35768

rakekniven opened this issue Dec 14, 2022 · 7 comments · Fixed by #35769
Labels

Comments

@rakekniven
Copy link
Member

rakekniven commented Dec 14, 2022

Actually the wording server and app updates is different.

Target is to unify this two messages.

Notification for server update looks like:

$notification->setParsedSubject($l->t('Update to %1$s is available.', [$parameters['version']]));

Notification for an app update looks like:

->setRichSubject($l->t('Update for {app} to version %s is available.', [$notification->getObjectId()]), [

First improvement for server is a change to:
Update to version %1$s is available.

Second improvement for server is adding product name:
Update for Nextcloud to version %1$s is available.

@rakekniven rakekniven added enhancement 0. Needs triage Pending check for reproducibility or if it fits our roadmap feature: language l10n and translations labels Dec 14, 2022
rakekniven added a commit that referenced this issue Dec 14, 2022
Solution for #35768

Reported at Transifex.

Signed-off-by: rakekniven <2069590+rakekniven@users.noreply.github.com>


Signed-off-by: rakekniven <2069590+rakekniven@users.noreply.github.com>
nickvergessen pushed a commit that referenced this issue Dec 20, 2022
Solution for #35768

Reported at Transifex.

Signed-off-by: rakekniven <2069590+rakekniven@users.noreply.github.com>
@rakekniven
Copy link
Member Author

My PR to this issue had been hijacked by another improvement. This issue is still open.

@rakekniven rakekniven reopened this Dec 20, 2022
@rakekniven
Copy link
Member Author

@nickvergessen does not want to split productname and server version into two variables. As long as this is the case there is no way to align the update notification messages.

@nickvergessen nickvergessen closed this as not planned Won't fix, can't repro, duplicate, stale Dec 20, 2022
@nickvergessen
Copy link
Member

Closing as not planned as per discussion in #35769

@rakekniven
Copy link
Member Author

This time I am really sad. There was no discussion, just a statement and this is an example that the Nextcloud company and the community are not on the same level when it comes to decisions.
I can't provide code ok, but to justify this with non-existent discussion is bad.
This issue is not a bug report or a great improvement, but one of several examples where supporters have no voice.

Unknown to me, my screenshot is now gone ...

@nickvergessen
Copy link
Member

nickvergessen commented Dec 20, 2022

Unknown to me, my screenshot is now gone ...

It's in the review which is "resolved" which automatically folds it away:
#35769 (review)

There was no discussion

Sorry for the poor choice of wording then: s/discussion/comment thread
The data string comes from a remote server, it could be Nextcloud followed by a version number, it could be something else, so splitting it is not a good idea.

I even tried to outline that using the same wording is not a good idea, because they have different impact levels for admins in terms of the effort they may need to invest.

This issue is not a bug report or a great improvement, but one of several examples where supporters have no voice.

I made a suggestion after some comments and you commented on the proposed approach "Your suggestion will solve this partly."
So I thought it's done. There was never any intention to outweigh a supporters voice here!

@nickvergessen
Copy link
Member

Background / information

Because I didn't want to make you sad, I went to check it again:

But I just remembered that daily could be special and indeed the notifications are skipped in that case:

if (\in_array($this->getChannel(), ['daily', 'git'], true)) {
// "These aren't the update channels you're looking for." - Ben Obi-Wan Kenobi
return;
}

So we can ignore that.


Option 1: combine app name and version

If alignment is so strongly desired that it would make you sad, we could try to do it the other way around and replace

$notification->setRichSubject($l->t('Update for {app} to version %s is available.', [$notification->getObjectId()]), [
'app' => [
'type' => 'app',
'id' => $notification->getObjectType(),
'name' => $appName,
]
]);

with:

			$notification->setRichSubject($l->t('Update to {app} is available.'), [
				'app' => [
					'type' => 'app',
					'id' => $notification->getObjectType(),
					'name' => $appName . ' ' . $notification->getObjectId(),
				]
			]);

resulting in Update to **Mail 4.2.0** is available
But might be worse translation-wise?


Option 2: attempt careful string split

Alternatively we could try to regex match the versionstring of server to something like ^Nextcloud\ (\d+\.\d+\.\d+)$
If it matches, we can try the string split and otherwise we keep the old translation string where we just insert whatever the updater server sent?

@nickvergessen nickvergessen reopened this Dec 20, 2022
@nickvergessen nickvergessen added 1. to develop Accepted and waiting to be taken care of and removed 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Dec 20, 2022
@rakekniven
Copy link
Member Author

As a user I do not understand why the product name and version info needs to be bundled. But if the code is like that and there is no easy way to split it we should keep it as it is. Using regex to split it sounds like a workaround to me. But as stated before I am not deep into the code. I will close here as it is not that important for the project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants