Closed
Bug 403993
Opened 17 years ago
Closed 16 years ago
Make python compare-locales script work with SeaMonkey
Categories
(SeaMonkey :: Build Config, defect)
SeaMonkey
Build Config
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: kairo, Assigned: asrail)
References
Details
Attachments
(4 files, 10 obsolete files)
257 bytes,
patch
|
Details | Diff | Splinter Review | |
245 bytes,
patch
|
Details | Diff | Splinter Review | |
213 bytes,
patch
|
Pike
:
review+
|
Details | Diff | Splinter Review |
1.64 KB,
patch
|
asrail
:
review+
kairo
:
approval-seamonkey2.0a1+
|
Details | Diff | Splinter Review |
Bug 395396 made python compare-locales exceptions app-dependent, we should add a filter.py to suite for those. Axel says we could also figure out there how to make extensions/irc optional for l10n.mk, if I understood correctly what he said on IRC.
Comment 1•16 years ago
|
||
You'll need an l10n.ini now, too. See bug 445217.
Reporter | ||
Updated•16 years ago
|
Summary: add suite/locales/filter.py to work with python compare-locales script → Make python compare-locales script work with SeaMonkey
Assignee | ||
Comment 2•16 years ago
|
||
Requesting Kairo review for the list of files. Remember to ask for Axel review latter.
Attachment #334303 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #334303 -
Flags: review? → review?(kairo)
Assignee | ||
Comment 3•16 years ago
|
||
Comment on attachment 334303 [details] Proposed l10n.ini for suite Bug 445146 will require changes here.
Attachment #334303 -
Attachment is obsolete: true
Attachment #334303 -
Flags: review?(kairo)
Reporter | ||
Comment 4•16 years ago
|
||
probably, yes. The reason why I haven't looked into this yet is that we probably want a filter.py as well and I've had an attempt at that sitting around on my disk for some time now, but i still don't have compare-locales set up to be able to use it. Is there a good doc about how to set it up?
Comment 5•16 years ago
|
||
(In reply to comment #4) > Is there a good doc about how to set it up? http://developer.mozilla.org/en/Compare-locales
Assignee | ||
Comment 6•16 years ago
|
||
It will require a change on the file: remove the reference to mozilla/editor/ui. Probably, nothing else. But I do prefer to wait and test, SeaMonkey will require the changes to the dtd's, to remove the incorrect comments before working properly. Some solution to the "#expand" will have to be found. SeaMonkey will also require a filter.py, but the work here doesn't depend on that.
Reporter | ||
Comment 7•16 years ago
|
||
(In reply to comment #6) > It will require a change on the file: remove the reference to > mozilla/editor/ui. > Probably, nothing else. But I do prefer to wait and test, > > SeaMonkey will require the changes to the dtd's, to remove the incorrect > comments before working properly. Those not-actually-incorrect comments have a bug filed, right? And AFAIK, there was a positive review for it. > Some solution to the "#expand" will have to be found. Part of bug 451601. > SeaMonkey will also require a filter.py, but the work here doesn't depend on > that. Oh, that is what this bug was originally filed for :)
Assignee | ||
Comment 8•16 years ago
|
||
(In reply to comment #7) > Those not-actually-incorrect comments have a bug filed, right? And AFAIK, there > was a positive review for it. I haven't found it. But that should block this bug. > > Some solution to the "#expand" will have to be found. > > Part of bug 451601. Blocking this one now. > > SeaMonkey will also require a filter.py, but the work here doesn't depend on > > that. > > Oh, that is what this bug was originally filed for :) I meant the patches are orthogonal.
Reporter | ||
Comment 9•16 years ago
|
||
(In reply to comment #8) > (In reply to comment #7) > > Those not-actually-incorrect comments have a bug filed, right? And AFAIK, there > > was a positive review for it. > > I haven't found it. But that should block this bug. Bug 451065 - unless it's other comments you're talking of.
Depends on: 451065
Reporter | ||
Comment 10•16 years ago
|
||
This is the filter.py I had lying around for a while and the l10n.ini from asrail. Not sure if all this is correct for the current compare-locales version, so requesting review from Axel, who probably knows compare-locales better than anyone else. ;-) Even if bug 445146 probably needs us to change the editor/ui stuff, I want us to get something in.
Assignee | ||
Comment 11•16 years ago
|
||
+ if mod not in ("netwerk", "dom", "toolkit", "security/manager", + "extensions/reporter", "extensions/spellcheck", + "extensions/irc", "extensions/venkman", + "editor/ui", "suite"): + return False + if mod != "suite" and mod != "extensions/spellcheck" and mod != "extensions/irc" and mod != "extensions/venkman": nit: you can use "mot not in" tuple on the second if, just like the first one. + if mod == "extensions/irc" and loc in irc_locales: + # compare chatzilla if current locale is said to be present for it + return True + if mod == "extensions/venkman" and loc in venkman_locales: + # compare venkman if current locale is said to be present for it + return True ... + return True Are you willing to not compare venkman and CZ if the language is not said to be present for it? Both will fail to the last "return True" statement and will be compared anyway. What to do about the import wizard variants? The translator may choose to add or remove some of the entries - I do that. I use this: if re.match(r".*_(ie|opera|dogbert|seamonkey|phoenix|safari|macie)", entity): return False But this solution is far from perfect. Maybe Axel could tell us if there is any elegant solution supported on compare-locales.
Assignee | ||
Updated•16 years ago
|
Assignee: kairo → asrail
Assignee | ||
Comment 12•16 years ago
|
||
See comment #11.
Attachment #336195 -
Attachment is obsolete: true
Attachment #336226 -
Flags: review?(l10n)
Attachment #336195 -
Flags: review?(l10n)
Reporter | ||
Comment 13•16 years ago
|
||
Comment on attachment 336226 [details] [diff] [review] Update to the patch >diff --git a/suite/locales/l10n.ini b/suite/locales/l10n.ini >+[compare] >+dirs = suite >+ mozilla/editor/ui >+ mozilla/extensions/venkman >+ mozilla/extensions/inspector >+ mozilla/extensions/typeaheadfind >+ mozilla/extensions/webdav webdav is dead and not even checked out any more by client.py, so it's wrong for sure. typeaheadfind contains no L10n as far as I know, so I wonder why it's listed here. And inspector L10n is not tracked by L10n repositories yet, is it really supposed to be listed here? >+[extras] >+dirs = mozilla/extensions/spellcheck >+ mozilla/extensions/irc Why is irc listed in [extras] and venkman in [compare]? They are both set up the same way, so I'd guess they should be treated the same. Both those dirs contain optional L10n, depending on their all-locales files.
Assignee | ||
Comment 14•16 years ago
|
||
Sorry, the issue with irc and inspector was a mistake. I've removed the other dirs. I've found a lot of issues with filter.py today. I've made some changes, but some points still not working properly. Kairo: you first wanted not to compare spellchek and latter wanted to compare it. I've taken into account the latter and the comment which comes with it. "loc" is not defined and the check for loc in irc or venkman locales fails. I'll take a look into that and in why netwerk is not compared. I've changed the entity check and noticed that the only files outside mozilla/extensions which are not compared are the .src and .png files, so I've changed that 'if'. Not requesting review due the pending points.
Attachment #336226 -
Attachment is obsolete: true
Attachment #336226 -
Flags: review?(l10n)
Reporter | ||
Comment 15•16 years ago
|
||
(In reply to comment #14) > I've found a lot of issues with filter.py today. I'm sure of that, I created it without a lot of knowledge and based on an old copy of the Firefox one. > Kairo: you first wanted not to compare spellchek and latter wanted to compare > it. > I've taken into account the latter and the comment which comes with it. We want to go the same way Firefox goes there, and I guess that's not comparing spellcheck. > "loc" is not defined and the check for loc in irc or venkman locales fails. > I'll take a look into that and in why netwerk is not compared. I suppose it was defined in an older compare-locales version then - it's supposed to hold the code of the compared locale. > Not requesting review due the pending points. I guess we probably need some input from Axel there.
Reporter | ||
Comment 16•16 years ago
|
||
With your newest patch, I get "// add and localize this file" warnings about all files in mozilla/editor/ui/chrome mozilla/extensions/irc mozilla/extensions/spellcheck/hunspell mozilla/extensions/venkman So this doesn't really work that well, actually.
Assignee | ||
Comment 17•16 years ago
|
||
(In reply to comment #16) > With your newest patch, I get "// add and localize this file" warnings about > all files in > mozilla/editor/ui/chrome > mozilla/extensions/irc > mozilla/extensions/spellcheck/hunspell > mozilla/extensions/venkman > > So this doesn't really work that well, actually. compare-locales looks for files on "mozilla/extensions" on the translation side, so it doesn't find the folders. hunspell won't be checked anymore. About irc and venkman, maybe the solution will be to create l10n.ini for each one. I think this solution is better than to hardcode the "mozilla" directory on compare-locales.
Assignee | ||
Comment 18•16 years ago
|
||
This is required for using the newest compare-locales with Chatzilla.
Assignee | ||
Comment 19•16 years ago
|
||
Assignee | ||
Comment 20•16 years ago
|
||
Attachment #336303 -
Attachment is obsolete: true
Assignee | ||
Comment 21•16 years ago
|
||
I won't request review now because it requires a change on compare-locales. You can test with compare-locales from here: http://hg.mozilla.org/users/asrail_gmail.com/tooling The web interface for this repository is broken as of now, but it works.
Attachment #336260 -
Attachment is obsolete: true
Assignee | ||
Comment 22•16 years ago
|
||
Attachment #336304 -
Attachment is obsolete: true
Assignee | ||
Comment 23•16 years ago
|
||
Attachment #336305 -
Attachment is obsolete: true
Comment 24•16 years ago
|
||
How would a localizer go about when they want to start translating, say, cz? I find it tough to require them to hack all-locales right from the start. I probably need to redo the [includes] anyway, going for something that works both remote and local, I picture having something like includes=irc,venkman,toolkit in [general], and make all those separate sections like [irc] localpath=mozilla/extensions/irc/locales/l10n.ini url=http://mxr.mozilla.org/mozilla/source/extensions/irc/locales/l10n.ini optional=True and then have a param that would say compare-locales --with=irc,venkman ...l10n.ini l10n-central ab-CD We could make the build process pass in the params based on all-locales, so that by default it does the right thing. What do you think? PS: I'm not yet sure how to get changes detected in buildbot.
Reporter | ||
Comment 25•16 years ago
|
||
Axel, I like your idea, and if it's going to be implemented this year still, then I feel good about just excluding chatzilla and venkman from compare-locales in SeaMonkey for now, they both are running compare-locales.pl at build time right now, so we have some kind of check there, and once we got your solution, we'll have something that really works.
Assignee | ||
Comment 26•16 years ago
|
||
l10.ini required, since it is inside "mozilla" directory.
Attachment #336778 -
Flags: review?(l10n)
Assignee | ||
Comment 27•16 years ago
|
||
- Updating for the new location of "editor/ui" - Removing the references to venkman and chatzilla - Not requiring anymore changes to the parameters passed to the test function - Requires l10n.ini for "reporter" (attached)
Attachment #336309 -
Attachment is obsolete: true
Attachment #336780 -
Flags: review?(l10n)
Assignee | ||
Comment 28•16 years ago
|
||
(In reply to comment #24) > How would a localizer go about when they want to start translating, say, cz? I > find it tough to require them to hack all-locales right from the start. That's a good point. The localizer always could use the l10n.ini from chatzilla or venkman and run separately from SM, but your proposal is better. > I probably need to redo the [includes] anyway, going for something that works > both remote and local, I picture having something like > > includes=irc,venkman,toolkit in [general], and make all those separate sections > like > [irc] > localpath=mozilla/extensions/irc/locales/l10n.ini > url=http://mxr.mozilla.org/mozilla/source/extensions/irc/locales/l10n.ini > optional=True I would prefer to list the obligatory sections on a proper place instead of using a boolean. > and then have a param that would say > > compare-locales --with=irc,venkman ...l10n.ini l10n-central ab-CD Maybe the syntax used by the the autotools is more powerful and better to this case: --with-irc (alternatively --with-irc=yes), --without-irc (alternatively --with-irc=no), --with-irc=[path] (assume --with-irc=yes) So we can change the default from no to yes, from yes to no and use an alternative file, for whatever reason - for instance: a lot of trees, in case the person is also a developer. > We could make the build process pass in the params based on all-locales, so > that by default it does the right thing. That's a nice idea. > PS: I'm not yet sure how to get changes detected in buildbot. I didn't understand this.
Assignee | ||
Comment 29•16 years ago
|
||
I had made a wrong assumption about new files and its entities. This patch fix this bug and I've added a check to ignore hidden files (files starting with a dot), temporary files (files starting with a dot or a "#") and files from rejects of patches (files ending on ".orig" and ".rej").
Attachment #336780 -
Attachment is obsolete: true
Attachment #336973 -
Flags: review?(l10n)
Attachment #336780 -
Flags: review?(l10n)
Assignee | ||
Comment 30•16 years ago
|
||
Axel: ping on the reviews.
Comment 31•16 years ago
|
||
Comment on attachment 336973 [details] [diff] [review] Updating Looks good to me, just that the comment on entity == None is wrong. Make that if entity is None: # missing and obsolete files ?
Attachment #336973 -
Flags: review?(l10n) → review+
Updated•16 years ago
|
Attachment #336778 -
Flags: review?(l10n) → review+
Reporter | ||
Updated•16 years ago
|
Attachment #336973 -
Flags: approval-seamonkey2.0a1+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 32•16 years ago
|
||
Assignee | ||
Updated•16 years ago
|
Keywords: push-needed
Assignee | ||
Comment 33•16 years ago
|
||
The last attachment has the approved patch as should be pushed on comm-central - just a comment and a minor changes.
Attachment 338778 [details] [diff] should be checked in on CVS.
Reporter | ||
Comment 34•16 years ago
|
||
The correct keyword is checkin-needed. And attachment 338778 [details] [diff] [review] actually should go into mozilla-central, that's where reporter lives now.
Reporter | ||
Updated•16 years ago
|
Attachment #336430 -
Attachment description: Addning all-locales → venkman patch, adding all-locales
Reporter | ||
Updated•16 years ago
|
Attachment #336431 -
Attachment description: Adding all-locales → chatzilla patch, adding all-locales
Reporter | ||
Comment 35•16 years ago
|
||
Attachment 336778 [details] [diff] it is, actually.
Are the venkman and chatzilla patches still needed or obsolete?
And it's usual to mark the old patch obsolete and forward the review flags, i.e. mark them + yourself if they have been granted on the old one with comments.
Assignee | ||
Comment 36•16 years ago
|
||
I've seen some people using push-needed to triage mercurial patches. Venkman and Chatzilla patches aren't obsoletes, but they won't be required for this part of the patch. They can be used freely manually for each one if venkman or chatzilla people wants so, but they won't be used on suite until compare-locales got incremented.
Assignee | ||
Comment 37•16 years ago
|
||
Comment on attachment 337995 [details] [diff] [review] Addressing Axel comments (checked in) Forwarding Axel review.
Attachment #337995 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Attachment #336973 -
Attachment is obsolete: true
Assignee | ||
Comment 38•16 years ago
|
||
(In reply to comment #36) > I've seen some people using push-needed to triage mercurial patches. Wrongly... thanks.
Keywords: push-needed
Reporter | ||
Comment 39•16 years ago
|
||
Comment on attachment 337995 [details] [diff] [review] Addressing Axel comments (checked in) forwarding the approval. And yes, push-needed is only for patches that need to be pushed *by the sheriff* for times when the tree only allows checkins from that sheriff - which is never true for comm-central ;-)
Attachment #337995 -
Flags: approval-seamonkey2.0a1+
Reporter | ||
Comment 40•16 years ago
|
||
Pushed suite/ part as http://hg.mozilla.org/comm-central/rev/d1004044a681 and reporter part as http://hg.mozilla.org/mozilla-central/rev/ab5bd6c98c1e asrail, I'm leaving it to you to mark fixed, or leave open until it fully works, however you like it :)
Reporter | ||
Updated•16 years ago
|
Keywords: checkin-needed
Reporter | ||
Updated•16 years ago
|
Attachment #336778 -
Attachment description: l10n.ini for reporter → l10n.ini for reporter (checked in)
Reporter | ||
Updated•16 years ago
|
Attachment #337995 -
Attachment description: Addressing Axel comments [should be pushed] → Addressing Axel comments (checked in)
Reporter | ||
Comment 41•16 years ago
|
||
From what I see, compare-locales works with SeaMonkey now, so closing this bug.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•