Update move/copy/link dialog window's title to reflect selected files
ClosedPublic

Authored by ngraham on Oct 21 2017, 7:39 PM.

Details

Summary

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"

Test Plan

Tested in KDE Neon. Works fine.

Single image selection:

Multiple image selection:

Diff Detail

Repository
R260 Gwenview
Branch
373629
Lint
No Linters Available
Unit
No Unit Test Coverage
ngraham created this revision.Oct 21 2017, 7:39 PM
ngraham edited the summary of this revision. (Show Details)Oct 21 2017, 7:41 PM
ngraham edited the test plan for this revision. (Show Details)

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

ngraham updated this revision to Diff 21085.Oct 21 2017, 7:52 PM

Cleanup and @broulik's suggestions

ngraham marked an inline comment as done.Oct 21 2017, 7:53 PM
ngraham updated this revision to Diff 21086.Oct 21 2017, 7:55 PM

numberOfFiles should be const

broulik added inline comments.Oct 21 2017, 7:56 PM
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.

ngraham updated this revision to Diff 21093.Oct 21 2017, 8:57 PM

Eventually I will figure this stuff out :)

ngraham marked 3 inline comments as done.Oct 21 2017, 8:57 PM
ngraham updated this revision to Diff 21095.Oct 21 2017, 9:16 PM

Think I figured out the i18n part, and got rid of all the word puzzles. :)

ngraham updated this revision to Diff 21096.Oct 21 2017, 9:16 PM

"You missed a spot"

rkflx requested changes to this revision.Oct 21 2017, 9:52 PM
rkflx added inline comments.
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.

This revision now requires changes to proceed.Oct 21 2017, 9:52 PM
ngraham added inline comments.Oct 21 2017, 9:55 PM
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.

ngraham updated this revision to Diff 21098.Oct 21 2017, 10:02 PM

A few corrections

ngraham marked 2 inline comments as done.Oct 21 2017, 10:02 PM

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

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.

ngraham added inline comments.Oct 21 2017, 11:31 PM
app/fileoperations.cpp
62

Oh so much simpler. It's amazing when things work as they're supposed to!

ngraham updated this revision to Diff 21099.Oct 21 2017, 11:49 PM

Fix singular format strings

ngraham marked an inline comment as done.Oct 21 2017, 11:49 PM
ngraham added inline comments.
app/fileoperations.cpp
62

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?

ngraham added inline comments.Oct 21 2017, 11:57 PM
app/fileoperations.cpp
62

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.

ngraham added inline comments.Oct 22 2017, 2:03 AM
app/fileoperations.cpp
62

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?

rkflx added inline comments.Oct 22 2017, 6:23 AM
app/fileoperations.cpp
62

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?

rkflx added a subscriber: ilic.Oct 22 2017, 8:32 AM

Ok, can reproduce. The trick is to compile R249 KI18n in Debug mode 🙈. Then I get:

  • (I18N_GAPS_IN_PLACEHOLDER_SEQUENCE) appended to the window title
  • Placeholder %1 skipped in message {Bar - Copy %2 images}. on the console

@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.

ilic added a comment.Oct 22 2017, 8:52 AM

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 edited the summary of this revision. (Show Details)Oct 22 2017, 8:08 PM
ngraham retitled this revision from Update move/copy/link dialog's window title appropriately to Update move/copy/link dialog window's title to reflect selected files.Oct 22 2017, 8:14 PM
ngraham updated this revision to Diff 21137.Oct 22 2017, 8:15 PM

i18n changes

ngraham marked 8 inline comments as done.Oct 22 2017, 8:15 PM
rkflx added a comment.Oct 22 2017, 8:39 PM

@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)
ngraham updated this revision to Diff 21140.Oct 22 2017, 8:54 PM

Maybe eventually I will get this right :)

rkflx accepted this revision.Oct 22 2017, 9:08 PM

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!

This revision is now accepted and ready to land.Oct 22 2017, 9:08 PM

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.

rkflx added a comment.EditedOct 22 2017, 9:18 PM

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 :)

Yeah, I already passed the bug back to the reporter with a request for more info.

ilic accepted this revision.Oct 22 2017, 10:09 PM

@broulik, can I get a thumbs up before I commit this?

I'd say you can commit since @ilic (thanks for helping out, BTW!) gave his okay.

This revision was automatically updated to reflect the committed changes.