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



editors/editpage.cpp
<http://git.reviewboard.kde.org/r/102290/#comment6583>

    such a check is necessary indeed .. however, i believe KIO::CopyJob 
provides this internally already. have you tested this?



editors/imageviewer/imageviewer.cpp
<http://git.reviewboard.kde.org/r/102290/#comment6585>

    this is going to cause problems on re-use. QWidget does not let you set a 
layout on a widget if it already has one.
    
    i really, really recommend creating the layout in the consructor, adding a 
QSvgWidget* and a QLabel* member to this class and using those in setImage, 
e.g.:'
    
    if (..svg...) {
         delete m_label;
         if (!m_svgWidget) {
              ... create svg widget  . add it to the layout() .
         }
        m_svgWidget->load(m_image);
    } else {
        delete m_svgWidget;
        if (!m_label) {
    ...etc
    



editors/imageviewer/imageviewer.cpp
<http://git.reviewboard.kde.org/r/102290/#comment6586>

    this will not work as widget is created on the stack, so as soon the if 
statement is done it will be automatically deleted.



editors/imageviewer/imageviewer.cpp
<http://git.reviewboard.kde.org/r/102290/#comment6584>

    no need to check if lable is null, you just set it to null one line above.



mainwindow.cpp
<http://git.reviewboard.kde.org/r/102290/#comment6582>

    delete NULL; is fine, so you don't need to check with an if here. just 
"delete m_imageViewer;" is enough. in fact, i'd simply write:
    
    if (!m_imageViewer) {
        m_imageViewer = new ImageViewer(this);
    }
    
    to prevent unecessary deletion and recreation.


- Aaron J. Seigo


On Oct. 20, 2011, 8:49 a.m., Giorgos Tsiapaliwkas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102290/
> -----------------------------------------------------------
> 
> (Updated Oct. 20, 2011, 8:49 a.m.)
> 
> 
> Review request for Plasma and Aaron J. Seigo.
> 
> 
> Description
> -------
> 
> hello,
> 
> without this patch a user cannot add an image with plasmate.
> 
> reproduce,go to files-images-new,the plasmate will open a text editor instead 
> of a dialog,which(the dialog) is able to open an image.
> 
> With the patch a dialog will open asking for an image.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt f3f32a9 
>   editors/editpage.h 7b5dca3 
>   editors/editpage.cpp d4b0082 
>   editors/imageviewer/imageviewer.h e69de29 
>   editors/imageviewer/imageviewer.cpp e69de29 
>   main.cpp 66a7cd8 
>   mainwindow.h 1b1c2a2 
>   mainwindow.cpp 2fa2742 
>   packagemodel.cpp 8c0907a 
> 
> Diff: http://git.reviewboard.kde.org/r/102290/diff/diff
> 
> 
> Testing
> -------
> 
> the patch is not ready yet,i have noted some questions.
> Also the plasmate tries to open the image with a text editor.This have to be 
> fixed,but how?Should we make plasmate able to preview images?
> 
> In addition,when you add something in the list of files(using the different 
> options provided by the files qdockwidget) it names it as "new".This has to 
> be fixed and the plasmate has to show the real name of the file.(different 
> request,i just want an approval for this patch).
> 
> 
> Thanks,
> 
> Giorgos Tsiapaliwkas
> 
>

_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Reply via email to