<table><tr><td style="">ilic requested changes to this revision.<br />ilic added a comment.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D5208" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>I think it's fine to add this capability, especially given that Gettext (which Ki18n is an extension of) not only has it, but uses it exclusively (it does not search through any environment-derived directories).</p>
<p>However, there are a few questions of semantics.</p>
<p>So far it was always possible for a user to override the locale directory through environment variables, e.g. to test translation without building the full program. Should we still enable this, i.e. provide environment overriding also in case program sets the locale directory? But I guess we can postpone this question, since as you mention in KDE 4 Plasma the locale directory was being forced by the program anyway by other means.</p>
<p>On the other side, if the locale directory for the domain is set, should (as in currently proposed implementation) environment-derived directories still be looked through if catalog is not found in the set directory?</p>
<p>I think better name for the method is setDomainLocaleDir, because if it is called again for the same domain, it will override the previous setting and not add another directory for it.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5208#inline-21515" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">klocalizedstringtest.cpp:59</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">m_hasFrench</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">m_hasFrench</span> <span style="color: #aa2211">=</span> <span class="n">compileCatalogs</span><span class="p">(</span><span class="n">dataDir</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">m_hasFrench</span> <span style="color: #aa2211">=</span> <span class="n">compileCatalogs</span><span class="p">(<span class="bright">{</span></span><span class="bright"></span><span class="n"><span class="bright">QFINDTESTDATA</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #766510"><span class="bright">"po/fr/ki18n-test.po"</span></span><span class="bright"></span><span class="p"><span class="bright">),</span></span><span class="bright"> </span><span class="n"><span class="bright">QFINDTESTDATA</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #766510"><span class="bright">"po/fr/ki18n-test-qt.po"</span></span><span class="bright"></span><span class="p"><span class="bright">)},</span></span><span class="bright"> </span><span class="n">dataDir</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I think we're not allowed to use initializer lists yet, due to MSVC11 support.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5208#inline-21516" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">klocalizedstringtest.cpp:523</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">compileCatalogs</span><span class="p">({</span><span class="n">QFINDTESTDATA</span><span class="p">(</span><span style="color: #766510">"po/fr/ki18n-test2.po"</span><span class="p">)},</span> <span class="n">dir</span><span class="p">.</span><span class="n">path</span><span class="p">());</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">KLocalizedString</span><span style="color: #aa2211">::</span><span class="n">setApplicationDomain</span><span class="p">(</span><span style="color: #766510">"ki18n-test2"</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">KLocalizedString</span><span style="color: #aa2211">::</span><span class="n">addDomainLocaleDir</span><span class="p">(</span><span style="color: #766510">"ki18n-test2"</span><span class="p">,</span> <span class="n">dir</span><span class="p">.</span><span class="n">path</span><span class="p">()</span> <span style="color: #aa2211">+</span> <span style="color: #766510">"/locale"</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">setApplicationDomain should not be called twice, use i18nd method instead, like i18nd("ki18n-test2", "Cheese").</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5208#inline-21517" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kcatalog.cpp:124</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QMutexLocker</span> <span class="n">lock</span><span class="p">(</span><span style="color: #aa2211">&</span><span class="n">catalogStaticData</span><span style="color: #aa2211">-></span><span class="n">mutex</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="n">QString</span> <span class="n">customLocaleDir</span> <span style="color: #aa2211">=</span> <span class="n">catalogStaticData</span><span style="color: #aa2211">-></span><span class="n">customCatalogDirs</span><span class="p">[</span><span class="n">domain</span><span class="p">];</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Only read access is needed here, user QHash::value instead of QHash::operator[].</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5208#inline-21519" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kcatalog.cpp:126</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="n">QString</span> <span class="n">customLocaleDir</span> <span style="color: #aa2211">=</span> <span class="n">catalogStaticData</span><span style="color: #aa2211">-></span><span class="n">customCatalogDirs</span><span class="p">[</span><span class="n">domain</span><span class="p">];</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">!</span><span class="n">customLocaleDir</span><span class="p">.</span><span class="n">isEmpty</span><span class="p">()</span> <span style="color: #aa2211">&&</span> <span style="color: #aa2211">!</span><span class="n">QFileInfo</span><span style="color: #aa2211">::</span><span class="n">exists</span><span class="p">(</span><span class="n">customLocaleDir</span> <span style="color: #aa2211">+</span> <span class="n">relpath</span><span class="p">))</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">return</span> <span class="n">customLocaleDir</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Maybe I'm seeing something wrongly here, but... customLocaleDir + relpath may miss path separator? Really !QFileInfo::exists and not QFileInfo::exists?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5208#inline-21518" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kcatalog.cpp:151</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QMutexLocker</span> <span class="n">lock</span><span class="p">(</span><span style="color: #aa2211">&</span><span class="n">catalogStaticData</span><span style="color: #aa2211">-></span><span class="n">mutex</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">catalogStaticData</span><span style="color: #aa2211">-></span><span class="n">customCatalogDirs</span><span class="p">.</span><span class="n">contains</span><span class="p">(</span><span class="n">domain_</span><span class="p">))</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Also only read access needed.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R249 KI18n</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D5208" rel="noreferrer">https://phabricator.kde.org/D5208</a></div></div><br /><div><strong>To: </strong>davidedmundson, Frameworks, ilic<br /><strong>Cc: </strong>ilic<br /></div>