Review Request: Add recentdocuments:/ kio slave to kde-runtime.

[David Faure]

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



kioslave/recentdocuments/recentdocuments.cpp
<http://git.reviewboard.kde.org/r/103849/#comment8659>

    This local variable hides the "url" argument to the method, making this code confusing to read.
    Also, it's not needed here outside of the if, so I would recommend changing line 93 to
    
    KUrl desktopFileUrll(entry);
    
    But even then, it's only used at line 100, right? So this should really become
    
    else
       urlToStat = KUrl(entry);
    
    Not that I see much point in stat'ing the desktop file anyway... You'll replace the name and display name and url ... so basically a remote URL will appear to be a file (could be a bug, if it was a directory) of size "a few bytes" (the size of the .desktop file) ...
    I guess you didn't want to stat remote files because this might ask for passwords, or trigger connecting to the internet at an unwanted time, or might be too slow?
    I guess all of this is valid, and in that case the proper long-term solution would be to write out more information about recent urls in the file...
    OK. But my point is, the size of the .desktop file is wrong, as "size of the remote file", so better not set any size (== unknown).
    => better not stat the desktop file, ever.
    
    Just have two code paths:
    
      if urlInside.isLocalFile {
         stat(urlInside)
         and fix name, displayname, set localpath.
      } else {
         fill in a UDS entry from scratch with the little we know about the remote url
       }
        set UDS_TARGET_URL
        udslist << uds;
    



kioslave/recentdocuments/recentdocuments.cpp
<http://git.reviewboard.kde.org/r/103849/#comment8658>

    State? I guess you meant Stat.



kioslave/recentdocuments/recentdocuments.cpp
<http://git.reviewboard.kde.org/r/103849/#comment8662>

    How about testing urlInside.isEmpty() together with the "protocol==recentdocuments" a few lines up, to use "continue" for both?
    
    All this unnecessary nesting of if()s hurts readability a bit, creating multiple code paths.



kioslave/recentdocuments/recentdocuments.cpp
<http://git.reviewboard.kde.org/r/103849/#comment8661>

    This if() is unnecessary, KIO::stat will never return a NULL job.



kioslave/recentdocuments/recentdocuments.cpp
<http://git.reviewboard.kde.org/r/103849/#comment8660>

    if (uds.count()) is usually written as if (!uds.isEmpty()) in Qt code, more readable.
    
    But it can only be empty if the job failed, so maybe it would be simpler to "continue" after failure, and thus remove if()s.
    



kioslave/recentdocuments/recentdocuments.cpp
<http://git.reviewboard.kde.org/r/103849/#comment8657>

    This should be i18n("Recent Documents")  ('s' missing).
    Use a local QString variable to avoid duplicating the string in the code.



kioslave/recentdocuments/recentdocumentsnotifier.cpp
<http://git.reviewboard.kde.org/r/103849/#comment8655>

    Double connect, this was already done on line 24.



kioslave/recentdocuments/recentdocumentsnotifier.cpp
<http://git.reviewboard.kde.org/r/103849/#comment8656>

    Not sure what there is to clean in "recentdocuments:/filename" :-)
    
    I think this line can be removed.


- David Faure


On Feb. 5, 2012, 7:24 p.m., Xuetian Weng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103849/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2012, 7:24 p.m.)
> 
> 
> Review request for KDE Runtime.
> 
> 
> Description
> -------
> 
> Add recentdocuments:/ kio slave to kde-runtime.
> Did some little rename "recentdocument -> recentdocuments", based on http://kde-apps.org/content/show.php?content=145878
> 
> 
> Diffs
> -----
> 
>   kioslave/CMakeLists.txt f3d5b00 
>   kioslave/recentdocuments/CMakeLists.txt PRE-CREATION 
>   kioslave/recentdocuments/Messages.sh PRE-CREATION 
>   kioslave/recentdocuments/recentdocuments.h PRE-CREATION 
>   kioslave/recentdocuments/recentdocuments.cpp PRE-CREATION 
>   kioslave/recentdocuments/recentdocuments.protocol PRE-CREATION 
>   kioslave/recentdocuments/recentdocumentsnotifier.h PRE-CREATION 
>   kioslave/recentdocuments/recentdocumentsnotifier.cpp PRE-CREATION 
>   kioslave/recentdocuments/recentdocumentsnotifier.desktop PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/103849/diff/
> 
> 
> Testing
> -------
> 
> Works for me.
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20120213/5565ff46/attachment.htm>