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

Lukáš Tvrdý lukast.dev at gmail.com
Sat Mar 9 10:32:33 GMT 2013 

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



CMakeLists.txt
<http://git.reviewboard.kde.org/r/109358/#comment21540>

    Why this changes?



data/CMakeLists.txt
<http://git.reviewboard.kde.org/r/109358/#comment21541>

    This is intended?



data/shaders/gl2.vert
<http://git.reviewboard.kde.org/r/109358/#comment21542>

    Can you add some comments what these shaders are supposed to do?



image/CMakeLists.txt
<http://git.reviewboard.kde.org/r/109358/#comment21543>

    Why is memory leak tracker removed?



image/kis_default_bounds.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21544>

    I would remove this commented parted



image/kis_shared_ptr.h
<http://git.reviewboard.kde.org/r/109358/#comment21545>

    Again, why are you removing MemoryLeakTracker?



image/recorder/kis_macro_player.cc
<http://git.reviewboard.kde.org/r/109358/#comment21546>

    The string is confusing, Replaying macro?



plugins/assistants/RulerAssistant/kis_ruler_assistant_tool.cc
<http://git.reviewboard.kde.org/r/109358/#comment21547>

    Can you explain this ifdefing coding related to QT_NO_DBUS?
    
    Why it is needed to have all this blocks ifdefed?



plugins/extensions/bigbrother/bigbrother.cc
<http://git.reviewboard.kde.org/r/109358/#comment21548>

    Replaying macro?



plugins/extensions/share/dlg_login.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21549>

    I would emove qDebug and use kdebug instead?



plugins/extensions/share/dlg_login.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21550>

    qDebug again



plugins/extensions/share/dlg_login.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21551>

    qDebug



plugins/extensions/share/imageshare.h
<http://git.reviewboard.kde.org/r/109358/#comment21552>

    Fix copyright, it's probably 2012-2013



plugins/extensions/share/imageshare.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21553>

    Do you have the response from dA?



plugins/extensions/share/imageshare.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21554>

    Isn't this a little dangerous to provide?
    I would suggest to make this configurable by remote external entity. I remember Digikam had problems with their Facebook API key misused by spam applications and then all content with given API client secret was deleted.
    
    It's complicated issue without good solution, but let's be aware of this.



plugins/extensions/share/imageshare.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21555>

    Can you remove all qDebug occuriences or replace them with kdebug?



plugins/extensions/share/imageshare.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21556>

    Is the call fixed by dA already?



plugins/extensions/share/imageshare.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21557>

    No folder?



plugins/extensions/share/o2.h
<http://git.reviewboard.kde.org/r/109358/#comment21558>

    Is this licence compatible with our code-standards?



plugins/extensions/share/o2.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21559>

    qDebug should be avoided if possible



plugins/extensions/share/o2.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21560>

    qWarning -> kWarning 



plugins/extensions/share/o2deviantart.h
<http://git.reviewboard.kde.org/r/109358/#comment21561>

    Can be this code found in some library?



plugins/extensions/share/stash.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21562>

    Can this TODO be fixed now?



plugins/extensions/share/stash.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21563>

    Is it still derp?



plugins/extensions/share/stash.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21564>

    Maybe fix this TODO too.



plugins/extensions/share/stash.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21565>

    qDebug -> kDebug



plugins/extensions/share/submitdlg.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21566>

    Remove this line or add one :)



plugins/formats/CMakeLists.txt
<http://git.reviewboard.kde.org/r/109358/#comment21567>

    Are you removing JPEG support?



plugins/formats/jpeg/CMakeLists.txt
<http://git.reviewboard.kde.org/r/109358/#comment21568>

    Is this intended?



plugins/formats/jpeg/CMakeLists.txt
<http://git.reviewboard.kde.org/r/109358/#comment21570>

    Why?



plugins/formats/jpeg/CMakeLists.txt
<http://git.reviewboard.kde.org/r/109358/#comment21569>

    also this is inteded change?



plugins/tools/defaulttools/kis_tool_movetooloptionswidget.h
<http://git.reviewboard.kde.org/r/109358/#comment21571>

    Remove or add some line here



plugins/tools/defaulttools/kis_tool_movetooloptionswidget.cpp
<http://git.reviewboard.kde.org/r/109358/#comment21572>

    also here



plugins/tools/selectiontools/kis_tool_select_elliptical.cc
<http://git.reviewboard.kde.org/r/109358/#comment21573>

    Is this race condition real?



plugins/tools/tool_crop/kistoolcropconfigwidget.h
<http://git.reviewboard.kde.org/r/109358/#comment21574>

    fixme



plugins/tools/tool_transform2/CMakeLists.txt
<http://git.reviewboard.kde.org/r/109358/#comment21575>

    Do we want to link declarative library to tool?



plugins/tools/tool_transform2/kis_tool_transform.cc
<http://git.reviewboard.kde.org/r/109358/#comment21576>

    Can you fix this code before merge?



plugins/tools/tool_transform2/tool_transform.cc
<http://git.reviewboard.kde.org/r/109358/#comment21577>

    Do we want to mix QML and tools code? I don't find it correct


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

- Lukáš Tvrdý


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/c5393168/attachment.htm>

More information about the calligra-devel mailing list