<table><tr><td style="">pino requested changes to this revision.<br />pino 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/D17494">View Revision</a></tr></table><br /><div><div><p>The approach here should be changed: instead of hardcoding the names of plugins in the <tt style="background: #ebebeb; font-size: 13px;">styleCombo</tt>, they ought to be read directly from the <tt style="background: #ebebeb; font-size: 13px;">sceneModel()</tt> of the GL widget. This way:</p>
<ol class="remarkup-list">
<li class="remarkup-list-item">the strings come directly from avogadro itself, no more need to translate them in kalzium</li>
<li class="remarkup-list-item">the list of styles reflects what avogadro provides, so no more mismatch between what is hardcoded in kalzium vs the plugins avogadro has</li>
<li class="remarkup-list-item">the documentation of kalzium ought to better not list all the plugins/styles available, since they depend on what avogadro has (mentioning a couple is more than enough, IMHO)</li>
</ol>
<p>This also means that more i18n work is needed in avogadro itself.</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/D17494#inline-96679">View Inline</a><span style="color: #4b4d51; font-weight: bold;">index.docbook:494</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; "> <para>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> The Molecular Editor allows you to view and edit molecules using <ulink url="http://avogadro.<span class="bright">openmolecules.net/wiki/Main_Page</span>">Avogadro 2</ulink> libraries.
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> The Molecular Editor allows you to view and edit molecules using <ulink url="http<span class="bright">s</span>://avogadro.<span class="bright">cc/</span>">Avogadro 2</ulink> libraries.
</div><div style="padding: 0 8px; margin: 0 4px; "> </para>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This change is fine, just commit it directly outside of this review request.</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/D17494#inline-96678">View Inline</a><span style="color: #4b4d51; font-weight: bold;">index.docbook:498-502</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(251, 175, 175, .7);"> <!-- The downloaded from kde-apps data will be saved in <filename class="directory">$KDEHOME/share/apps/kalzium/molecules</filename>, they should be renamed to <filename><replaceable>molecule_name</replaceable>.cml</filename> manually. <envar>$KDEHOME</envar> is usually a hidden folder in your Home folder called <filename class="directory">.kde</filename> or <filename class="directory">.kde4</filename>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> "$KDEHOME" is not always defined eg not in kubuntu 11.04
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <filename class="directory">`kde4-config - -localprefix`/share/apps/kvtml</filename>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> (without whitespace between '- -') is afaik always defined
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> -->
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This change is fine, so just commit it outside of this review request.</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/D17494#inline-96624">View Inline</a><span style="color: #4b4d51; font-weight: bold;">yurchor</span> wrote in <span style="color: #4b4d51; font-weight: bold;">moleculeview.cpp:176</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">But it does (tested). Does not work when changed to Qt::UserRole.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">It "works" mostly because there is no i18n system in avogadrolibs, even though the strings are supposed to be translatable (marked with tr(), mentioned "shown in the gui" in the API docs, etc).</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R326 Kalzium</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D17494">https://phabricator.kde.org/D17494</a></div></div><br /><div><strong>To: </strong>yurchor, KDE Edu, pino<br /><strong>Cc: </strong>kde-doc-english, pino, kde-edu, skadinna, narvaez, apol<br /></div>