Differential D29270

Fix broken localization of kcmkwinrules
AbandonedPublic
Actions

Authored by yurchor on Apr 29 2020, 5:49 AM.

Details

Reviewers
broulik
Group Reviewers
KWin
Summary

Current extraction and applying translations are broken.

Patch mostly by Victor Ryzhykh.

Test Plan

Run extract-messages.sh manually, test in vivo.

Diff Detail

Repository
R108 KWin
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26106
Build 26124: arc lint + arc unit
yurchor created this revision.Apr 29 2020, 5:49 AM
Restricted Application added a project: KWin. · View Herald TranscriptApr 29 2020, 5:49 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
yurchor requested review of this revision.Apr 29 2020, 5:49 AM
yurchor edited the summary of this revision. (Show Details)Apr 29 2020, 5:51 AM
zzag added a subscriber: zzag.Apr 29 2020, 6:05 AM
zzag added inline comments.
kcmkwin/kwinrules/Messages.sh
2

If we rename the pot file to kcm_kwinrules.pot and adjust TRANSLATION_DOMAIN in CMakeLists.txt, will we still need to use i18nd?

yurchor added a subscriber: ltoscano.Apr 29 2020, 6:08 AM
yurchor added inline comments.
kcmkwin/kwinrules/Messages.sh
2

@ltoscano Can you clear up the current rules of catalog naming for this case? Thanks in advance for your help.

pino added a subscriber: pino.Apr 29 2020, 6:26 AM
pino added inline comments.
kcmkwin/kwinrules/Messages.sh
2

TRANSLATION_DOMAIN is a C/C++ define, it does not apply to strings in QML files.

zzag added inline comments.Apr 29 2020, 6:28 AM
kcmkwin/kwinrules/Messages.sh
2

Could someone explain why we don't need to use i18nd in Destop Effects or Virtual Desktops KCM?

broulik requested changes to this revision.EditedApr 29 2020, 6:37 AM
broulik added a subscriber: broulik.

I bet the KAboutData in the KCM c++ is wrong. The component name determines the translation domain to be used on the KCM QML side.

This revision now requires changes to proceed.Apr 29 2020, 6:37 AM
ltoscano added a comment.Apr 29 2020, 7:59 AM
In D29270#659495, @broulik wrote:

I bet the KAboutData in the KCM c++ is wrong. The component name determines the translation domain to be used on the KCM QML side.

Some time ago I asked if it was a way to have the QML-based KCMs use the same way for setting the translation domain as the C++ ones. This is also needed for all KCMs defined outside Plasma, because technically they should be called kcm5_<foo> to prevent co-installability issues (with kdelibs 4.x back then, with the KF6 ones in a few months). Would it be possible to revisit this point? We are going to have installation conflicts when KF6 is out otherwise.

broulik added a comment.Apr 29 2020, 8:00 AM

Would it be possible to revisit this point? We are going to have installation conflicts when KF6 is out otherwise.

Feel free to add a task to the allmighty KF6 board https://phabricator.kde.org/tag/kf6/

yurchor updated this revision to Diff 81488.Apr 29 2020, 8:09 AM

Change KAboutData to be in sync with the translation catalog name

ltoscano mentioned this in T13056: Uniform way to set the naming of KCM gettext catalog files.Apr 29 2020, 8:15 AM
In D29270#659529, @broulik wrote:

Would it be possible to revisit this point? We are going to have installation conflicts when KF6 is out otherwise.

Feel free to add a task to the allmighty KF6 board https://phabricator.kde.org/tag/kf6/

Added T13056, even though it is not KF6-specific (but I guess it would be easier to implement it directly there).

victorr added a subscriber: victorr.Apr 29 2020, 7:00 PM

Set the value to "auto about = new KAboutData(QStringLiteral("kcmkwinrules"),"
Got such an error.

yurchor updated this revision to Diff 81544.Apr 29 2020, 7:18 PM

Move i18nd() back

yurchor updated this revision to Diff 81545.Apr 29 2020, 7:20 PM

Replace KAboutData with an old value

victorr added a comment.EditedApr 29 2020, 7:21 PM

If you rename it to “kcm_kwinrules” with the attached patch, then the translation works.
Now my packages are going to check.

source-fix.patch983 BDownload

victorr added a comment.Apr 29 2020, 7:34 PM
In D29270#659983, @victorr wrote:

If you rename it to “kcm_kwinrules” with the attached patch, then the translation works.
Now my packages are going to check.

source-fix.patch983 BDownload

Yes, renaming to “kcm_kwinrules” corrects the display of the translation.

victorr added a comment.EditedApr 30 2020, 1:28 AM
In D29270#659983, @victorr wrote:

If you rename it to “kcm_kwinrules” with the attached patch, then the translation works.
Now my packages are going to check.

source-fix.patch983 BDownload

Yes, renaming to “kcm_kwinrules” corrects the display of the translation.

No, not everywhere translation works.
It took one more patch to the previous one.

kwin-fix.patch9 KBDownload

Before

After

yurchor added a comment.May 2 2020, 3:43 PM

So what is the resolution? What should I do with this?

Thanks in advance for your advice.

davidedmundson added a subscriber: davidedmundson.May 4 2020, 12:06 PM
davidedmundson added inline comments.
kcmkwin/kwinrules/Messages.sh
2

It all comes from

configmodule.cpp in kdeclarative

QQuickItem *ConfigModule::mainUi()

d->_qmlObject->setTranslationDomain(aboutData()->componentName());

If this is set then anything loaded within this QML Context will have the right domain.
We do use ConfigModule in this case.

kcmrules.cpp: auto about = new KAboutData(QStringLiteral("kcm_kwinrules"),

So as Vlad says from what I can tell if this pot is renamed it should magically start working without i18nd.

broulik requested changes to this revision.May 4 2020, 12:20 PM

KAboutData is used for translation context and for finding the plug-in. Make sure to update the install location in kpackage5/ in CMake when changing it

This revision now requires changes to proceed.May 4 2020, 12:20 PM
yurchor abandoned this revision.May 4 2020, 1:12 PM

This cannot be easily merged anymore (files were changed meanwhile) so I prefer to abandon this revision.

@broulik Can you fix it if you know how to do this please?

victorr added a comment.May 4 2020, 1:21 PM
In D29270#662900, @yurchor wrote:

This cannot be easily merged anymore (files were changed meanwhile) so I prefer to abandon this revision.

Maybe I should make a new patch?
Today I planned to build kwin with this commit https://cgit.kde.org/kwin.git/commit/?id=1a3cb256d74166ba175f4634904abea84928b635
Although you can wait with this.:)

yurchor added a comment.May 4 2020, 1:25 PM
In D29270#662923, @victorr wrote:
In D29270#662900, @yurchor wrote:

This cannot be easily merged anymore (files were changed meanwhile) so I prefer to abandon this revision.

Maybe I should make a new patch?
Today I planned to build kwin with this commit https://cgit.kde.org/kwin.git/commit/?id=1a3cb256d74166ba175f4634904abea84928b635
Although you can wait with this.:)

Whatever you want.

Personally, I think that the whole system is wrong. Every single release someone break the translation of KCM, there is no conventional naming, conventional style, and written rules. Just a prediction: the next release of Plasma will have broken KCMs too.

victorr added a comment.May 4 2020, 1:54 PM
In D29270#662931, @yurchor wrote:
In D29270#662923, @victorr wrote:
In D29270#662900, @yurchor wrote:

This cannot be easily merged anymore (files were changed meanwhile) so I prefer to abandon this revision.

Maybe I should make a new patch?
Today I planned to build kwin with this commit https://cgit.kde.org/kwin.git/commit/?id=1a3cb256d74166ba175f4634904abea84928b635
Although you can wait with this.:)

Whatever you want.

Made a new patch.

kwin-fix.string.patch10 KBDownload

yurchor added a comment.May 4 2020, 1:57 PM
In D29270#662958, @victorr wrote:

Made a new patch.

kwin-fix.string.patch10 KBDownload

It will not be accepted because it uses i18nd(). @broulik wants this problem to be resolved on KAboutData/CMakeLists.txt level.

victorr added a comment.EditedMay 4 2020, 2:10 PM
In D29270#662975, @yurchor wrote:
In D29270#662958, @victorr wrote:

Made a new patch.

kwin-fix.string.patch10 KBDownload

It will not be accepted because it uses i18nd(). @broulik wants this problem to be resolved on KAboutData/CMakeLists.txt level.

It would be nice.
And when I tried this option, this error got out

ltoscano added a comment.May 4 2020, 2:18 PM

Just please patch Messages.sh to rename the file, I will take care of moving it.

Regarding the more general issue of the name of the template, as I mentioned earlier, I created T13056 . It would be good if it was fixed before KF6 though.

broulik added a comment.May 4 2020, 2:19 PM

It would be nice.
And when I tried this option, this error got out

Because you need to fix the damn location where the package is installed

yurchor added a comment.May 4 2020, 2:31 PM
In D29270#663017, @broulik wrote:

It would be nice.
And when I tried this option, this error got out

Because you need to fix the damn location where the package is installed

I will not create additional differential revisions. I do not know what "kcmkwinrules" should be left and what should be replaced with "kcm_kwinrules" and cannot test by myself.

Dear Plasma developers, can you resolve this problem without incompetent translators?

victorr added a comment.May 4 2020, 2:34 PM
In D29270#663017, @broulik wrote:

It would be nice.
And when I tried this option, this error got out

Because you need to fix the damn location where the package is installed

I did with the patch from this post
https://phabricator.kde.org/D29270#659987
At first I was pleased that everything was translated.
Then I saw an error in setting the window title, and made another patch, which is here https://phabricator.kde.org/D29270#660131

Revision Contents
Changeset List

Diff 81544

View Options

kcmkwin/kwinrules/Messages.sh

Loading...
View Options

kcmkwin/kwinrules/kcmrules.cpp

Loading...
View Options

kcmkwin/kwinrules/package/contents/ui/FileDialogLoader.qml

Loading...
View Options

kcmkwin/kwinrules/package/contents/ui/OptionsComboBox.qml

Loading...
View Options

kcmkwin/kwinrules/package/contents/ui/RulesEditor.qml

Loading...
View Options

kcmkwin/kwinrules/package/contents/ui/RulesList.qml

Loading...
View Options

kcmkwin/kwinrules/package/contents/ui/ValueEditor.qml

Loading...