Special case added for handling tooltip text when in summary page
Gets rid of the confusing "Files: 2 (100%)" and "Click to go up to parent directory"
Fixes the partition name and "Used"/"New" being concatenated without spacing in between
Details
Details
- Reviewers
sandsmark sitter - Commits
- R352:ff4ecabf7632: Display more readable tooltip text
Diff Detail
Diff Detail
- Repository
- R352 Filelight
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| src/radialMap/widgetEvents.cpp | ||
|---|---|---|
| 156 | should be on the previous line | |
| 158 | I would, for all strings, find them easier to read if they were QString("%1\n%2: %3).arg(foo, bar, kitten)instead of concatenations. It would also be easier to spot spacing problems when written that way. @sandsmark might disagree though ^^ | |
| 168 | should be on the next line | |
| 170 | should be on the previous line | |
Comment Actions
please fix what harald pointed out, otherwise it looks good.
| src/radialMap/widgetEvents.cpp | ||
|---|---|---|
| 158 | doesn't matter much to me, I don't really disagree in this case. might make sense to make it translatable, I'm not sure if the way this is structured works in all languages. | |
Comment Actions
I'm not sure if there is any need to localize these strings, so I'm using QString(QLatin1String("...")). if you think there is a need, I can replace them with i18n().
Comment Actions
yeah, I prefer to use i18n() wherever possible, different languages have very different ways of structuring text and information. (and use i18nc() to explain to the translators the context)
Comment Actions
Well, that opened a can of worms 😒
| src/radialMap/widgetEvents.cpp | ||
|---|---|---|
| 157 | I am pretty sure the context needs to explain what %1,2,3 are. It's not obvious from the string itself what the placeholders are. This applies to pretty much all the i18n strings in the diff. | |
| 175–177 | This isn't new code I think, but this needs to be something like if (percent > 0) {
string += i18ncp("%1 is an amount of files", "File: %1", "Files: %1", files);
} else {
string += i18ncp("%1 is an amount of files", "File: %1 (%2%)", "Files: %1 (%2%)", files, percent);
}in a right-to-left language the percent may well need to go on the left of the string, not the right. I am also not sure a plural is called for here. Files: 1 seems perfectly acceptable to me. File: 1 on the other hand looks somewhat odd. | |
Comment Actions
Localization is hard :(
I put the used/free tooltips into separate cases, hopefully that will make the context for localization clearer.
I agree, 1 File looks better than Files: 1.
Comment Actions
Getting there :)
| src/radialMap/widgetEvents.cpp | ||
|---|---|---|
| 190 | You could probably move the \n out of the i18n call, and prepend it always. It doesn't need localization and it being not mid-string seems like something that can easily get lost when translating. | |
| src/summaryWidget.cpp | ||
| 148 | Doesn't this undo some of the i18n in the original code? | |
| src/radialMap/widgetEvents.cpp | ||
|---|---|---|
| 190 | I agree, but wouldn't that mean the translators would have to re-translate this string? I wonder if it's worth it | |
| src/summaryWidget.cpp | ||
| 148 | I moved the i18n to this part: if (strcmp("used", m_focus->file()->name8Bit()) == 0) {
string = i18nc("Tooltip of used space on the partition, %1 is path, %2 is size", "%1\nUsed: %2",..
} else if (strcmp("free", m_focus->file()->name8Bit()) == 0) {
string = i18nc("Tooltip of free space on the partition, %1 is path, %2 is size", "%1\nFree: %2",..My logic is the context will be less confusing to translators if they see used or free instead of another variable "%2". | |
Comment Actions
Looks good to land unless @sandsmark sees more issues.
| src/radialMap/widgetEvents.cpp | ||
|---|---|---|
| 190 | Oh, it's a pre-existing string. Yeah, they'd have to re-translate. Nevermind changing this then. | |
| src/summaryWidget.cpp | ||
| 148 | Ah, perfect. | |