-
Notifications
You must be signed in to change notification settings - Fork 383
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
New locale: 'ko' #23
New locale: 'ko' #23
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OHMYGOSH! 👍
Thanks for this. I have a few questions for you, but this looks really good overall.
@@ -12,7 +12,7 @@ | |||
<script src="../../resources/timericon.js"></script> | |||
<script src="../../resources/widgetlist.js"></script> | |||
<script src="../../resources/lang.js"></script> | |||
<script src="../../resources/lang-en.js"></script> | |||
<script src="../../resources/lang/en.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you include lang/ko.js here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
resources/lang/ko.js
Outdated
return Regexes.Parse(' 1E:' + target + ' loses the effect of ' + effect + ' from ' + attacker + '.*\.'); | ||
}; | ||
|
||
this.countdownStartRegex = function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these the same in Korean? Could you add a "TODO: not translated yet" if not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resources/lang/ko.js
Outdated
// We can not look for log messages from FFXIV "You use X" here. Instead we | ||
// look for the actual ability usage provided by the XIV plugin. | ||
// Also, the networked parse info is given much quicker than the lines from the game. | ||
this.youUseAbilityRegex = function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't look line by line, but at first glance most of these regex functions look unchanged from the en version. I didn't know at the time whether or not other languages would have the same log messages. Could you move them into the base CactbotLang class so both en and ko can share and not duplicate them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can Regex
es be a method? Since we (yes, Korean) using English version of the plugin, so I can't predict how this should behave on ja/de/fr versions, to always override or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, agreed that I'm not sure ja/de/fr versions will need to do. However, if some derived class needs to override it, they can just override it themselves.
I went ahead and moved these in 30888c4, which hopefully won't cause you too much conflict.
resources/lang/en.js
Outdated
Brotherhood: 'Brotherhood', | ||
Devotion: 'Devotion', | ||
FoeRequiem: 'Foe Requiem', | ||
BluntResistDown: 'Blunt Resistance Down', // 0x23d, 0x335, 0x3a3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure at first what these hex codes meant, but I looked through logs and I think I understand more of the log format. I am very curious about this!
It seems like pairs of :.*0F:(effectId):
designate the effect id. Where did all of these values come from? Did you investigate these yourself or is there a list somewhere?
Some of these values, I see in logs, but others I do not. For example, Foe Requiem up I see [15:57:43.750] 15:1067BEA6:Azel Nox:73:Foe Requiem:1067BEA6:Azel Nox:90030F:8B:
but I do not see any :8C: for Foe Requiem lost. Similarly, Left Eye and Right Eye are not abilities but only effects, and so I don't know where the values 0x4a0 and 0x49f come from.
Can you help me understand more? I'd love to know. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is internal effect ID and used very widely. also skills, zones has unique ID like this.
0x8C
is for enemies. (ps: XIVDB uses the decimal ID to build its URL)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't miss out that we can access unparsed log like saved to a .log file, like OverlayPlugin and SpecialSpellTimer do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see! Thanks for the explanation. By not seeing 0x8C
, I meant that I didn't see it anywhere in the log, applied to either players or enemies.
I'll have to investigate the unparsed log, thanks for the pointer!
resources/lang/en.js
Outdated
Aetherflow: 'Aetherflow', | ||
ChainStrategem: 'Chain Strategem', | ||
Hypercharge: 'Hypercharge', | ||
DragonKick: 'Dragon Kick', // 0x4a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were these id comments useful for you when translating? I feel like these ids are all already inside of lang.js and so duplicating them here is a bit noisy. I think I'd prefer to not have them if it was just me, but if they were useful in translating, then I'm happy to leave them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Translators can reference this ID to query XIVDB (0x8c
-> /status/140
), or extracted FFXIV data itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it was duplicate, I didn't saw them. I'll remove these comments.
resources/lang/ko.js
Outdated
}; | ||
|
||
this.countdownStartRegex = function() { | ||
return Regexes.Parse(/Battle commencing in (\y{Float}) seconds!/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is TODO, I didn't saw countdown recently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change your upstream branch to be quisquous:master instead of quisquous:localization? I merged localization into master already and had just made a branch for code review purposes.
Once you do that, I think this should be ready to commit.
resources/lang/ko.js
Outdated
// We can not look for log messages from FFXIV "You use X" here. Instead we | ||
// look for the actual ability usage provided by the XIV plugin. | ||
// Also, the networked parse info is given much quicker than the lines from the game. | ||
this.youUseAbilityRegex = function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, agreed that I'm not sure ja/de/fr versions will need to do. However, if some derived class needs to override it, they can just override it themselves.
I went ahead and moved these in 30888c4, which hopefully won't cause you too much conflict.
This doesn't localize jobs yet, but moves all strings and regular expressions out to a separate lang-en.js file. It seems plausible for here to create additiona lang-kr.js and lang-jp.js files if folks want to take on the task of converting all the strings.
08a3da3
to
2f6144d
Compare
I (accidentally) rebased onto |
resources/lang.js
Outdated
// We can not look for log messages from FFXIV "You use X" here. Instead we | ||
// look for the actual ability usage provided by the XIV plugin. | ||
// Also, the networked parse info is given much quicker than the lines from the game. | ||
youUseAbilityRegex() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you leave these in CactbotLanguage but remove them from CactbotLanguageEn and CactbotLanguageKo so that they are not duplicated? I think youStartUsingRegex, youGainEffectRegex, youLoseEffectRegex, abilityRegex, gainsEffectRegex, and losesEffectRegex should all be in CactbotLanguage if they are shared. Sorry if I haven't communicated this clearly enough--that was why I moved these functions from CactbotLanguageEn to CactbotLanguage in master.
The countdown regex functions (since they'll be different) can be in CactbotLanguageEn and CactbotLanguageKo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my merging fault, sorry ;( - cherry-picked and amended on 04501a1.
Thanks for your patience with all the back and forth! I really appreciate all the work. :) |
Also, should rename all class functions to start with lower case for consistency, but not in this patch to avoid causing conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries at all! I just feel guilty going many rounds of comments and wanted to make sure I was being clear. :)
ui/jobs/jobs.js
Outdated
@@ -360,8 +360,9 @@ function setupBuffTracker() { | |||
sortKey: 9, | |||
}, | |||
requiem: { | |||
gainRegex: gLang.gainsEffectRegex(gLang.kEffect.FoeRequiem, '(\\y{Name})', '\\1'), | |||
loseRegex: gLang.losesEffectRegex(gLang.kEffect.FoeRequiem, '(\\y{Name})', '\\1'), | |||
// TODO: Can't use \y{Name} because https://github.com/quisquous/cactbot/issues/22. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is accidental too. Can you revert these three lines?
Thanks! Let me know what I can do to help debug the message issue you're seeing. That really seems like the next issue to getting this working for you. |
+ fixed some missingTranslation labels
+ fixed some missingTranslation labels
also:
resources/lang
.