Improvements for Gettext entries wordwrapping
Needs ReviewPublic

Authored by azamathackimov on Jan 21 2018, 5:58 PM.

Details

Summary

Improves word wrapping code for Gettext catalog.
https://bugs.kde.org/show_bug.cgi?id=380007

Diff Detail

Repository
R456 Lokalize
Lint
Lint Skipped
Unit
Unit Tests Skipped
azamathackimov requested review of this revision.Jan 21 2018, 5:58 PM
azamathackimov created this revision.
lueck added a subscriber: lueck.

adding Lokalize maintainer

meskobalazs accepted this revision.Apr 18 2018, 12:01 PM
meskobalazs added a subscriber: meskobalazs.

I have tested this patch when it was submitted, it solves the annoying word wrapping problem. Could we merge this?

This revision is now accepted and ready to land.Apr 18 2018, 12:01 PM
aacid requested changes to this revision.May 20 2018, 9:45 PM
aacid added a reviewer: KDE Applications.
aacid added a subscriber: aacid.

I'm going to add a Needs changes here to remind myself to review this, given that meskobalazs is not a kde/lokalize developer i don't think it was a good idea of him marking this as accepted.

This revision now requires changes to proceed.May 20 2018, 9:45 PM

I'm going to add a Needs changes here to remind myself to review this, given that meskobalazs is not a kde/lokalize developer i don't think it was a good idea of him marking this as accepted.

You are right, of course. I didn't mark it on a whim though, I did give it three months for someone to accept it, but there was no reaction until now. By the way, I authored a very similar patch before I found that somebody already did this.
Anyway, thanks for reviewing it. Finally fixing this issue will enable our localization community to use Lokalize again.

aacid added a comment.Jun 19 2018, 9:46 PM

This is a change that would greatly benefit from an auto test, sadly we don't have to have any in lokalize.

Can you please explain the code changes?

Can you please explain the code changes?

I can't give you a line-by-line explanation, as I am not the author. However, it restores the old way as Lokalize exported the PO files - which matches other PO-based tools. Namely, in multiline msgstr strings, the spaces are put at the end of the lines, instead of the beginning of the next one.

Simon what do you think about this?

sdepiets updated this revision to Diff 39238.Aug 7 2018, 5:12 AM

Fixing patch

Simon what do you think about this?

I've just "rebased" the patch because I fixed an issue with the wordwrap already in the same file.
I need more context to understand what exactly is the issue with the current behavior to begin with. Do you have specific examples ?

With this patch this is the behavior that I see (it removes all line breaks in source and target), which seems rather unwanted in that situation.

-msgid ""
-"<center>This investment account does not contain the \"%1\" security.</center>"
-"<center>Transactions involving this security will be ignored.</center>"
-msgstr ""
-"<center>Ce compte de placement ne contient pas le titre « %1 ».</center>"
-"<center>Les opérations impliquant ce titre seront ignorées.</center>"
+msgid "<center>This investment account does not contain the \"%1\" security.</center><center>Transactions involving this security will be ignored.</center>"
+msgstr "<center>Ce compte de placement ne contient pas le titre « %1 ».</center> <center>Les opérations impliquant ce titre seront ignorées.</center>"
sdepiets requested changes to this revision.Aug 7 2018, 5:20 AM
This revision now requires changes to proceed.Aug 7 2018, 5:20 AM

It has been more than a year. Should we close this and https://bugs.kde.org/show_bug.cgi?id=380007?

We never got a proper explanation of what this does or how it works.

The person that submitted it never answered any question.

I'd say abandon this.

May be the original author is not following Phabricator. You can try and ping him in the bugs.kde.org report or by email.

Hello, it's me again.

Previous work was not ideal, so I rewrote whole thing.

aacid added inline comments.Dec 11 2019, 9:52 PM
src/catalog/gettext/gettextexport.h
77

wouldn't it make more sense if the signature was
static QStringList tokenize(const QString &text, const QRegExp &delimiters);
?

azamathackimov marked an inline comment as done.Dec 14 2019, 6:12 PM
azamathackimov added inline comments.
src/catalog/gettext/gettextexport.h
77

Well, I thought that will be non-optimal. Refactored to

static QStringList tokenize(const QString &text, const QRegExp &delimiters);
azamathackimov marked 2 inline comments as done.Dec 17 2019, 8:52 PM