377513 - Addon types are static and should be handled in a static way to simplify localization

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Localization, enhancement)

Description

[Jesper Kristensen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; da; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: 

The addon types table has three fields: "name", "name_plural" and "description". These are localized in the database. The localization should be moved to gettext for the following reasons:

1: Strings in messages.po are easier to maintain for localizers because you have feeds and svn to track changes.
2: Right now, the same string is used in different places, where it should be localized in different ways. By replacing the single string in the database with multiple strings in messages.po, proper localization would be possible. (See bug 371951)
3: Addon types are static. At the moment only themes and extensions share a lot of code. Dictionaries have their own setup, search engines have their own setup, plugins have their own setup and i don't know about language packs. Conclution: To add a new addon type, you would have do make significant code changes anyway. Therefore having the list of addon types static (hardcoded) would not mean much extra work if you decide to add a new type in the future.
4: Removing addon types from dynamic translations would remove complexity in the admin and localizer sections, as pages for maintaining these strings are no longer needed.

I have done some research in the code on how to do this, and I believe I can produce a working patch in a reasonable timeframe. Please let me know if you think this is a stupid idea or if it is something I should go on with.

Reproducible: Always

Comment 1

[Jesper Kristensen]

Attachment: Patch v1

This is my initial try for a solution. It still needs some work (e.g. .po file needs some comments), but I would like to hear your comments first.

Comment 2

[Jesper Kristensen]

Attachment: Patch v2

Comment 3

[Fred Wenzel [:wenzel]]

Shortly looking at your patch, I'd personally refrain from doing this:

_('general_addontype_'.$addontype['Addontype']['text_id'])

because it will make it impossible to ever use a tool again to extract gettext tags from the source code files.

YMMV, though.

Comment 4

[Jesper Kristensen]

Attachment: Patch v3

I don't know much about gettext tools, but here is a new patch with all msgids written in their full length.

Comment 5

[Jesper Kristensen]

Attachment: Patch v4

Removed some unused code

Comment 6

[Fred Wenzel [:wenzel]]

Alright. The argument that this patch will resolve conjugation issues sounds good to me.

I am just reviewing your patch; it looks quite good. Just one note so far:
- Don't leave the addontypes table in there with id, created and modified fields only. Remove it altogether. (note, this will eventually have to happen in the production database manually of course)

Also, trying to apply the patch against the trunk threw quite a few rejects. You might want to svn up your working copy some time.

Thanks for your work!

Comment 7

[Fred Wenzel [:wenzel]]

Actually, after talking to sancus and shaver, I have to admit removing the table is not such a good idea after all. It's valuable for the FK constraints and should stay. Sorry.

Comment 8

[Jesper Kristensen]

Attachment: Patch v5

The Addontype model is loaded/referenced (but not used) all over the app. So removing the table would probably require removing those references.

Merged patch with current revision.

Comment 9

[Mike Shaver (:shaver emeritus)]

(In reply to comment #3)
> Shortly looking at your patch, I'd personally refrain from doing this:
> 
> _('general_addontype_'.$addontype['Addontype']['text_id'])
> 
> because it will make it impossible to ever use a tool again to extract gettext
> tags from the source code files.

Extraction tools are a pretty low-value case versus being able to compute these sorts of things, IMO.  Making the code more fragile and indirect so that the (weak) gettext tools can cope is a bad trade.

Comment 10

[Fred Wenzel [:wenzel]]

Yeah that patch looks good (from looking at the diff). Unfortunately, I couldn't try it out because it gave me lots of errors when patching. I take it the patch does not align well with the current trunk anymore. Can you merge it and resubmit? Sorry 'bout that.

Will look at it again then, and probably apply.

Comment 11

[Jesper Kristensen]

Attachment: Patch v6

Updated to today's trunk

Comment 12

[Justin Scott [:fligtar]]

Comment on attachment 264510 [details] [diff] [review]
Patch v6

I won't have time to look at this until sometime next week. Fred can review the patch and I'll look at it when I can to make sure the developer/editor/localizer/admin side is OK.

Comment 13

[Fred Wenzel [:wenzel]]

Attachment: Rejects from patch v6

Sorry, but I am unable to apply your patch. What do you generate it with? Do you do an svn diff > mylittle.patch ? When I do a "patch -f -p0 --dry-run < blah.patch" I get a lot of rejects (see the attachment) and also "(Stripping trailing CRs from patch.)" for each hunk.

Anyone know what's going on?

Comment 14

[Jesper Kristensen]

I used svn diff (Using TortoiseSVN, so I don't know the exact command line arguments), it worked for fligtar in another bug.

Comment 15

[Jesper Kristensen]

Attachment: Patch v6, synced with trunk

Comment 16

[Wil Clouser [:clouserw]]

Marcio emailed me asking about the odd looking titles like "Navegar Themes por Categoria" - This bug would fix that problem.

Comment 17

[David Farning]

Are you interested in reviving this patch as a series of patches against the current trunk?

Comment 18

[Fred Wenzel [:wenzel]]

(In reply to comment #17)
> Are you interested in reviving this patch as a series of patches against the
> current trunk?

Absolutely. A bug requesting to make addon types *more dynamic* was recently WONTFIXed because of this bug here, so we might as well fix it :)

Comment 19

[David Farning]

Attachment: removes addontypes from admin view

Comment 20

[David Farning]

Attachment: removes addontypes from localizer view

Comment 21

[David Farning]

Attachment: removes addontypes from admin controller

Comment 22

[David Farning]

Attachment: removes addontypes from localizer controller

Comment 23

[David Farning]

The above attachments pick through Jesper's patch picking out the bits related to modifying addontypes via the web interface

Comment 24

[David Farning]

Just a follow up to see if this patch set is headed in the right direction.  If it is, I will finish breaking out jaspers original patch against trunk.

thanks

Comment 25

[Wil Clouser [:clouserw]]

(In reply to comment #24)
> Just a follow up to see if this patch set is headed in the right direction.  If
> it is, I will finish breaking out jaspers original patch against trunk.
> 
> thanks

Looks fine so far.  I'm assigning to you for the rest. :)

When you upload them feel free to request review from me (it's the r? box).

Comment 26

[Tomeu Vizoso]

Attachment: Removes addontypes from admin view

Comment 27

[Tomeu Vizoso]

Attachment: Removes addontypes from localizer view

Comment 28

[Tomeu Vizoso]

Attachment: Removes addontypes from admin controller

Comment 29

[Tomeu Vizoso]

Attachment: Removes addontypes from localizer controller

Comment 30

[Tomeu Vizoso]

Attachment: Remove name, name_plural and description columns from the table addontypes

Comment 31

[Tomeu Vizoso]

Attachment: Remove translated fields from the addontype model

Comment 32

[Tomeu Vizoso]

Attachment: Remove addontype actions from audit.php

Comment 33

[Tomeu Vizoso]

Attachment: Replace the use of the addontypes table for a constant array and gettext messages

Comment 34

[Tomeu Vizoso]

Sorry about the spam, I guess I could have merged some of the smaller patches.

I'm giving a hand to David in getting AMO to work for Sugar activities and in the future I hope we can offer some more patches that make easier to add addontypes without messing too much with the php code.

You can check here the rest of modifications we have already made:

http://git.sugarlabs.org/projects/activities-sugarlabs-org/repos/aso-tomeu/logs/master

Comment 35

[Wil Clouser [:clouserw]]

So far the patches seem to work for me.  Do you have a conversion script to go from database -> .po?

Comment 36

[Tomeu Vizoso]

Not really, have done it semi-automatically with something like this:

./locale/extract-po.sh
./locale/merge-po.sh messages.po ./locale
cp locale/en_US/LC_MESSAGES/messages.po /tmp/.
git-checkout locale/*/LC_MESSAGES/messages.po
diff -u site/app/locale/en_US/LC_MESSAGES/messages.po /tmp/messages.po  | grep ^\+msgid

Then added those new messages manually in the right positions inside the .po

Comment 37

[Tomeu Vizoso]

Ooops, misunderstood Wil's question, the steps above are for adding the new messages to the .po, not the existing translations to the .po files.

I don't have such script and would be great if someone could help with this as I'm going to be really busy in the next weeks.

Have entered a ticket in the SugarLabs' tracin the hope that someone will take this: http://dev.sugarlabs.org/ticket/170

Comment 38

[Jason Woofenden]

Attachment: script to convert translations table to .po files

I wrote a ruby script to import the contents of the "translations" sql table to a set of .po files.

You should be able to just run mysqldump, and run this on the output. It doesn't have a full sql parser, so if the output of mysqldump differs from the sql file I've tested on, it probably won't work.

If you'd like me to do any work on this, please let me know where I can get a mysqldump of the real data.

Comment 39

[Wil Clouser [:clouserw]]

I tried running the script and got:

mysqldump_to_many_po.rb:44: undefined method `lines' for #<File:translations.sql> (NoMethodError)

I tried changing "lines" to "readlines" but then got:

parse error 3: couldn't find end of data set

I'm using ruby 1.8.6 and just ran `mysqldump remora translations > translations.sql`

Just glancing at the script, but is this going to filter out just the add-on types?

Comment 40

[Jason Woofenden]

Drat... try "read" instead of "lines" like so:

f.read.each do

Comment 41

[Jason Woofenden]

oh! that "parse error 3" thing is from my code (complaining that it couldn't find a line ending with ";" to tell it that it's at the end of the section that needs reading.

ignore my last post, readlines is fine.

can you send me your translations.sql file?

I'd like to just hack my script so it works on the format you're getting from mysqldump on your server.

Thanks,   - Jason

Comment 42

[Wil Clouser [:clouserw]]

If you like http://people.mozilla.org/~clouserw/temp/translations.sql.gz

Like I mentioned though, if this isn't filtering out the add-on type strings it's going to generate huge .po files that we'll have to comb through again

Comment 43

[Jason Woofenden]

Attachment: script to generate .po files for add-on type translations

Progress! Wil helped me get a dataset that was both easier to parse, and contained only the translations for add-on type strings. And I updated my script to parse that format.

I also cleaned it up a bit, (made it set the charset field correctly and so on) and tested it by running msgfmt -c -v (which reportedly tests the file formatting in some detail). It spits out some errors, but the ones I looked at seem to be genuine problems in the input data, and not caused by my script.

See the comments near the top of the script to see how we generated the input file.

The output looks good to me. It generates .po files and the compiled .mo files.

Please let me know if I can be of further assistance.

Comment 44

[Wil Clouser [:clouserw]]

Good news indeed!

I ran the script - don't worry about those warnings.  Those are the localizers' doing and it'll still work fine.

After looking at the patch I started looking at the descriptions we have in the database.  Are those actually used on the site?  Because I can't find them anywhere and if there is no mapping gettext will just delete them anyway...  Unless someone chimes in with the location I'm in favor of throwing them out.

Which means we can ignore them (sorry if they've been extra work).  The only other thing I see left to do is change the format of the .po.  Right now the script isn't using the placeholders.  For example, "Themes" is not "Themes" it's "general_addontype_theme".

So, right now you're generating:

#: fake_c_file.c:5
msgid "Extension"
msgstr "Extension"

#: fake_c_file.c:11 
msgid "Extensions" 
msgstr "Extensions

And to use gettext properly it should be generating:
#: fake_c_file.c:5
msgid "general_addontype_extension"
msgid_plural "general_addontype_extension"
msgstr[0] "Extension"
msgstr[1] "Extensions"

For locales that have multiple types of plurals I can tweak them manually but that output should work fine.

Does that make sense?  Gettext plurals are a pain and confusing so let me know.  The script is working great. Thanks.

Comment 45

[Wil Clouser [:clouserw]]

Attachment: condensed patch

I'm tired of merging all these in to review so here is a condensed patch.  r+ anyway

Comment 46

[Tomeu Vizoso]

Hi, what needs to happen so this is committed? Thanks!

Comment 47

[Wil Clouser [:clouserw]]

I think Jason was tweaking his script to the output in comment #44.  That's the only thing left I think.

Comment 48

[David Farning]

I pinged Jason.  He will follow up soon.

Comment 49

[Jason Woofenden]

Attachment: convert addon types from translation table to many .po/.mo files

I improved my script so it uses the output format described by Wil (ie doing plurals properly)

more than one plural is not supported by this script because the dataset I have to work with doesn't have these. Wil said he'd deal with getting them in by hand.

Sorry for the time lag. I lost my wireless card and most of my hard drive, so fixing that and getting al set up again took all my "spare" time. But now I'm back in business. Let me know if/how I can help further.

Comment 50

[Wil Clouser [:clouserw]]

Attachment: webpages

So, first and foremost, thanks for the script Jason.  It's working well.

Secondly, this bug sucks. :)  Since I started working on it tonight:

- realized the $addontypes array was assuming the string was singular and the whole point here is to support many kinds of plural
- noticed getAddontypeName() was deprecated.  
- wrote Addontype::getName() and reworked Addontype::getNames()[1] as well as changed everywhere that called either of them.  
- replaced all the calls to getAddontypeName() and moved the $addontypes global into Addontype (and got rid of the global part)
- realized we have no fallback for plural strings so I created n___()
- discovered how to add n___ as a keyword to the extraction script
- discovered how to add a keywordspec to it so xgettext knows n___ is plural

At this point I think the code is workable.  Still needs to be done:

- write tests for ___(), n___() and the new Addontype functions
- merge English -> all locales, commit
- run Jason's script, msgmerge all the new .po files with their counterparts
- review all that merging and manually tweak files to fix broken plural-formats and plural strings
- commit new .po files; alert localizers that:  there are a couple new strings, if they are in the list of affected locales from the last step they should also review the plural forms of the new strings
- commit new code

I'm writing this down as much for me as anyone so I can keep track of what needs to happen next. kthx.

Also, I'm attaching the current code if anyone would like to see.

[1] Side note: We need to return plural names here but don't have a set number of objects whenever we refer to the plural name so I'm defaulting it to 999 objects which I hope will mean "many" in most languages.  We can override if necessary.

Comment 51

[Wil Clouser [:clouserw]]

> - merge English -> all locales, commit
> - run Jason's script, msgmerge all the new .po files with their counterparts
> - review all that merging and manually tweak files to fix broken plural-formats
> and plural strings
> - commit new .po files; alert localizers that:  there are a couple new strings,
> if they are in the list of affected locales from the last step they should also
> review the plural forms of the new strings

This is all in r22043

Comment 52

[Wil Clouser [:clouserw]]

Attachment: final? patch

Comment 53

[Wil Clouser [:clouserw]]

Comment on attachment 360810 [details] [diff] [review]
final? patch

I found a few more places that were using the db and removed the 2 addontypes views.

Comment 54

[Michael Morgan [:morgamic]]

Comment on attachment 360810 [details] [diff] [review]
final? patch

This worked for me -- couldn't find another place where the old type func was called.  Admin tools, upload, update scripts all worked fine.  Changes seem pretty transparent for rest of the app.

Comment 55

[Wil Clouser [:clouserw]]

Thanks everyone.  This is in r22069.  Also added some stuff for the db to the wiki.