Review Request: Fixes: misspelt icon names, improved icon choices, out-dated/missing icons

C. Boemann cbr at boemann.dk
Tue May 1 16:35:00 BST 2012 


> On April 28, 2012, 1:15 p.m., C. Boemann wrote:
> > plugins/textshape/TextTool.cpp, line 419
> > <http://git.reviewboard.kde.org/r/104729/diff/1/?file=58555#file58555line419>
> >
> >     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
> 
> Inge Wallin wrote:
>     I think the time has come to up this requirement.  How about KDE 4.5 for Calligra 2.5? I want this patch in.
> 
> Friedrich W. H. Kossebau wrote:
>     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 :)

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. :)


- C.


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104729/#review13027
-----------------------------------------------------------


On April 26, 2012, 12:16 a.m., Friedrich W. H. Kossebau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104729/
> -----------------------------------------------------------
> 
> (Updated April 26, 2012, 12:16 a.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   kexi/core/KexiWindow.cpp f49c967 
>   kexi/formeditor/editlistviewdialog.cpp f66ceaa 
>   kexi/formeditor/richtextdialog.cpp 80748a5 
>   kexi/main/startup/KexiNewProjectAssistant.cpp 1c52d79 
>   kexi/plugins/reports/kexireportview.cpp ccdf3f9 
>   kexi/plugins/scripting/kexiscripting/kexiscriptdesignview.cpp d4d8e2d 
>   kexi/widget/navigator/KexiProjectListView.cpp 3fa561f 
>   kexi/widget/navigator/KexiProjectNavigator.cpp 9c39a1a 
>   kexi/widget/pixmapcollection.cpp b4f9dfe 
>   plugins/chartshape/ChartConfigWidget.cpp 29af6cb 
>   plugins/chartshape/dialogs/TableEditorDialog.cpp 62acce9 
>   plugins/textshape/TextTool.cpp 1dc2e02 
>   sheets/dialogs/LinkDialog.cpp 4ce4636 
>   sheets/plugins/calendar/CalendarToolWidget.cpp a7c7814 
>   sheets/shape/SheetsEditor.cpp 5c1ba6b 
>   sheets/ui/CellToolBase.cpp 33f2d29 
>   sheets/ui/CellToolBase_p.cpp 0cba586 
> 
> Diff: http://git.reviewboard.kde.org/r/104729/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Friedrich W. H. Kossebau
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20120501/36ed1ac7/attachment.htm>

More information about the calligra-devel mailing list