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> | ||||
| 24 | #include <QIcon> | 25 | #include <QIcon> | ||
| 25 | #include <QStandardPaths> | 26 | #include <QStandardPaths> | ||
| 26 | 27 | | |||
| 27 | #include <KAboutData> | 28 | #include <KAboutData> | ||
| 28 | #include <KCoreAddons> | 29 | #include <KCoreAddons> | ||
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 | |||||
| 29 | #include <KConfig> | 30 | #include <KConfig> | ||
| 30 | #include <KConfigGroup> | 31 | #include <KConfigGroup> | ||
| 31 | #include <KDesktopFile> | 32 | #include <KDesktopFile> | ||
| 32 | #include <KFormat> | 33 | #include <KFormat> | ||
| 33 | #include <KLocalizedString> | 34 | #include <KLocalizedString> | ||
| 34 | #include <KSharedConfig> | 35 | #include <KSharedConfig> | ||
| 35 | 36 | | |||
| 36 | #include <solid/device.h> | 37 | #include <solid/device.h> | ||
| ▲ Show 20 Lines • Show All 74 Lines • ▼ Show 20 Line(s) | |||||
| 111 | 112 | | |||
| 112 | Module::~Module() | 113 | Module::~Module() | ||
| 113 | { | 114 | { | ||
| 114 | delete ui; | 115 | delete ui; | ||
| 115 | } | 116 | } | ||
| 116 | 117 | | |||
| 117 | void Module::load() | 118 | void Module::load() | ||
| 118 | { | 119 | { | ||
| 120 | labelsForClipboard.clear(); | ||||
| 119 | loadSoftware(); | 121 | loadSoftware(); | ||
| 120 | loadHardware(); | 122 | loadHardware(); | ||
| 123 | | ||||
| 124 | connect(ui->pushButtonCopyInfo, &QPushButton::clicked, this, &Module::copyToClipboard); | ||||
| 121 | } | 125 | } | ||
| 122 | 126 | | |||
| 123 | void Module::save() | 127 | void Module::save() | ||
| 124 | { | 128 | { | ||
| 125 | } | 129 | } | ||
| 126 | 130 | | |||
| 127 | void Module::defaults() | 131 | void Module::defaults() | ||
| 128 | { | 132 | { | ||
| Show All 19 Lines | 136 | { | |||
| 148 | OSRelease os; | 152 | OSRelease os; | ||
| 149 | // We allow overriding of the OS name for branding purposes. | 153 | // We allow overriding of the OS name for branding purposes. | ||
| 150 | // For example OS Ubuntu may be rebranded as Kubuntu. Also Kubuntu Active | 154 | // For example OS Ubuntu may be rebranded as Kubuntu. Also Kubuntu Active | ||
| 151 | // as a product brand is different from Kubuntu. | 155 | // as a product brand is different from Kubuntu. | ||
| 152 | const QString distroName = cg.readEntry("Name", os.name); | 156 | const QString distroName = cg.readEntry("Name", os.name); | ||
| 153 | const QString versionId = cg.readEntry("Version", os.versionId); | 157 | const QString versionId = cg.readEntry("Version", os.versionId); | ||
| 154 | ui->nameVersionLabel->setText(QStringLiteral("%1 %2").arg(distroName, versionId)); | 158 | ui->nameVersionLabel->setText(QStringLiteral("%1 %2").arg(distroName, versionId)); | ||
| 155 | 159 | | |||
| 156 | const QString variant = cg.readEntry("Variant", QString()); | 160 | 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… | |||||
| 157 | if (variant.isEmpty()) { | 161 | if (variant.isEmpty()) { | ||
| 158 | ui->variantLabel->hide(); | 162 | ui->variantLabel->hide(); | ||
| 159 | } else { | 163 | } else { | ||
| 160 | ui->variantLabel->setText(variant); | 164 | ui->variantLabel->setText(variant); | ||
| 161 | } | 165 | } | ||
| 162 | 166 | | |||
| 163 | const QString url = cg.readEntry("Website", os.homeUrl); | 167 | const QString url = cg.readEntry("Website", os.homeUrl); | ||
| 164 | if (url.isEmpty()) { | 168 | if (url.isEmpty()) { | ||
| 165 | ui->urlLabel->hide(); | 169 | ui->urlLabel->hide(); | ||
| 166 | } else { | 170 | } else { | ||
| 167 | ui->urlLabel->setText(QStringLiteral("<a href='%1'>%1</a>").arg(url)); | 171 | ui->urlLabel->setText(QStringLiteral("<a href='%1'>%1</a>").arg(url)); | ||
| 168 | } | 172 | } | ||
| 169 | 173 | | |||
| 170 | // Since Plasma version detection isn't based on a library query it can fail | 174 | // Since Plasma version detection isn't based on a library query it can fail | ||
| 171 | // in weird cases; instead of admiting defeat we simply hide everything :P | 175 | // in weird cases; instead of admiting defeat we simply hide everything :P | ||
| 172 | const QString plasma = plasmaVersion(); | 176 | const QString plasma = plasmaVersion(); | ||
| 173 | if (plasma.isEmpty()) { | 177 | if (plasma.isEmpty()) { | ||
| 174 | ui->plasma->hide(); | 178 | ui->plasma->hide(); | ||
| 175 | ui->plasmaLabel->hide(); | 179 | ui->plasmaLabel->hide(); | ||
| 176 | } else { | 180 | } else { | ||
| 177 | ui->plasmaLabel->setText(plasma); | 181 | ui->plasmaLabel->setText(plasma); | ||
| 182 | labelsForClipboard << qMakePair(ui->plasma, ui->plasmaLabel); | ||||
| 178 | } | 183 | } | ||
| 179 | 184 | | |||
| 180 | ui->qtLabel->setText(qVersion()); | 185 | const QString qversion = qVersion(); | ||
| 186 | ui->qtLabel->setText(qversion); | ||||
| 187 | labelsForClipboard << qMakePair(ui->qt, ui->qtLabel); | ||||
| 181 | 188 | | |||
| 182 | ui->frameworksLabel->setText(KCoreAddons::versionString()); | 189 | const QString frameworksVersion = KCoreAddons::versionString(); | ||
| 190 | ui->frameworksLabel->setText(frameworksVersion); | ||||
| 191 | labelsForClipboard << qMakePair(ui->frameworksLabelKey, ui->frameworksLabel); | ||||
| 183 | } | 192 | } | ||
| 184 | 193 | | |||
| 185 | void Module::loadHardware() | 194 | void Module::loadHardware() | ||
| 186 | { | 195 | { | ||
| 187 | struct utsname utsName; | 196 | struct utsname utsName; | ||
| 188 | if(uname(&utsName) != 0) { | 197 | if(uname(&utsName) != 0) { | ||
| 189 | ui->kernel->hide(); | 198 | ui->kernel->hide(); | ||
| 190 | ui->kernelLabel->hide(); | 199 | ui->kernelLabel->hide(); | ||
| 191 | } else { | 200 | } else { | ||
| 192 | ui->kernelLabel->setText(utsName.release); | 201 | ui->kernelLabel->setText(utsName.release); | ||
| 202 | labelsForClipboard << qMakePair(ui->kernel, ui->frameworksLabelKey); | ||||
| 193 | } | 203 | } | ||
| 194 | 204 | | |||
| 195 | const int bits = QT_POINTER_SIZE == 8 ? 64 : 32; | 205 | const int bits = QT_POINTER_SIZE == 8 ? 64 : 32; | ||
| 206 | const QString bitsStr = QString::number(bits); | ||||
| 196 | ui->bitsLabel->setText(i18nc("@label %1 is the CPU bit width (e.g. 32 or 64)", | 207 | ui->bitsLabel->setText(i18nc("@label %1 is the CPU bit width (e.g. 32 or 64)", | ||
| 197 | "%1-bit", QString::number(bits))); | 208 | "%1-bit", bitsStr)); | ||
| 209 | labelsForClipboard << qMakePair(ui->bitsKey, ui->bitsLabel); | ||||
| 198 | 210 | | |||
| 199 | const QList<Solid::Device> list = Solid::Device::listFromType(Solid::DeviceInterface::Processor); | 211 | const QList<Solid::Device> list = Solid::Device::listFromType(Solid::DeviceInterface::Processor); | ||
| 200 | ui->processor->setText(i18np("Processor:", "Processors:", list.count())); | 212 | ui->processor->setText(i18np("Processor:", "Processors:", list.count())); | ||
| 201 | // Format processor string | 213 | // Format processor string | ||
| 202 | // Group by processor name | 214 | // Group by processor name | ||
| 203 | QMap<QString, int> processorMap; | 215 | QMap<QString, int> processorMap; | ||
| 204 | Q_FOREACH(const Solid::Device &device, list) { | 216 | Q_FOREACH(const Solid::Device &device, list) { | ||
| 205 | const QString name = device.product(); | 217 | const QString name = device.product(); | ||
| Show All 10 Lines | |||||
| 216 | for (auto it = processorMap.constBegin(); it != processorMap.constEnd(); ++it) { | 228 | for (auto it = processorMap.constBegin(); it != processorMap.constEnd(); ++it) { | ||
| 217 | const int count = it.value(); | 229 | const int count = it.value(); | ||
| 218 | QString name = it.key(); | 230 | QString name = it.key(); | ||
| 219 | name.replace(QStringLiteral("(TM)"), QChar(8482)); | 231 | name.replace(QStringLiteral("(TM)"), QChar(8482)); | ||
| 220 | name.replace(QStringLiteral("(R)"), QChar(174)); | 232 | name.replace(QStringLiteral("(R)"), QChar(174)); | ||
| 221 | name = name.simplified(); | 233 | name = name.simplified(); | ||
| 222 | names.append(QStringLiteral("%1 × %2").arg(count).arg(name)); | 234 | names.append(QStringLiteral("%1 × %2").arg(count).arg(name)); | ||
| 223 | } | 235 | } | ||
| 224 | ui->processorLabel->setText(names.join(QStringLiteral(", "))); | 236 | | ||
| 237 | const QString processorLabel = names.join(QStringLiteral(", ")); | ||||
| 238 | ui->processorLabel->setText(processorLabel); | ||||
| 225 | if (ui->processorLabel->text().isEmpty()) { | 239 | if (ui->processorLabel->text().isEmpty()) { | ||
| 226 | ui->processor->setHidden(true); | 240 | ui->processor->setHidden(true); | ||
| 227 | ui->processorLabel->setHidden(true); | 241 | ui->processorLabel->setHidden(true); | ||
| 242 | } else { | ||||
| 243 | labelsForClipboard << qMakePair(ui->processor, ui->processorLabel); | ||||
| 228 | } | 244 | } | ||
| 229 | 245 | | |||
| 230 | const qlonglong totalRam = calculateTotalRam(); | 246 | const qlonglong totalRam = calculateTotalRam(); | ||
| 231 | ui->memoryLabel->setText(totalRam > 0 | 247 | const QString memoryLabel = totalRam > 0 | ||
| 232 | ? i18nc("@label %1 is the formatted amount of system memory (e.g. 7,7 GiB)", | 248 | ? i18nc("@label %1 is the formatted amount of system memory (e.g. 7,7 GiB)", | ||
| 233 | "%1 of RAM", KFormat().formatByteSize(totalRam)) | 249 | "%1 of RAM", KFormat().formatByteSize(totalRam)) | ||
| 234 | : i18nc("Unknown amount of RAM", "Unknown")); | 250 | : i18nc("Unknown amount of RAM", "Unknown"); | ||
| 251 | ui->memoryLabel->setText(memoryLabel); | ||||
| 252 | labelsForClipboard << qMakePair(ui->memory, ui->memoryLabel); | ||||
| 253 | } | ||||
| 254 | | ||||
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… | |||||
| 255 | void Module::copyToClipboard() | ||||
| 256 | { | ||||
| 257 | QString text; | ||||
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. | |||||
| 258 | if (!ui->nameVersionLabel->isHidden()) { | ||||
ngraham: Is "Distro" not a translated string? | |||||
| 259 | text += i18nc("one line in the information that goes to the clipboard", "%1 %2", i18nc("label in the Copy to Clipboard button", "Distro:"), ui->nameVersionLabel->text()) + QStringLiteral("\n"); | ||||
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. | |||||
| 260 | } | ||||
| 261 | | ||||
@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… | |||||
| 262 | typedef QPair<QLabel*, QLabel*> QLabelPair; // because the "," cannot be used in the foreach macro | ||||
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… | |||||
| 263 | foreach (QLabelPair p, labelsForClipboard) { // note that does not necessarily represent the same order as in the GUI | ||||
| 264 | auto labelLabel = p.first; | ||||
| 265 | auto versionLabel = p.second; | ||||
| 266 | if (!versionLabel->isHidden()) { | ||||
| 267 | text += i18nc("one line in the information that goes to the clipboard", "%1 %2", labelLabel->text(), versionLabel->text()) + QStringLiteral("\n"); | ||||
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… | |||||
| 268 | } | ||||
| 269 | } | ||||
| 270 | | ||||
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… | |||||
| 271 | QGuiApplication::clipboard()->setText(text); | ||||
| 235 | } | 272 | } | ||
| 236 | 273 | | |||
| 237 | QString Module::plasmaVersion() const | 274 | QString Module::plasmaVersion() const | ||
| 238 | { | 275 | { | ||
| 239 | const QStringList &filePaths = QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, | 276 | const QStringList &filePaths = QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, | ||
| 240 | QStringLiteral("xsessions/plasma.desktop")); | 277 | QStringLiteral("xsessions/plasma.desktop")); | ||
| 241 | 278 | | |||
| 242 | if (filePaths.length() < 1) { | 279 | if (filePaths.length() < 1) { | ||
| Show All 10 Lines | |||||
Is this necessary for this patch? If not, let's address it in a different patch or commit.