Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix, CI] Push to Transifex from master, fix multiline strings for xgettext #5238

Merged
merged 21 commits into from
Aug 21, 2019

Conversation

Frenzie
Copy link
Member

@Frenzie Frenzie commented Aug 21, 2019

Related to #5237

@Frenzie Frenzie added this to the 2019.09 milestone Aug 21, 2019
@Frenzie Frenzie changed the title [CI] Push to Transifex from master WIP [CI] Push to Transifex from master Aug 21, 2019
@Frenzie
Copy link
Member Author

Frenzie commented Aug 21, 2019

It looks like you need a workflow for this context stuff. The docs don't really say so: https://circleci.com/docs/2.0/contexts/#creating-and-using-a-context

But https://discuss.circleci.com/t/contexts-feedback/13908/12

So, annoying. :-)

@Frenzie Frenzie closed this Aug 21, 2019
@Frenzie Frenzie reopened this Aug 21, 2019
@Frenzie
Copy link
Member Author

Frenzie commented Aug 21, 2019

Curious fact: three new strings seem to have shown up:

Screenshot_2019-08-22_00-04-36

They look like they're actually typos:

return false, T(_("Extracting archive failed:\n\n%1", archive))

local info_text = T(_"Type: %1\nName: %2\nAddress: %3", "FTP", item.text, item.address)

local info_text = T(_"Type: %1\nName: %2\nAddress: %3", "WebDAV", item.text, item.address)

info_text = T(_"Type: %1\nName: %2\nEmail: %3\nCounty: %4",
"Dropbox",info.name.display_name, info.email, info.country)

Edit: no scratch that, of course _"string" is a valid function invocation. Only the util one is a typo.

@Frenzie Frenzie changed the title WIP [CI] Push to Transifex from master [fix, CI] Push to Transifex from master, fix multiline strings for xgettext Aug 21, 2019
@Frenzie Frenzie merged commit e2ceace into koreader:master Aug 21, 2019
@Frenzie Frenzie deleted the transifex-push branch August 21, 2019 22:12
@Frenzie Frenzie added the chore label Aug 21, 2019
@poire-z
Copy link
Contributor

poire-z commented Aug 21, 2019

Uh, reverting 335a513 (#4524, where we agree it was preferable), which reverted 9971eb8 (#4132), so we're back there ?

@Frenzie
Copy link
Member Author

Frenzie commented Aug 21, 2019

Unfortunately so. xgettext apparently doesn't do koreader/koreader-misc@d72e711

Browsing through the xgettext test cases they don't actually properly test newlines.

Perhaps it's really easy to patch. x-lua.c says things like this (but that's for multiline comments). I'll check some more tomorrow.

                      /* Ignore leading spaces and tabs.  */
                      if (buflen == 0 && (c == ' ' || c == '\t'))
                        continue;

@Frenzie
Copy link
Member Author

Frenzie commented Aug 22, 2019

@poire-z I wrote this proof of concept patch for xgettext:

diff --git a/gettext-tools/src/x-lua.c b/gettext-tools/src/x-lua.c
index 3a2296f36..1eed3dccc 100644
--- a/gettext-tools/src/x-lua.c
+++ b/gettext-tools/src/x-lua.c
@@ -740,6 +740,18 @@ phase3_get (token_ty *tp)
 
           string_start ();
 
+          /* Ignore the first newline after '[[' because so does Lua. */
+          c = phase1_getc ();
+          if (c == '\\')
+            {
+              c = phase1_getc ();
+              if (c != 'n')
+                {
+                  string_add ('\\');
+                  string_add (c);
+                }
+            }
+
           for (;;)
             {
               c = phase1_getc ();

@poire-z
Copy link
Contributor

poire-z commented Aug 22, 2019

Kudos for following that ugly coding style :)
Haven't looked at the surrounding context, but won't you be consuming the \ (and its next char), if it's not followed by a n ? So a \t will disappear?

@Frenzie
Copy link
Member Author

Frenzie commented Aug 22, 2019

You're right, there's something very wrong there. :-D

@poire-z
Copy link
Contributor

poire-z commented Aug 22, 2019

But re-reading that snippet, you were indeed re-adding them if they were not \n. But may be not when the first one is not a \.
And re-adding them at the start or end or elsewhere?

@Frenzie
Copy link
Member Author

Frenzie commented Aug 22, 2019

I massively overthought the buffer. Apparently this suffices:

diff --git a/gettext-tools/src/x-lua.c b/gettext-tools/src/x-lua.c
index 3a2296f36..262f688cc 100644
--- a/gettext-tools/src/x-lua.c
+++ b/gettext-tools/src/x-lua.c
@@ -740,6 +740,13 @@ phase3_get (token_ty *tp)
 
           string_start ();
 
+          /* Ignore the first newline after '[[' because so does Lua. */
+          c = phase1_getc ();
+          if (c != '\n')
+            {
+              string_add (c);
+            }
+
           for (;;)
             {
               c = phase1_getc ();
_([[


only two newlines]])

_([[	


tab two newlines]])

_([[ 


space two newlines]])

_([[just one line]])
#: reader.lua:16
msgid ""
"\n"
"\n"
"only two newlines"
msgstr ""

#: reader.lua:21
msgid ""
"\t\n"
"\n"
"\n"
"tab two newlines"
msgstr ""

#: reader.lua:26
msgid ""
" \n"
"\n"
"\n"
"space two newlines"
msgstr ""


#: reader.lua:30
msgid "just one line"
msgstr ""

@Frenzie
Copy link
Member Author

Frenzie commented Aug 22, 2019

@poire-z

And re-adding them at the start or end or elsewhere?

Yes, I was missing something like an else condition in the above but I was too focused on my specific "should eat newline" scenario. :-)

@Frenzie
Copy link
Member Author

Frenzie commented Aug 22, 2019

Anyway, I'll look into reporting the bug and hopefully the maintainer can rewrite it properly in less than a minute if necessary.

From the README:

Report bugs

@Frenzie
Copy link
Member Author

Frenzie commented Aug 22, 2019

@poire-z Alright, I noticed another function. :-P

diff --git a/gettext-tools/src/x-lua.c b/gettext-tools/src/x-lua.c
index 3a2296f36..e0dd99709 100644
--- a/gettext-tools/src/x-lua.c
+++ b/gettext-tools/src/x-lua.c
@@ -740,6 +740,13 @@ phase3_get (token_ty *tp)
 
           string_start ();
 
+          /* Ignore the first newline after '[[' because so does Lua. */
+          c = phase1_getc ();
+          if (c != '\n')
+            {
+              phase1_ungetc (c);
+            }
+
           for (;;)
             {
               c = phase1_getc ();

That way the character is actually processed properly by the rest of the code; of course it shouldn't be just blindly added.

@poire-z
Copy link
Contributor

poire-z commented Aug 22, 2019

Isn't there a peekc() ?

@Frenzie
Copy link
Member Author

Frenzie commented Aug 22, 2019

This pattern seems to be more or less how the rest of the code does it, for example:

      else
        {
          /* Minus sign.  */
          phase1_ungetc (c);
          return '-';
        }

Edit: anyhow, submitted

https://savannah.gnu.org/bugs/index.php?56794

Frenzie added a commit to Frenzie/virdevenv that referenced this pull request Aug 22, 2019
See koreader/koreader#5238 (comment)

Even if a fix is implemented upstream soon, it'd be years before we could use a precompiled binary anyway.
Frenzie added a commit to koreader/virdevenv that referenced this pull request Aug 22, 2019
See koreader/koreader#5238 (comment)

Even if a fix is implemented upstream soon, it'd be years before we could use a precompiled binary anyway.
Frenzie added a commit to Frenzie/koreader that referenced this pull request Aug 22, 2019
Because let's face it, it just looks much better this way.

Docker image update in koreader/virdevenv#43

Discussion in koreader#5238 (comment) and koreader#4524
Frenzie added a commit to Frenzie/koreader that referenced this pull request Aug 22, 2019
Because let's face it, it just looks much better this way.

Docker image update in koreader/virdevenv#43

Discussion in koreader#5238 (comment) and koreader#4524
Frenzie added a commit to Frenzie/koreader that referenced this pull request Aug 22, 2019
Because let's face it, it just looks much better this way.

Docker image update in koreader/virdevenv#43

Discussion in koreader#5238 (comment) and koreader#4524
Frenzie added a commit that referenced this pull request Aug 22, 2019
)

Because let's face it, it just looks much better this way.

Docker image update in koreader/virdevenv#43

Discussion in #5238 (comment) and #4524
@Frenzie Frenzie added the i18n label Aug 26, 2019
mwoz123 pushed a commit to mwoz123/koreader that referenced this pull request Mar 29, 2020
mwoz123 pushed a commit to mwoz123/koreader that referenced this pull request Mar 29, 2020
…reader#5242)

Because let's face it, it just looks much better this way.

Docker image update in koreader/virdevenv#43

Discussion in koreader#5238 (comment) and koreader#4524
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants