Page MenuHomePhabricator

[clang-format] PR48539 ReflowComments breaks Qt translation comments
ClosedPublic

Authored by MyDeveloperDay on Dec 17 2020, 1:20 PM.

Details

Summary

https://bugs.llvm.org/show_bug.cgi?id=48539

Add support for Qt Translator Comments to reflow

When reflown and a part of the comments are added on a new line, it should repeat these extra characters as part of the comment token.

Diff Detail

Event Timeline

MyDeveloperDay requested review of this revision.Dec 17 2020, 1:20 PM
MyDeveloperDay created this revision.
curdeius accepted this revision.Dec 17 2020, 11:04 PM

LGTM! Thanks for fixing this.

This revision is now accepted and ready to land.Dec 17 2020, 11:04 PM
HazardyKnusperkeks requested changes to this revision.Dec 18 2020, 6:15 AM

I don't know if this really fixes the problem.

From the Docs:

An alternative way to attach meta-data is to use the following syntax:

//~ <field name> <field contents>

With the example

//~ loc-layout_id foo_dialog
//~ loc-blank False
//~ magic-stuff This might mean something magic.

So I would say breaking the ~ comment is a no go, but I would have to test it. (Which I can do next week.)
//: should be fine, //= would have the same problem, with the exception that it seems not to be used normally with long comments.

This revision now requires changes to proceed.Dec 18 2020, 6:15 AM

Maybe @JVApen can help as they logged https://bugs.llvm.org/show_bug.cgi?id=48539

I'm not a Qt developer so I can't really comment beyond fixing what was in the bug rpeort

JVApen added a comment.EditedDec 21 2020, 7:48 AM

Maybe @JVApen can help as they logged https://bugs.llvm.org/show_bug.cgi?id=48539

I'm not a Qt developer so I can't really comment beyond fixing what was in the bug rpeort

We actually only use //: in our code, I found the others when looking at the docs. Maybe it's better to skip //~ and add a to-do with the open question if we even can correctly reflow this?

So, I've tested a bit:

//= Die ID
//~ foo bar
//~ EinWort Ein langer Text der fortgesetzt wird
//: Dies ist ein langer Kommentar
//: der umgebrochen wird
tr("Foo");

Results in

<message id="Die ID">
    <location filename="wid.cpp" line="9"/>
    <source>Foo</source>
    <extracomment>Dies ist ein langer Kommentar der umgebrochen wird</extracomment>
    <translation type="unfinished"></translation>
    <extra-EinWort>Ein langer Text der fortgesetzt wird</extra-EinWort>
    <extra-foo>bar</extra-foo>
</message>

And breaking //= or //~ as followed

//= Die
//= ID
//~ foo bar
//~ EinWort Ein langer Text der
//~ fortgesetzt wird
//: Dies ist ein langer Kommentar
//: der umgebrochen wird
tr("Foo");

changes the result

<message id="ID">
    <location filename="wid.cpp" line="11"/>
    <source>Foo</source>
    <extracomment>Dies ist ein langer Kommentar der umgebrochen wird</extracomment>
    <translation type="unfinished"></translation>
    <extra-EinWort>Ein langer Text der</extra-EinWort>
    <extra-foo>bar</extra-foo>
    <extra-fortgesetzt>wird</extra-fortgesetzt>
</message>

So my conclusion is //: can and should be wrapped like in this patch, but //= and //~ should not be touched anyhow, this can be accomplished with the CommentPragmas Setting.

MyDeveloperDay updated this revision to Diff 313297.EditedDec 22 2020, 4:44 AM

remove = and ~ cases

remove = and ~ cases

Wouldn't it make sense to explicitly document that //= and //~ are not put in here as reflowing them results in wrong results?

This looks good to me.

remove = and ~ cases

Wouldn't it make sense to explicitly document that //= and //~ are not put in here as reflowing them results in wrong results?

I think this would fit in a separate change which would also modify the documentation of CommentPragmas, so that who ever needs it could set that accordingly.

I'm currently trying to fix a misformating with AlignConsecutiveDeclarations with Qts emit, maybe there should be at Qt-Mode or something similar? As far as I can see there are some code paths to accommodate Qt.

This revision is now accepted and ready to land.Dec 22 2020, 2:09 PM