Review Request 109358: Krita Sketch branch, general fixes (1/3 for Krita itself)

Boudewijn Rempt boud at valdyas.org
Sat Mar 9 11:59:36 GMT 2013 


> On March 9, 2013, 10:32 a.m., Lukáš Tvrdý wrote:
> > CMakeLists.txt, line 134
> > <http://git.reviewboard.kde.org/r/109358/diff/1/?file=117957#file117957line134>
> >
> >     Why this changes?

Fixed


> On March 9, 2013, 10:32 a.m., Lukáš Tvrdý wrote:
> > image/kis_default_bounds.cpp, line 87
> > <http://git.reviewboard.kde.org/r/109358/diff/1/?file=117968#file117968line87>
> >
> >     I would remove this commented parted

Hm, actually, it was Dmitry who added that stuff. I think it only got moved around.


> On March 9, 2013, 10:32 a.m., Lukáš Tvrdý wrote:
> > plugins/assistants/RulerAssistant/kis_ruler_assistant_tool.cc, line 32
> > <http://git.reviewboard.kde.org/r/109358/diff/1/?file=117983#file117983line32>
> >
> >     Can you explain this ifdefing coding related to QT_NO_DBUS?
> >     
> >     Why it is needed to have all this blocks ifdefed?

That's because kio needs dbus, and we want to be able to compile sketch (and krita) without dbus on Windows and OSX.


> On March 9, 2013, 10:32 a.m., Lukáš Tvrdý wrote:
> > plugins/extensions/bigbrother/bigbrother.cc, line 129
> > <http://git.reviewboard.kde.org/r/109358/diff/1/?file=117985#file117985line129>
> >
> >     Replaying macro?

Well, that's what it is:

     dbgPlugins << "Play the macro";
     KoProgressUpdater* updater = m_view->createProgressUpdater();
-    updater->start(1);
+    updater->start(1, i18n("Playing back macro"));

Basically, we need to have this text to show what's happening when doing progress. So, following the comments in the code, text was added.


> On March 9, 2013, 10:32 a.m., Lukáš Tvrdý wrote:
> > plugins/extensions/share/dlg_login.cpp, line 26
> > <http://git.reviewboard.kde.org/r/109358/diff/1/?file=117989#file117989line26>
> >
> >     I would emove qDebug and use kdebug instead?

fixed.


> On March 9, 2013, 10:32 a.m., Lukáš Tvrdý wrote:
> > plugins/extensions/share/imageshare.cpp, line 64
> > <http://git.reviewboard.kde.org/r/109358/diff/1/?file=117992#file117992line64>
> >
> >     Do you have the response from dA?

Dan -- that's really for you to answer.


> On March 9, 2013, 10:32 a.m., Lukáš Tvrdý wrote:
> > plugins/extensions/share/imageshare.cpp, line 141
> > <http://git.reviewboard.kde.org/r/109358/diff/1/?file=117992#file117992line141>
> >
> >     Can you remove all qDebug occuriences or replace them with kdebug?

Done.


> On March 9, 2013, 10:32 a.m., Lukáš Tvrdý wrote:
> > plugins/extensions/share/o2deviantart.h, line 1
> > <http://git.reviewboard.kde.org/r/109358/diff/1/?file=117997#file117997line1>
> >
> >     Can be this code found in some library?

It's basic BSD license, so a copy should be fine. It is not in any library used.


> On March 9, 2013, 10:32 a.m., Lukáš Tvrdý wrote:
> > plugins/formats/CMakeLists.txt, line 16
> > <http://git.reviewboard.kde.org/r/109358/diff/1/?file=118013#file118013line16>
> >
> >     Are you removing JPEG support?

No, this is just a duplicated find_package -- we have another earlier on.


> On March 9, 2013, 10:32 a.m., Lukáš Tvrdý wrote:
> > plugins/formats/jpeg/CMakeLists.txt, line 9
> > <http://git.reviewboard.kde.org/r/109358/diff/1/?file=118014#file118014line9>
> >
> >     Is this intended?

Yes, jpeg support was moved to krita/ui, parallel to krita/png


> On March 9, 2013, 10:32 a.m., Lukáš Tvrdý wrote:
> > plugins/formats/jpeg/CMakeLists.txt, line 43
> > <http://git.reviewboard.kde.org/r/109358/diff/1/?file=118014#file118014line43>
> >
> >     also this is inteded change?

Yes.


> On March 9, 2013, 10:32 a.m., Lukáš Tvrdý wrote:
> > plugins/tools/defaulttools/kis_tool_movetooloptionswidget.h, line 2
> > <http://git.reviewboard.kde.org/r/109358/diff/1/?file=118028#file118028line2>
> >
> >     Remove or add some line here

Fixed.


> On March 9, 2013, 10:32 a.m., Lukáš Tvrdý wrote:
> > plugins/tools/selectiontools/kis_tool_select_elliptical.cc, line 67
> > <http://git.reviewboard.kde.org/r/109358/diff/1/?file=118030#file118030line67>
> >
> >     Is this race condition real?

yes, definitely.


> On March 9, 2013, 10:32 a.m., Lukáš Tvrdý wrote:
> > plugins/tools/tool_transform2/tool_transform.cc, line 51
> > <http://git.reviewboard.kde.org/r/109358/diff/1/?file=118044#file118044line51>
> >
> >     Do we want to mix QML and tools code? I don't find it correct

Arjen will look at that, as well as the duplicated getters/setters.


On March 9, 2013, 10:32 a.m., Dan Leinir Turthra Jensen wrote:
> > I made first round of brief review. I did not test the code.
> > I don't see any obvious regression from the functionality point of view.
> > I don't understand why JPEG and MemoryLeakTracer is removed in this patch.
> > 
> > OAuth2 dA code should be probably external dependency if it is possible,
> > the licence is confusing for me too, is it ok for inclusion?
> > 
> > General advice:
> > o avoid to use qDebug()
> > o fix TODOs/FIXME before merging to master

We probably need to:

a) skip the deviant art integration
b) make one big patch

I've been cleaning up the patch already a bit, but need to do that in the branch now.


- Boudewijn


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


On March 8, 2013, 3:21 p.m., Dan Leinir Turthra Jensen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109358/
> -----------------------------------------------------------
> 
> (Updated March 8, 2013, 3:21 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> The first of a series of batches of changes from the Krita Sketch branch. This is everything inside krita, except for sketch and the ui directory.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt f8943b7 
>   config-qjson.h.cmake PRE-CREATION 
>   data/CMakeLists.txt c621b5f 
>   data/paintoppresets/Pencil_HB.kpp 0855da8 
>   data/shaders/CMakeLists.txt 3b40741 
>   data/shaders/gl2.frag PRE-CREATION 
>   data/shaders/gl2.vert PRE-CREATION 
>   image/CMakeLists.txt 0d4b2f8 
>   image/commands/kis_image_command.h e316bf9 
>   image/commands/kis_set_global_selection_command.cpp d806cd6 
>   image/kis_default_bounds.h d51124b 
>   image/kis_default_bounds.cpp 5e61ad1 
>   image/kis_filter_weights_applicator.h 7689de0 
>   image/kis_image.h 1b70680 
>   image/kis_image.cc 73c0cfb 
>   image/kis_processing_visitor.cpp 5761544 
>   image/kis_shared_ptr.h 85aee50 
>   image/recorder/kis_macro_player.cc 5cc848f 
>   image/recorder/kis_recorded_node_action.cc 455c82e 
>   image/tests/kis_filter_weights_buffer_test.cpp 6e2401a 
>   image/tests/kis_fixed_point_maths_test.cpp ef1f312 
>   main.cc a40958f 
>   pics/CMakeLists.txt d7ce005 
>   pics/icon-kritasketch-256.png PRE-CREATION 
>   pics/icon-kritasketch.svg PRE-CREATION 
>   plugins/assistants/RulerAssistant/kis_ruler_assistant_tool.h e3bee14 
>   plugins/assistants/RulerAssistant/kis_ruler_assistant_tool.cc 34b8a71 
>   plugins/extensions/CMakeLists.txt 4084890 
>   plugins/extensions/bigbrother/bigbrother.cc f59e55e 
>   plugins/extensions/dropshadow/kis_dropshadow_plugin.cc 0722c3e 
>   plugins/extensions/share/CMakeLists.txt PRE-CREATION 
>   plugins/extensions/share/dlg_login.h PRE-CREATION 
>   plugins/extensions/share/dlg_login.cpp PRE-CREATION 
>   plugins/extensions/share/dlg_login.ui PRE-CREATION 
>   plugins/extensions/share/imageshare.h PRE-CREATION 
>   plugins/extensions/share/imageshare.cpp PRE-CREATION 
>   plugins/extensions/share/imageshare.rc PRE-CREATION 
>   plugins/extensions/share/kritaimageshare.desktop PRE-CREATION 
>   plugins/extensions/share/o2.h PRE-CREATION 
>   plugins/extensions/share/o2.cpp PRE-CREATION 
>   plugins/extensions/share/o2deviantart.h PRE-CREATION 
>   plugins/extensions/share/o2deviantart.cpp PRE-CREATION 
>   plugins/extensions/share/o2reply.h PRE-CREATION 
>   plugins/extensions/share/o2reply.cpp PRE-CREATION 
>   plugins/extensions/share/o2replyserver.h PRE-CREATION 
>   plugins/extensions/share/o2replyserver.cpp PRE-CREATION 
>   plugins/extensions/share/o2requestor.h PRE-CREATION 
>   plugins/extensions/share/o2requestor.cpp PRE-CREATION 
>   plugins/extensions/share/simplecrypt.h PRE-CREATION 
>   plugins/extensions/share/simplecrypt.cpp PRE-CREATION 
>   plugins/extensions/share/stash.h PRE-CREATION 
>   plugins/extensions/share/stash.cpp PRE-CREATION 
>   plugins/extensions/share/submit.ui PRE-CREATION 
>   plugins/extensions/share/submitdlg.h PRE-CREATION 
>   plugins/extensions/share/submitdlg.cpp PRE-CREATION 
>   plugins/filters/unsharp/kis_unsharp_filter.cpp 316c3d0 
>   plugins/formats/CMakeLists.txt 4b9090d 
>   plugins/formats/jpeg/CMakeLists.txt 6bee059 
>   plugins/formats/jpeg/iccjpeg.h a3747b8 
>   plugins/formats/jpeg/iccjpeg.c 1f6c4b1 
>   plugins/formats/jpeg/kis_jpeg_converter.h e080d23 
>   plugins/formats/jpeg/kis_jpeg_converter.cc 33ec941 
>   plugins/formats/jpeg/kis_jpeg_destination.h d575158 
>   plugins/formats/jpeg/kis_jpeg_destination.cpp 4bde42b 
>   plugins/formats/jpeg/kis_jpeg_source.h acddee0 
>   plugins/formats/jpeg/kis_jpeg_source.cpp ae89209 
>   plugins/tools/defaulttools/CMakeLists.txt 0840796 
>   plugins/tools/defaulttools/kis_tool_brush.h dbf5c37 
>   plugins/tools/defaulttools/kis_tool_brush.cc fad20b5 
>   plugins/tools/defaulttools/kis_tool_move.h 362add0 
>   plugins/tools/defaulttools/kis_tool_move.cc 2cf9890 
>   plugins/tools/defaulttools/kis_tool_movetooloptionswidget.h PRE-CREATION 
>   plugins/tools/defaulttools/kis_tool_movetooloptionswidget.cpp PRE-CREATION 
>   plugins/tools/selectiontools/kis_tool_select_elliptical.cc 4c071a2 
>   plugins/tools/selectiontools/kis_tool_select_polygonal.h cdfc0f6 
>   plugins/tools/selectiontools/kis_tool_select_polygonal.cc fb97f3d 
>   plugins/tools/selectiontools/kis_tool_select_rectangular.h b826bba 
>   plugins/tools/selectiontools/kis_tool_select_rectangular.cc 97854963 
>   plugins/tools/tool_crop/CMakeLists.txt e84978d 
>   plugins/tools/tool_crop/kis_tool_crop.h dfce084 
>   plugins/tools/tool_crop/kis_tool_crop.cc 65c9458 
>   plugins/tools/tool_crop/kistoolcropconfigwidget.h PRE-CREATION 
>   plugins/tools/tool_crop/kistoolcropconfigwidget.cpp PRE-CREATION 
>   plugins/tools/tool_transform2/CMakeLists.txt 7a6c19b 
>   plugins/tools/tool_transform2/kis_tool_transform.h 4ce6c6a 
>   plugins/tools/tool_transform2/kis_tool_transform.cc e7c4f52 
>   plugins/tools/tool_transform2/kis_tool_transform_config_widget.h 5b7f932 
>   plugins/tools/tool_transform2/tool_transform.cc 9372cfd 
>   plugins/tools/tool_transform2/tool_transform_args.h 8e5befd 
> 
> Diff: http://git.reviewboard.kde.org/r/109358/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dan Leinir Turthra Jensen
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20130309/20c4fe52/attachment.htm>

More information about the calligra-devel mailing list