<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 />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This review has been submitted with commit 5115be3c34c05d54028fa75c85d3905480e8584a by Friedrich W. H. Kossebau to branch master.</pre>
<br />
<p>- Commit</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>