> 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 :) > > C. Boemann wrote: > 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. :) > >
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. - Friedrich W. H. ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104729/#review13027 ----------------------------------------------------------- On May 1, 2012, 4:27 p.m., Friedrich W. H. Kossebau wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104729/ > ----------------------------------------------------------- > > (Updated May 1, 2012, 4:27 p.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 161f085 > 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 > >
_______________________________________________ calligra-devel mailing list calligra-devel@kde.org https://mail.kde.org/mailman/listinfo/calligra-devel
