Changeset View
Standalone View
Modules/about-distro/src/Module.cpp
| Show All 15 Lines | 1 | /* | |||
|---|---|---|---|---|---|
| 16 | 16 | | |||
| 17 | You should have received a copy of the GNU General Public License | 17 | You should have received a copy of the GNU General Public License | ||
| 18 | along with this program. If not, see <http://www.gnu.org/licenses/>. | 18 | along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
| 19 | */ | 19 | */ | ||
| 20 | 20 | | |||
| 21 | #include "Module.h" | 21 | #include "Module.h" | ||
| 22 | #include "ui_Module.h" | 22 | #include "ui_Module.h" | ||
| 23 | 23 | | |||
| 24 | #include <QClipboard> | ||||
| 25 | #include <QMenu> | ||||
| 26 | #include <KHelpMenu> | ||||
| 27 | | ||||
| 24 | #include <QIcon> | 28 | #include <QIcon> | ||
| 25 | #include <QStandardPaths> | 29 | #include <QStandardPaths> | ||
| 26 | 30 | | |||
| 27 | #include <kcoreaddons_version.h> | 31 | #include <kcoreaddons_version.h> | ||
| 28 | #include <KAboutData> | 32 | #include <KAboutData> | ||
| 29 | #if KCOREADDONS_VERSION >= QT_VERSION_CHECK(5,20,0) | | |||
ngraham: Is this necessary for this patch? If not, let's address it in a different patch or commit. | |||||
This was a suggestion from the discussion on ReviewBoard. Is there an obvious way to tell arc to do this in a separate commit? Otherwise I'll try to separate this somehow manually. gregormi: This was a suggestion from the discussion on ReviewBoard. Is there an obvious way to tell arc… | |||||
I just saw that this change was already done with 8193488408011c4971711b81b782bb003ffd1c3d gregormi: I just saw that this change was already done with 8193488408011c4971711b81b782bb003ffd1c3d | |||||
| 30 | #include <KCoreAddons> | 33 | #include <KCoreAddons> | ||
| 31 | #endif | | |||
| 32 | #include <KConfig> | 34 | #include <KConfig> | ||
| 33 | #include <KConfigGroup> | 35 | #include <KConfigGroup> | ||
| 34 | #include <KDesktopFile> | 36 | #include <KDesktopFile> | ||
| 35 | #include <KFormat> | 37 | #include <KFormat> | ||
| 36 | #include <KLocalizedString> | 38 | #include <KLocalizedString> | ||
| 37 | #include <KSharedConfig> | 39 | #include <KSharedConfig> | ||
| 38 | 40 | | |||
| 39 | #include <solid/device.h> | 41 | #include <solid/device.h> | ||
| ▲ Show 20 Lines • Show All 76 Lines • ▼ Show 20 Line(s) | |||||
| 116 | { | 118 | { | ||
| 117 | delete ui; | 119 | delete ui; | ||
| 118 | } | 120 | } | ||
| 119 | 121 | | |||
| 120 | void Module::load() | 122 | void Module::load() | ||
| 121 | { | 123 | { | ||
| 122 | loadSoftware(); | 124 | loadSoftware(); | ||
| 123 | loadHardware(); | 125 | loadHardware(); | ||
| 126 | | ||||
| 127 | connect(ui->pushButtonCopyInfo, &QPushButton::clicked, this, &Module::copyToClipboard); | ||||
| 124 | } | 128 | } | ||
| 125 | 129 | | |||
| 126 | void Module::save() | 130 | void Module::save() | ||
| 127 | { | 131 | { | ||
| 128 | } | 132 | } | ||
| 129 | 133 | | |||
| 130 | void Module::defaults() | 134 | void Module::defaults() | ||
| 131 | { | 135 | { | ||
| 132 | } | 136 | } | ||
| 133 | 137 | | |||
| 134 | void Module::loadSoftware() | 138 | void Module::loadSoftware() | ||
| 135 | { | 139 | { | ||
| 136 | // NOTE: do not include globals, otherwise kdeglobals could provide values | 140 | // NOTE: do not include globals, otherwise kdeglobals could provide values | ||
| 137 | // even though we only explicitly want them from our own config. | 141 | // even though we only explicitly want them from our own config. | ||
| 138 | KSharedConfig::Ptr config = KSharedConfig::openConfig(QStringLiteral("kcm-about-distrorc"), | 142 | KSharedConfig::Ptr config = KSharedConfig::openConfig(QStringLiteral("kcm-about-distrorc"), | ||
| 139 | KConfig::NoGlobals); | 143 | KConfig::NoGlobals); | ||
| 140 | KConfigGroup cg = KConfigGroup(config, "General"); | 144 | KConfigGroup cg = KConfigGroup(config, "General"); | ||
| 141 | 145 | | |||
| 142 | QString logoPath = cg.readEntry("LogoPath", QString()); | 146 | const QString logoPath = cg.readEntry("LogoPath", QString()); | ||
| 143 | QPixmap logo; | 147 | QPixmap logo; | ||
| 144 | if (logoPath.isEmpty()) { | 148 | if (logoPath.isEmpty()) { | ||
| 145 | logo = QIcon::fromTheme(QStringLiteral("start-here-kde")).pixmap(128, 128); | 149 | logo = QIcon::fromTheme(QStringLiteral("start-here-kde")).pixmap(128, 128); | ||
| 146 | } else { | 150 | } else { | ||
| 147 | logo = QPixmap(logoPath); | 151 | logo = QPixmap(logoPath); | ||
| 148 | } | 152 | } | ||
| 149 | ui->logoLabel->setPixmap(logo); | 153 | ui->logoLabel->setPixmap(logo); | ||
| 150 | 154 | | |||
| 151 | OSRelease os; | 155 | OSRelease os; | ||
| 152 | // We allow overriding of the OS name for branding purposes. | 156 | // We allow overriding of the OS name for branding purposes. | ||
| 153 | // For example OS Ubuntu may be rebranded as Kubuntu. Also Kubuntu Active | 157 | // For example OS Ubuntu may be rebranded as Kubuntu. Also Kubuntu Active | ||
| 154 | // as a product brand is different from Kubuntu. | 158 | // as a product brand is different from Kubuntu. | ||
| 155 | QString distroName = cg.readEntry("Name", os.name); | 159 | const QString distroName = cg.readEntry("Name", os.name); | ||
| 156 | QString versionId = cg.readEntry("Version", os.versionId); | 160 | const QString versionId = cg.readEntry("Version", os.versionId); | ||
| 161 | collectedData["4_Distro"] = distroName + " " + versionId; | ||||
| 157 | ui->nameVersionLabel->setText(QStringLiteral("%1 %2").arg(distroName, versionId)); | 162 | ui->nameVersionLabel->setText(QStringLiteral("%1 %2").arg(distroName, versionId)); | ||
| 158 | 163 | | |||
| 159 | QString variant = cg.readEntry("Variant", QString()); | 164 | const QString variant = cg.readEntry("Variant", QString()); | ||
rkflx: You could add a `const`. | |||||
dhaumann: Is "Distro" a proper English term? Should it meaybe read "Distribution"? | |||||
"Distro" is slang, but very common. I'm not sure "Distribution" is much better if were considering average people. They would consider the software on their computer to be an "Operating System." For that matter, distros themselves might appreciate being referred to as operating systems here. Perhaps we should use that term instead. ngraham: "Distro" is slang, but very common. I'm not sure "Distribution" is much better if were… | |||||
| 160 | if (variant.isEmpty()) { | 165 | if (variant.isEmpty()) { | ||
| 161 | ui->variantLabel->hide(); | 166 | ui->variantLabel->hide(); | ||
| 162 | } else { | 167 | } else { | ||
| 163 | ui->variantLabel->setText(variant); | 168 | ui->variantLabel->setText(variant); | ||
| 164 | } | 169 | } | ||
| 165 | 170 | | |||
| 166 | QString url = cg.readEntry("Website", os.homeUrl); | 171 | const QString url = cg.readEntry("Website", os.homeUrl); | ||
| 167 | if (url.isEmpty()) { | 172 | if (url.isEmpty()) { | ||
| 168 | ui->urlLabel->hide(); | 173 | ui->urlLabel->hide(); | ||
| 169 | } else { | 174 | } else { | ||
| 170 | ui->urlLabel->setText(QStringLiteral("<a href='%1'>%1</a>").arg(url)); | 175 | ui->urlLabel->setText(QStringLiteral("<a href='%1'>%1</a>").arg(url)); | ||
| 171 | } | 176 | } | ||
| 172 | 177 | | |||
| 173 | // Since Plasma version detection isn't based on a library query it can fail | 178 | // Since Plasma version detection isn't based on a library query it can fail | ||
| 174 | // in weird cases; instead of admiting defeat we simply hide everything :P | 179 | // in weird cases; instead of admiting defeat we simply hide everything :P | ||
| 175 | QString plasma = plasmaVersion(); | 180 | const QString plasma = plasmaVersion(); | ||
| 176 | if (plasma.isEmpty()) { | 181 | if (plasma.isEmpty()) { | ||
| 177 | ui->plasma->hide(); | 182 | ui->plasma->hide(); | ||
| 178 | ui->plasmaLabel->hide(); | 183 | ui->plasmaLabel->hide(); | ||
| 179 | } else { | 184 | } else { | ||
| 185 | collectedData["1_Plasma Version"] = plasma; | ||||
| 180 | ui->plasmaLabel->setText(plasma); | 186 | ui->plasmaLabel->setText(plasma); | ||
| 181 | } | 187 | } | ||
| 182 | 188 | | |||
| 183 | ui->qtLabel->setText(qVersion()); | 189 | const QString qversion = qVersion(); | ||
| 184 | 190 | collectedData["3_Qt Version"] = qversion; | |||
| 185 | #if KCOREADDONS_VERSION >= QT_VERSION_CHECK(5,20,0) | 191 | ui->qtLabel->setText(qversion); | ||
| 186 | ui->frameworksLabel->setText(KCoreAddons::versionString()); | 192 | | ||
| 187 | #else | 193 | const QString frameworksVersion = KCoreAddons::versionString(); | ||
| 188 | ui->frameworksLabelKey->setVisible(false); | 194 | collectedData["2_KDE Frameworks Version"] = frameworksVersion; | ||
| 189 | ui->frameworksLabel->setVisible(false); | 195 | ui->frameworksLabel->setText(frameworksVersion); | ||
| 190 | #endif | | |||
| 191 | } | 196 | } | ||
| 192 | 197 | | |||
| 193 | void Module::loadHardware() | 198 | void Module::loadHardware() | ||
| 194 | { | 199 | { | ||
| 195 | struct utsname utsName; | 200 | struct utsname utsName; | ||
| 196 | if(uname(&utsName) != 0) { | 201 | if(uname(&utsName) != 0) { | ||
| 197 | ui->kernel->hide(); | 202 | ui->kernel->hide(); | ||
| 198 | ui->kernelLabel->hide(); | 203 | ui->kernelLabel->hide(); | ||
| 199 | } else { | 204 | } else { | ||
| 205 | collectedData["4_Kernel"] = utsName.release; | ||||
| 200 | ui->kernelLabel->setText(utsName.release); | 206 | ui->kernelLabel->setText(utsName.release); | ||
| 201 | } | 207 | } | ||
| 202 | 208 | | |||
| 203 | const int bits = QT_POINTER_SIZE == 8 ? 64 : 32; | 209 | const int bits = QT_POINTER_SIZE == 8 ? 64 : 32; | ||
| 210 | const QString bitsStr = QString::number(bits); | ||||
| 211 | collectedData["5_OS Type"] = bitsStr + " bit"; | ||||
| 204 | ui->bitsLabel->setText(i18nc("@label %1 is the CPU bit width (e.g. 32 or 64)", | 212 | ui->bitsLabel->setText(i18nc("@label %1 is the CPU bit width (e.g. 32 or 64)", | ||
| 205 | "%1-bit", QString::number(bits))); | 213 | "%1-bit", bitsStr)); | ||
| 206 | 214 | | |||
| 207 | const QList<Solid::Device> list = Solid::Device::listFromType(Solid::DeviceInterface::Processor); | 215 | const QList<Solid::Device> list = Solid::Device::listFromType(Solid::DeviceInterface::Processor); | ||
| 208 | ui->processor->setText(i18np("Processor:", "Processors:", list.count())); | 216 | ui->processor->setText(i18np("Processor:", "Processors:", list.count())); | ||
| 209 | // Format processor string | 217 | // Format processor string | ||
| 210 | // Group by processor name | 218 | // Group by processor name | ||
| 211 | QMap<QString, int> processorMap; | 219 | QMap<QString, int> processorMap; | ||
| 212 | Q_FOREACH(const Solid::Device &device, list) { | 220 | Q_FOREACH(const Solid::Device &device, list) { | ||
| 213 | const QString name = device.product(); | 221 | const QString name = device.product(); | ||
| Show All 9 Lines | |||||
| 223 | for (auto it = processorMap.constBegin(); it != processorMap.constEnd(); ++it) { | 231 | for (auto it = processorMap.constBegin(); it != processorMap.constEnd(); ++it) { | ||
| 224 | const int count = it.value(); | 232 | const int count = it.value(); | ||
| 225 | QString name = it.key(); | 233 | QString name = it.key(); | ||
| 226 | name.replace(QStringLiteral("(TM)"), QChar(8482)); | 234 | name.replace(QStringLiteral("(TM)"), QChar(8482)); | ||
| 227 | name.replace(QStringLiteral("(R)"), QChar(174)); | 235 | name.replace(QStringLiteral("(R)"), QChar(174)); | ||
| 228 | name = name.simplified(); | 236 | name = name.simplified(); | ||
| 229 | names.append(QStringLiteral("%1 × %2").arg(count).arg(name)); | 237 | names.append(QStringLiteral("%1 × %2").arg(count).arg(name)); | ||
| 230 | } | 238 | } | ||
| 231 | ui->processorLabel->setText(names.join(QStringLiteral(", "))); | 239 | | ||
| 240 | const QString processorLabel = names.join(QStringLiteral(", ")); | ||||
| 241 | collectedData["6_Processors"] = processorLabel; | ||||
| 242 | ui->processorLabel->setText(processorLabel); | ||||
| 232 | if (ui->processorLabel->text().isEmpty()) { | 243 | if (ui->processorLabel->text().isEmpty()) { | ||
| 233 | ui->processor->setHidden(true); | 244 | ui->processor->setHidden(true); | ||
| 234 | ui->processorLabel->setHidden(true); | 245 | ui->processorLabel->setHidden(true); | ||
| 235 | } | 246 | } | ||
| 236 | 247 | | |||
| 237 | const qlonglong totalRam = calculateTotalRam(); | 248 | const qlonglong totalRam = calculateTotalRam(); | ||
| 238 | ui->memoryLabel->setText(totalRam > 0 | 249 | const QString memoryLabel = totalRam > 0 | ||
| 239 | ? i18nc("@label %1 is the formatted amount of system memory (e.g. 7,7 GiB)", | 250 | ? i18nc("@label %1 is the formatted amount of system memory (e.g. 7,7 GiB)", | ||
| 240 | "%1 of RAM", KFormat().formatByteSize(totalRam)) | 251 | "%1 of RAM", KFormat().formatByteSize(totalRam)) | ||
| 241 | : i18nc("Unknown amount of RAM", "Unknown")); | 252 | : i18nc("Unknown amount of RAM", "Unknown"); | ||
| 253 | collectedData["7_Memory"] = memoryLabel; | ||||
| 254 | ui->memoryLabel->setText(memoryLabel); | ||||
| 255 | } | ||||
| 256 | | ||||
Probably not strictly required for this patch, but it would be nicer to refactor this in such a way that adding another string to the UI does not require adding it here too. Perhaps this can be achieved by creating a list of label/version pairs, and then iterating through that list both when creating the UI and when generating the text to copy. rkflx: Probably not strictly required for this patch, but it would be nicer to refactor this in such a… | |||||
Yes, I also had the list idea in an earlier stage of the patch< but without reusing the translated label text. I don't expect the information labels to change often, so I think such a refactoring can be done at a later point in time. gregormi: Yes, I also had the list idea in an earlier stage of the patch< but without reusing the… | |||||
I changed my mind. With all the i18nc comments, I decided it is better to do the refactoring now. gregormi: I changed my mind. With all the i18nc comments, I decided it is better to do the refactoring… | |||||
| 257 | void Module::copyToClipboard() | ||||
| 258 | { | ||||
| 259 | auto clipboard = QGuiApplication::clipboard(); | ||||
Nitpicking: Why do you declare the clipboard here, when you use it only 10 lines later? :-) Better would be: QGuiApplication::clipboard()->setText(text); So just one line at the end. dhaumann: Nitpicking: Why do you declare the clipboard here, when you use it only 10 lines later? :-)… | |||||
gregormi: Good question :). I'll fix that. | |||||
| 260 | QMapIterator<QString, QString> i(collectedData); | ||||
ngraham: Is "Distro" not a translated string? | |||||
| 261 | QString text; | ||||
Maybe you could avoid this special case if you added a hidden label in loadSoftware? Also, "label in … the button" sounds a bit odd. Perhaps "label" would be enough, or use @title:row? (See https://api.kde.org/frameworks/ki18n/html/prg_guide.html) rkflx: Maybe you could avoid this special case if you added a hidden label in `loadSoftware`?
Also… | |||||
gregormi: Good idea. DONE. | |||||
I'm afraid you missed the "hidden" part, so it shows up right in front of the distro logo ;) Adding dummyDistroDescriptionLabel->hide(); where you are creating the label solves the issue for me. rkflx: I'm afraid you missed the "hidden" part, so it shows up right in front of the distro logo ;)… | |||||
gregormi: dummy hide: Oh sorry, I should have seen that myself. Fixed it. | |||||
Coming back to this after a month, I now wonder what p stands for, which might indicate that variable could get a better name… rkflx: Coming back to this after a month, I now wonder what `p` stands for, which might indicate that… | |||||
gregormi: p stands for labelPair: renaming is a good idea. Done. | |||||
| 262 | while (i.hasNext()) { | ||||
| 263 | i.next(); | ||||
@dhaumann I wonder why you suggested to use i18n here too, as text() should already give you the translated text. How do you expect translators to translate "%1 %2"? The only challenge is to account for RTL languages, but this could be detected to swap the order (and possibly add an RTL BOM?). Do we have experts who could advice on that? rkflx: @dhaumann I wonder why you suggested to use `i18n` here too, as `text()` should already give… | |||||
The reason for this is that in a right-to-left language, you would translated "%1 %2" by using the reverse, e.g. "%2 %1". The problem here is that a translator will not know what to do with this. So what would help is to give translators context, i.g. i18nc("reverse order in rtl languages", ...). Does that make sense? dhaumann: The reason for this is that in a right-to-left language, you would translated "%1 %2" by using… | |||||
It does, and it's probably easier for @gregormi to implement. rkflx: > `i18nc("reverse order in rtl languages", ...)`. Does that make sense?
It does, and it's… | |||||
Please don't write (just) "do this in this case", but explain what the placeholders are meant to be. Leave it to the speakers of the language the decision about the order. ltoscano: Please don't write (just) "do this in this case", but explain what the placeholders are meant… | |||||
I will do it like this: i18nc("one line in the information that goes to the clipboard", "%1 %2", ... I see one problem: %1 contains a trailing colon (:). So just reversing to "%2 %1" would result in "openSUSE Tumbleweed Distro:". One solution would be to strip the colon from the %1 string and put it here: "%1: %2". gregormi: I will do it like this:
i18nc("one line in the information that goes to the clipboard", "%1… | |||||
| 264 | // "1_", "2_" etc. at the beginning of the key is for the ordering | ||||
I'd use the C++11 for ( : ) here, and add qAsConst to the second argument since we can depend on Qt 5.7. rkflx: I'd use the C++11 `for ( : )` here, and add `qAsConst` to the second argument since we can… | |||||
| 265 | // in the map and will not be part of the text for the clipboard | ||||
| 266 | text += i.key().mid(2) + ": " + i.value(); | ||||
| 267 | text += "\n"; | ||||
| 268 | } | ||||
| 269 | clipboard->setText(text); | ||||
Is endsWith RTL aware? If not, it might be better to also check startsWith, however that assumes in most languages : is kept intact, which I'm not sure about. Thinking about this again, why not
rkflx: Is `endsWith` RTL aware? If not, it might be better to also check `startsWith`, however that… | |||||
| 242 | } | 270 | } | ||
| 243 | 271 | | |||
| 244 | QString Module::plasmaVersion() const | 272 | QString Module::plasmaVersion() const | ||
I suspect @ltoscano meant something like this: %1 is a label, %2 is a corresponding value "One line" is meaningless unless the translator also knows the other lines, and whether it is saved to the clipboard or a text file is probably also more confusing than helping. rkflx: > explain what the placeholders are meant to be. Leave it to the speakers of the language the… | |||||
| 245 | { | 273 | { | ||
| 246 | const QStringList &filePaths = QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, | 274 | const QStringList &filePaths = QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, | ||
| 247 | QStringLiteral("xsessions/plasma.desktop")); | 275 | QStringLiteral("xsessions/plasma.desktop")); | ||
| 248 | 276 | | |||
| 249 | if (filePaths.length() < 1) { | 277 | if (filePaths.length() < 1) { | ||
| 250 | return QString(); | 278 | return QString(); | ||
| 251 | } | 279 | } | ||
| 252 | 280 | | |||
| 253 | // Despite the fact that there can be multiple desktop files we simply take | 281 | // Despite the fact that there can be multiple desktop files we simply take | ||
| 254 | // the first one as users usually don't have xsessions/ in their $HOME | 282 | // the first one as users usually don't have xsessions/ in their $HOME | ||
| 255 | // data location, so the first match should (usually) be the only one and | 283 | // data location, so the first match should (usually) be the only one and | ||
| 256 | // reflect the plasma session run. | 284 | // reflect the plasma session run. | ||
| 257 | KDesktopFile desktopFile(filePaths.first()); | 285 | KDesktopFile desktopFile(filePaths.first()); | ||
| 258 | return desktopFile.desktopGroup().readEntry("X-KDE-PluginInfo-Version", QString()); | 286 | return desktopFile.desktopGroup().readEntry("X-KDE-PluginInfo-Version", QString()); | ||
| 259 | } | 287 | } | ||
Is this necessary for this patch? If not, let's address it in a different patch or commit.