Bug 407179 - "Copy to Clipboard in English" not fully englished
Summary: "Copy to Clipboard in English" not fully englished
Status: CLOSED FIXED
Alias: None
Product: kinfocenter
Classification: Applications
Component: About this System (show other bugs)
Version: 5.15.4
Platform: Fedora RPMs Linux
: NOR wishlist
Target Milestone: ---
Assignee: Claudius Ellsel
URL:
Keywords:
: 416992 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-05-03 12:35 UTC by Alexander Potashev
Modified: 2023-08-08 14:32 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.19.2
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Potashev 2019-05-03 12:35:53 UTC
SUMMARY
copy to clipboard returns:
Операционная система: Fedora 30
Версия KDE Plasma: 5.15.4
Версия KDE Frameworks: 5.55.0
Версия Qt: 5.12.1
Версия ядра: 5.0.9-301.fc30.x86_64
Архитектура: 64-битная
Процессоры: 2 × Intel® Celeron® CPU B800 @ 1.50GHz
Память: 3,8 ГиБ ОЗУ

copy to clipboard in English returns:
Operating System: Fedora 30
KDE Plasma Version: 5.15.4
KDE Frameworks Version: 5.55.0
Qt Version: 5.12.1
Kernel Version: 5.0.9-301.fc30.x86_64
OS Type: 64-bit
Processors: 2 × Intel® Celeron® CPU B800 @ 1.50GHz
Memory: 3,8 ГиБ

OBSERVED RESULT
Memory: 3,8 ГиБ

EXPECTED RESULT
Memory: 3.8 GiB

SOFTWARE/OS VERSIONS
Operating System: Fedora 30
KDE Plasma Version: 5.15.4
KDE Frameworks Version: 5.55.0
Qt Version: 5.12.1
Kernel Version: 5.0.9-301.fc30.x86_64
OS Type: 64-bit
Processors: 2 × Intel® Celeron® CPU B800 @ 1.50GHz
Memory: 3,8 ГиБ

ADDITIONAL INFORMATION
Comment 1 Christoph Feck 2019-05-21 19:22:59 UTC
KFormat::formatByteSize() is used to get the memory size string.

KFormat can be passed a QLocale.

https://cgit.kde.org/kinfocenter.git/tree/Modules/about-distro/src/Module.cpp#n276

https://doc.qt.io/qt-5/qlocale.html#c
Comment 2 Harald Sitter 2020-02-03 09:36:47 UTC
*** Bug 416992 has been marked as a duplicate of this bug. ***
Comment 3 Claudius Ellsel 2020-06-14 12:52:53 UTC
Additionally in German it results in:

"Operating System: Manjaro Linux
KDE Plasma Version: 5.19.0
KDE Frameworks Version: 5.70.0
Qt Version: 5.15.0
Kernel Version: 5.6.17-1-MANJARO
OS Type: 64-bit
Processors: 4 × Intel® Xeon® CPU E3-1225 v3 @ 3.20GHz
Memory: 3,8 GiB Arbeitsspeicher
Graphics Processor: Mesa DRI Intel® HD Graphics P4600/P4700"

"Memory: 3,8 GiB Arbeitsspeicher"

Might be the same cause, though.
Comment 4 Harald Sitter 2020-06-15 13:45:54 UTC
Related at least. The memory entry as a whole isn't doing the string building delegation correctly making the entire entry unaware of the language context.
Comment 5 Claudius Ellsel 2020-06-15 18:31:33 UTC
I just had a closer look on this.

Since the link to cgit is not working anymore, I guess this line was meant: https://invent.kde.org/plasma/kinfocenter/-/blob/10d60170eaa46c3e4233c4eef966418d0065754d/Modules/about-distro/src/Module.cpp#L276

In the meantime there has been some refactoring going on. This abstraction unfortunately did not take into consideration that one needs to get the English version of translated values back. In the past that has been achieved by having a variable "englishTextForClipboard". This got removed with https://invent.kde.org/plasma/kinfocenter/-/commit/4b4f6a3f9639f416948f6810bcd64b61d240e4cc

For Memory, this is the corresponding code: https://invent.kde.org/plasma/kinfocenter/-/blob/master/Modules/about-distro/src/MemoryEntry.cpp#L46

The refactoring seems to have caused a regression that now "Arbeitsspeicher" appears in the English clipboard version. Also it makes fixing the original bug much harder, unfortunately.

On the other hand, I guess having a separate English Strings for the clipboard that would have to be maintained was also not really nice. I think that actually lead to inconsistencies between this English clipboard version and a version that would appear on a real English setup.

So maybe some better way of only using one String can be found. I am not really experienced in this field, so hopefully you can help me out a bit here Harald.
Comment 6 Harald Sitter 2020-06-15 19:44:08 UTC
Well, same as with the 'text' member the 'value' member translation needs to get deferred to the Entry class (currently it is in the MemoryEntry and thus always the language of the system, not the requested one).

With that in mind I'd say Entry needs to grow another constructor that takes a KLocalizedString for the value. Or just change the value member to always be a KLocalizedString maybe. At any rate, the MemoryEntry needs to give a KLocalizedString to the Entry base class somehow so it can then contextually localize it (see localizedLabel for how that works).

Hope that makes sense.
Comment 7 Claudius Ellsel 2020-06-15 20:06:27 UTC
I came up with similar ideas and tried some of them unfortunately with no luck until now.

Thanks for the idea to just use another constructor. I tried to change it to KLocalizedString for value altogether and did not really like that given that there are at least currently not always localized Strings for the values.

I tried implementing a localizedValue method, but then figured that I first need to tackle the problem of retrieving a KLocalizedString in MemoryEntry somehow. I did not really find useful documentation on that, unfortunately. I might look into it a bit deeper tomorrow, if I get the time.
Comment 8 Harald Sitter 2020-06-16 08:41:32 UTC
Yeah, I think this issue is actually a bit ballooned up in scope by design mistakes I've made. You are on the right track though.

Upon further inspection I don't think a constructor will do. We not only need the KLocalizedString to be evaluated "later" but also the KFormat substitution. That complicates things beyond what we can achieve by passing the KLS around. Instead we'll need a virtual function here:

Entry's member `value` needs to become a private m_value for starters.
To replace it we'll need a `virtual QString value(Language language = Language::System)` which by default simply returns the string member. This function can then be selectively overridden in Entries that need to localize the value. Functionally this would behave like localizedLabel.

Now that we can override value() in MemoryEntry things are a bit simpler.
In the MemoryEntry's value() override we'll want to recycle the logic current in text() and massage the code a bit. We'll need

a) to deal with KLocalizedStrings instead of QString
b) get a QLocale for KFormat
c) need a more generic `QString localize(const KLocalizedString &string, Language language) const` function to help us deal with localizing our strings

a) is kinda simple, albeit not well documented. i18n*() calls become ki18n*() calls. Additionally if they have substitutions those need to be calls on the KLS.

e.g. in our specific case i18nc(..., KFormat()) becomes ki18nc(...).subs(KFormat()). Where ki18nc() gives us the KLS and with subs() we replace the %N marker.

b) should simply be a switch over Language mapping to QLocale::system() or QLocale(Enlish, US). It'd probably be a good idea to pack that in a helper function `QLocale localeForLanguage(Language language) const` for ease of reading

c) could be easily built out of refactoring localizedLabel. it'd simply need to switch over language and call the right .toString() on the KLS

With that in place you should be able to achieve englishification.
Comment 9 Bug Janitor Service 2020-06-17 12:54:31 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/kinfocenter/-/merge_requests/6
Comment 10 Bug Janitor Service 2020-06-17 12:54:32 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/kinfocenter/-/merge_requests/6
Comment 11 Claudius Ellsel 2020-06-17 13:00:26 UTC
Thanks for the helping hints! I created a merge request for this now: https://invent.kde.org/plasma/kinfocenter/-/merge_requests/6

Since switching value to a private m_value would have required some more refactoring, I came up with a version that does not require to do that.

Also I did not understand all of your proposal, so I came up with a slightly modified version. Reading your suggestions again, I think it is doing most of the things similarly, although maybe not as nice as proposed.
Comment 12 Harald Sitter 2020-06-18 08:56:01 UTC
Git commit 97b66813bcfaeec046fc36dd4f5a8021b2a55264 by Harald Sitter, on behalf of Claudius Ellsel.
Committed on 18/06/2020 at 08:32.
Pushed by sitter into branch 'Plasma/5.19'.

Fix English copy to clipboard version

previously the value of the entries wasn't supporting deferred translation and thus always was localized
FIXED-IN: 5.19.2

M  +28   -5    Modules/about-distro/src/Entry.cpp
M  +11   -0    Modules/about-distro/src/Entry.h
M  +7    -5    Modules/about-distro/src/MemoryEntry.cpp
M  +3    -1    Modules/about-distro/src/MemoryEntry.h

https://invent.kde.org/plasma/kinfocenter/commit/97b66813bcfaeec046fc36dd4f5a8021b2a55264
Comment 13 Harald Sitter 2020-06-18 08:56:01 UTC
Git commit 95ee875df46f0541c1e419bcf8bbccbfd3286df4 by Harald Sitter.
Committed on 18/06/2020 at 08:50.
Pushed by sitter into branch 'Plasma/5.19'.

also localize the bit entry

M  +5    -4    Modules/about-distro/src/BitEntry.cpp
M  +1    -1    Modules/about-distro/src/BitEntry.h

https://invent.kde.org/plasma/kinfocenter/commit/95ee875df46f0541c1e419bcf8bbccbfd3286df4
Comment 14 Harald Sitter 2020-06-18 08:58:26 UTC
Someone might want to file a bug against kcoreaddons because it doesn't localize kformat correctly either. Still spits out cyrillic when run with LANGUAGE=ru
Comment 15 Claudius Ellsel 2020-10-15 16:02:32 UTC
(In reply to Harald Sitter from comment #14)
> Someone might want to file a bug against kcoreaddons because it doesn't
> localize kformat correctly either. Still spits out cyrillic when run with
> LANGUAGE=ru

Sorry, I had this tab open for a long time and did not reply. Unfortunately I did not really get what you meant by that, does it have the same bug as described here? Has there already been a bug filed in the meantime?
Comment 16 Harald Sitter 2020-10-16 09:51:19 UTC
Same bug as here but inside KFormat itself. Feel free to test various environment combinations, maybe it went away on its own in the mean time ^^
Comment 17 Claudius Ellsel 2020-10-16 15:26:19 UTC
Unfortunately I don't have that much time to invest into that currently. To not have this forgotten, I filed https://bugs.kde.org/show_bug.cgi?id=427814 now with a reference to your comment.
Comment 18 Andrew Shark 2023-08-08 14:27:19 UTC
This is not fixed.

Operating System: Arch Linux 
KDE Plasma Version: 5.27.7
KDE Frameworks Version: 5.108.0
Qt Version: 5.15.10
Kernel Version: 6.4.8-arch1-1 (64-bit)
Graphics Platform: Wayland
Memory: 15.3 ГиБ of RAM
Graphics Processor: Mesa Intel® Graphics

Intentionally did not edited Memory line manually :). It is partly localized though. Because in Russian it says "15,3 ГиБ ОЗУ".
Comment 19 Andrew Shark 2023-08-08 14:31:25 UTC
The "bit" is Englishified correctly:
Kernel Version: 6.4.8-arch1-1 (64-bit)
Comment 20 Harald Sitter 2023-08-08 14:32:08 UTC
https://bugs.kde.org/show_bug.cgi?id=427814