Summary: | "Copy to Clipboard in English" not fully englished | ||
---|---|---|---|
Product: | [Applications] kinfocenter | Reporter: | Alexander Potashev <aspotashev> |
Component: | About this System | Assignee: | Claudius Ellsel <claudius.ellsel> |
Status: | CLOSED FIXED | ||
Severity: | wishlist | CC: | ashark, bizyaev, claudius.ellsel, nate, sitter |
Priority: | NOR | ||
Version: | 5.15.4 | ||
Target Milestone: | --- | ||
Platform: | Fedora RPMs | ||
OS: | Linux | ||
Latest Commit: | https://invent.kde.org/plasma/kinfocenter/commit/97b66813bcfaeec046fc36dd4f5a8021b2a55264 | Version Fixed In: | 5.19.2 |
Sentry Crash Report: |
Description
Alexander Potashev
2019-05-03 12:35:53 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 *** Bug 416992 has been marked as a duplicate of this bug. *** 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. 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. 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. 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. 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. 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. A possibly relevant merge request was started @ https://invent.kde.org/plasma/kinfocenter/-/merge_requests/6 A possibly relevant merge request was started @ https://invent.kde.org/plasma/kinfocenter/-/merge_requests/6 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. 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 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 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 (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? 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 ^^ 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. 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 ГиБ ОЗУ". The "bit" is Englishified correctly: Kernel Version: 6.4.8-arch1-1 (64-bit) |