When a single image has been selected, the window title now says, "Move/Copy/Link <filename>
When multiple image have been selected, the window title now says, "Move/Copy/Link <number of images selected> images"
rkflx | |
broulik | |
ilic |
KDE Applications |
When a single image has been selected, the window title now says, "Move/Copy/Link <filename>
When multiple image have been selected, the window title now says, "Move/Copy/Link <number of images selected> images"
Tested in KDE Neon. Works fine.
Single image selection:
Multiple image selection:
No Linters Available |
No Unit Test Coverage |
Avoid using word puzzles, better use separate strings for everything, ie. i18n("Move %1") and i18np("%1 image", "%1 images", count)
app/fileoperations.cpp | ||
---|---|---|
60 | You can use constFirst |
app/fileoperations.cpp | ||
---|---|---|
59 | I don't think it's necessary to cache that in a variable | |
65 | i18np - p is for plural, you can't assume every language has just two numeri | |
95 | That's a word puzzle :) you cannot assume the operator comes first in every language (e.g. German would be "Foo verschieben" ) better use an individual string for each case even if it means having 6 different i18n statements. |
app/fileoperations.cpp | ||
---|---|---|
63 | Surely this should be a function call and not an assignment? If this compiled for you (it does not for me), I would be amazed. |
app/fileoperations.cpp | ||
---|---|---|
63 | Surely indeed! Whoops. I suspect doing repeated incremental builds cached something such that I could compile. I'll fix this, do a clean build, and test again. |
Compiles for me now, but please wait for @broulik to approve the i18n parts as well.
As for the "BUG:", I do not believe the reporter is talking about the window title but about the "file name field" itself. I tried reproducing the original issue by reverting 7be60997b7478, but even then "<copyMoveOrLink>" does not appear anywhere and the filename is still not empty. This would suggest this might be a different issue altogether, e.g. a Frameworks fix the reporter lacks or weird local patch? @ngraham Please recheck and possibly remove the bug here and change the bugzilla (WORKSFORME or NEEDSINFO?).
app/fileoperations.cpp | ||
---|---|---|
62–66 | Why do you need the if? If I understood the API docs correctly, a single call to i18ncp should be enough, shouldn't it? | |
65 | For the singular, dropping image was a good idea. Also do this here please. |
app/fileoperations.cpp | ||
---|---|---|
62–66 | Oh so much simpler. It's amazing when things work as they're supposed to! |
app/fileoperations.cpp | ||
---|---|---|
62–66 | Actually I don't think I can use il8ncp for both the singular and the plural. I'd need to do something like this: i18ncp("@title:window", "Copy %1", "Copy %2 images", urlList.constFirst().fileName()), numberOfImages, ...Bit then it complains about an invalid number of arguments in the format string, which I suppose isn't totally inappropriate since there are two variables but each string only consumes one of them. Can you think of a way around this? |
app/fileoperations.cpp | ||
---|---|---|
62–66 | I mean, it actually does work, but the pluralized string gets "I18N_EXCESS_ARGUMENTS_SUPPLIED" appended to the end. https://cgit.kde.org/ki18n.git/tree/src/klocalizedstring.cpp#n775 I'm tempted to report this as a bug, since in this particular use case it does actually work fine other than the error string that gets automatically appended. |
app/fileoperations.cpp | ||
---|---|---|
62–66 | Reported as https://bugs.kde.org/show_bug.cgi?id=386053. So for the time being I think I do need to keep the manual check for one item, or else I'll have to sacrifice showing the filename when only one item is selected. Thoughts? |
app/fileoperations.cpp | ||
---|---|---|
62–66 | Are you sure? This is working fine for me: dialog.setWindowTitle(i18ncp("@title:window", "Foo - Copy %1", "Bar - Copy %2 images", urlList.constFirst().fileName(), numberOfImages)); Did you make install? Also, at least on Phab, your code snippet had wrong parentheses… Could you check in a different environment/user/distro? |
Ok, can reproduce. The trick is to compile R249 KI18n in Debug mode 🙈. Then I get:
@ilic Any tips what to do here? Basically our singular and plural forms need to include different placeholders. Is there a workaround, should we ignore the debug message, should we fix i18np? Otherwise, we'll just keep the if.
i18np should be used for messages which have the same wording other than for parts that depend on which particular number occurs. So the visual warning in debug mode is correct, and the solution with if is correct (but see also line comments).
app/fileoperations.cpp | ||
---|---|---|
71 | Only the context and singular string are used as message key, so both messages would appear as single message to translator. Context should be used to disambiguate this, e.g. to this message "@title:window %1 file name". | |
79 | Same here. |
@ngraham You probably should have ignored my old comments and just followed the tips of the expert :)
If I understood correctly, it should be like this, i.e. bring back "image" in singular and explain the parameters (reason being the translators probably only see the context and the string to translate, but not all arguments of the function call):
i18nc("@title:window %1 file name", "Copy %1", urlList.constFirst().fileName()) i18ncp("@title:window %1 number of images", "Copy %1 image", "Copy %1 images", numberOfImages)
LGTM now. Please wait a day or two, in case @broulik has more comments.
While this does not solve the bug, it's still nice to have… Thanks!
I investigated the bug today and I think the reporter is running Gwenview in a GTK DE, and it's the GTK dialog which is being shimmed in that isn't working properly. The problem is either in Qt or GTK, but either way, it's not Gwenview's fault.
Gwenview in a GTK DE
Let's continue this in the bug report. Need to do one more test, then expect an comment from me :)