<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/104729/">http://git.reviewboard.kde.org/r/104729/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 28th, 2012, 1:15 p.m., <b>C. Boemann</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/104729/diff/1/?file=58555#file58555line419" style="color: black; font-weight: bold; text-decoration: underline;">plugins/textshape/TextTool.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void TextTool::createActions()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">419</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">action</span> <span class="o">=</span> <span class="k">new</span> <span class="n">KAction</span><span class="p">(</span><span class="n">KIcon</span><span class="p">(</span><span class="s">"merge"</span><span class="p">),</span> <span class="n">i18n</span><span class="p">(</span><span class="s">"Merge Cells"</span><span class="p">),</span> <span class="k">this</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">419</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">action</span> <span class="o">=</span> <span class="k">new</span> <span class="n">KAction</span><span class="p">(</span><span class="n">KIcon</span><span class="p">(</span><span class="s">"<span class="hl">edit-table-cell-</span>merge"</span><span class="p">),</span> <span class="n">i18n</span><span class="p">(</span><span class="s">"Merge Cells"</span><span class="p">),</span> <span class="k">this</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">well the reason we don't use the "real" names for merge and spli is because they are not in kde 4.3, which is our minimum requirement at the moment.
so it's a no go until the requirement changes</pre>
</blockquote>
<p>On April 30th, 2012, 8:22 a.m., <b>Inge Wallin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think the time has come to up this requirement. How about KDE 4.5 for Calligra 2.5? I want this patch in.</pre>
</blockquote>
<p>On May 1st, 2012, 2:53 p.m., <b>Friedrich W. H. Kossebau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I fear there are a few icon ids used already in Calligra code which are not in part of the oxygen icon-set as published with KDE 4.3. E.g. those with the "-calligra" postfix are also found without that postfix. And the "merge" and "split" icons you gave as example are in fact used with the "real" name multiple times in the very same file, it's just those two occurrences which are "unreal".
I guess Calligra really needs some way to automatically check the correctness of the icons used with regard to the minimal dependency. It's not easy to get an overview yourself, and most might simply look for an icon within their current system, forgetting that the icon is not ensured to also be in the 4.3 icon set.
You might have seen my blog post about how that could be solved by using some kind of macro-based markup for icon ids. During the WE I experimented with that to see how it works out. Will post the resulting patch as a separate review request to collect your feedback now. Extracting the icon-ids and comparing against the list of icon-ids in 4.3 (as well as the ones coming with Calligra) still needs to be implemented, but it's really time for some first feedback :)</pre>
</blockquote>
<p>On May 1st, 2012, 3:34 p.m., <b>C. Boemann</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">oooh you are right those lines shouldn't just be changed - those entire actions should be deleted. i'll do that now and then you can update your patch, and you have my ship-it
Yes i read your blog. :)
</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Oh, right, I remember I wanted to check why those lines were so similar. But then I wanted to keep my focus on the icon ids, and succeeded with that, it seems :)
Patch updated, so you can give the "ship-it" if otherwise alright with it.</pre>
<br />
<p>- Friedrich W. H.</p>
<br />
<p>On May 1st, 2012, 4:27 p.m., Friedrich W. H. Kossebau wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Calligra.</div>
<div>By Friedrich W. H. Kossebau.</div>
<p style="color: grey;"><i>Updated May 1, 2012, 4:27 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I grepped for all "KIcon(" (big mistake, there are quite a lot :) ) and checked if the icon exists in the Oxygen theme, is installed by Calligra itself, or are simply missing. There are more icon names/ids in the code, but finding them would have been even more work. Left for somebody else :)
(ideally icon names should have a marker, so they can be simply grepped, like translated strings are).
I used the version 4.8.2-139.1 of oxygen-icon-theme, as installed from obs://build.opensuse.org/KDE. Which is the oxygen version that Calligra depends on? I could not find a check in the CMakeLists.txt or a hint in the README.PACKAGERS .
So, I found a few typos, a few names refering to non-existing icons, and some icons for which I propose another choice. So the patch is about:
* "media-playback-start" is used in kexi a few times where "system-run" might be better
* "printer" where "document-print" might be better
* "words" and "tables" where "application-vnd.oasis.opendocument.text" and "application-vnd.oasis.opendocument.spreadsheet") might be better
* non-existing "edit_remove", "edit_add", "text_left", "text_center", "text_right", "icons", "chart-bar", "paste", "office-calendar", replaced with what seems a good match
* typo in "format-text-suscript", "video-x-genenric"
* "chart_*" icon names where oxygen has now "office-chart-*", as also used in rest of file. But some "chart_*" are without any match and without any icon, please comment on that
* "insert_table_*" and "delete_table_*" could be replaced with "edit-table-insert-*" and "edit-table-delete-*"
* "merge" and "split" seem to miss "edit-table-cell-" prefix, like in rest of icon names in that file
* "show_sheet_column" also looks like an accidental change that should be "show_table_column" still
* "mail-message" could be "internet-mail" to match other icons in sheets/dialogs/LinkDialog.cpp (and text there could be improved to "Mail Address" and "Email Address")
* "klipper" might be better "edit-paste" there
As far as I could see nothing related to http://community.kde.org/Calligra/Icons . But I found other icons to be missing (at least by the code, did not check all these by running the code):
document-page-setup (KexiProjectListView.cpp, KexiProjectNavigator.cpp, KexiMainWindow.cpp)
fontsizeup, fontsizedown (CellToolBase.cpp)
inspector (CellToolBase.cpp)
clearComment (CellToolBase_p.cpp)
snap-boundingbox (SnapGuideConfigWidget.cpp)
duplicate (braindump/src/View.cpp)
object-locked (braindump/src/import/ToolDocker.cpp, krita/plugins/extensions/imagesize/multilock_button.cc)
object-unlocked (braindump/src/import/ToolDocker.cpp, krita/plugins/extensions/imagesize/multilock_button.cc)
music-clef, music-clef-trebble, music-clef-bass, music-clef-alto (SetClefAction.cpp)
chart_circle_normal, chart_ring_normal, chart_scatter_normal, chart_radar_normal, chart_stock_normal, chart_bubble_normal, chart_surface_normal, chart_gantt_normal (ChartConfigWidget.cpp)
edit-merge, edit-duplicate (krita/plugins/extensions/dockers/defaultdockers/kis_layer_box.cpp)
extended_color_selector (krita/plugins/extensions/dockers/advancedcolorselector/kis_color_selector_settings.cpp)
erase-previous-guides, add-horizontal-edges, add-vertical-edges (plugins/defaultTools/guidestool/InsertGuidesToolOptionWidget.cpp)
krita_paintop_icon (krita/ui/widgets/kis_paintop_presets_popup.cpp)
And I wonder why are krita installs so many icons not with the usual naming pattern (hi-*/ox-*), shouldn't that be changed perhaps?
And why are there all the *-calligra icon names? In other places the non-calligra names are used, e.g. the "align-*" names. Some temporary solution that could be removed completely again? Definitely needs the knowledgde which version of the oxygen icons Calligra depends on.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kexi/core/KexiWindow.cpp <span style="color: grey">(f49c967)</span></li>
<li>kexi/formeditor/editlistviewdialog.cpp <span style="color: grey">(f66ceaa)</span></li>
<li>kexi/formeditor/richtextdialog.cpp <span style="color: grey">(80748a5)</span></li>
<li>kexi/main/startup/KexiNewProjectAssistant.cpp <span style="color: grey">(1c52d79)</span></li>
<li>kexi/plugins/reports/kexireportview.cpp <span style="color: grey">(ccdf3f9)</span></li>
<li>kexi/plugins/scripting/kexiscripting/kexiscriptdesignview.cpp <span style="color: grey">(d4d8e2d)</span></li>
<li>kexi/widget/navigator/KexiProjectListView.cpp <span style="color: grey">(3fa561f)</span></li>
<li>kexi/widget/navigator/KexiProjectNavigator.cpp <span style="color: grey">(9c39a1a)</span></li>
<li>kexi/widget/pixmapcollection.cpp <span style="color: grey">(b4f9dfe)</span></li>
<li>plugins/chartshape/ChartConfigWidget.cpp <span style="color: grey">(29af6cb)</span></li>
<li>plugins/chartshape/dialogs/TableEditorDialog.cpp <span style="color: grey">(62acce9)</span></li>
<li>plugins/textshape/TextTool.cpp <span style="color: grey">(161f085)</span></li>
<li>sheets/dialogs/LinkDialog.cpp <span style="color: grey">(4ce4636)</span></li>
<li>sheets/plugins/calendar/CalendarToolWidget.cpp <span style="color: grey">(a7c7814)</span></li>
<li>sheets/shape/SheetsEditor.cpp <span style="color: grey">(5c1ba6b)</span></li>
<li>sheets/ui/CellToolBase.cpp <span style="color: grey">(33f2d29)</span></li>
<li>sheets/ui/CellToolBase_p.cpp <span style="color: grey">(0cba586)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104729/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>