Review Request: Wrong icons selected after pasting files and renaming them (because there exists items with it's names)

[David Faure]

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



dolphin/src/views/draganddrophelper.h
<http://git.reviewboard.kde.org/r/107351/#comment18223>

    Why have two methods instead of one? There's no BC requirement here. You could use QString* instead of QString& if you don't want to force the caller to pass an error string -- but on the other hand, error handling is always a good idea...
    
    So I would remove the dropUrls(3 args) and keep the 4 args one only.



lib/konq/konq_operations.h
<http://git.reviewboard.kde.org/r/107351/#comment18224>

    oK. But please add a "TODO KDE 5.0" about merging the two methods (so we can get rid of the V2 naming in the long run).



lib/konq/konq_operations.h
<http://git.reviewboard.kde.org/r/107351/#comment18226>

    Please document what this signal really means. It's not fully clear from the name, for instance, that this is emitted for both pasting and dropping. Maybe "urlCreated"? Although one would then expect it to be emitted when doing New / Text Document etc.
    
    In fact this is pretty much the same as the aboutToCreate signal... [which is currently disabled, but could be re-enabled].
    What I don't like very much about urlParsed(KUrl) is that it notifies about one url at a time, even when pasting/dropping 1000 items. It sounds like it might create a performance problem, depending on what the slot does. The aboutToCreate signal has the benefit of sending the information for *all* the files in one go, which allows to handle all of it in a single pass in the slot. I think this would be better.
    
    I'll let you look into whether to actually use aboutToCreate (it might be a bit cumbersome for some cases like PUT?), or to create a new signal with a KUrl::List --- or to prove me wrong, if it's not doable.
    



lib/konq/konq_operations.cpp
<http://git.reviewboard.kde.org/r/107351/#comment18225>

    I know it's pretty inconsistent right now, but let's go for the kdelibs coding style in new code ('{' at end of line, no space, etc.)



lib/konq/konq_operations.cpp
<http://git.reviewboard.kde.org/r/107351/#comment18227>

    Hmm, this slotResult is used by many different operations, not all of which are about pasting/dropping.
    I can't find an actual case where a simplejob is used but still, this seems dangerous (= we could emit "pasted" when in fact a file was just deleted).
    
    Ah, a simple solution would be to check that m_method is equal to PUT.


- David Faure


On Dec. 13, 2012, 11:42 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107351/
> -----------------------------------------------------------
> 
> (Updated Dec. 13, 2012, 11:42 p.m.)
> 
> 
> Review request for Dolphin, David Faure and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> Bug 233335 - Wrong icons selected after pasting files and renaming them (because there exists items with it's names)
> 
> Dolphin 2.1 selects the existing items instead of the newly pasted + renamed items.
> 
> 
> This addresses bug 233335.
>     http://bugs.kde.org/show_bug.cgi?id=233335
> 
> 
> Diffs
> -----
> 
>   dolphin/src/dolphinmainwindow.cpp 11e03d0 
>   dolphin/src/dolphinviewcontainer.cpp c27550a 
>   dolphin/src/panels/folders/folderspanel.cpp 6e3a767 
>   dolphin/src/panels/places/placespanel.cpp 61c15a7 
>   dolphin/src/views/dolphinview.h a2fe9f6 
>   dolphin/src/views/dolphinview.cpp 941083f 
>   dolphin/src/views/draganddrophelper.h ac16f7c 
>   dolphin/src/views/draganddrophelper.cpp f81d4d0 
>   lib/konq/konq_operations.h b82feb4 
>   lib/konq/konq_operations.cpp e0f1ade 
> 
> Diff: http://git.reviewboard.kde.org/r/107351/diff/
> 
> 
> Testing
> -------
> 
> 1. Open a test folder with Dolphin.
> 2. Create the test files:
>     2.1. Open Dolphin’s terminal (F4).
>     2.2. Run “mkdir folder ; touch a b folder/b c d”.
>     2.3. Close Dolphin’s terminal (F4).
> 3. In the current folder, select the four empty files (a, b, c, d) and copy them (Ctrl+C).
> 4. Enter the “folder” folder.
> 5. Paste the files (Ctrl+V).
>     5.1. When asked for a new name for “b”, enter “b2” and hit Enter to continue.
> 
> Result:
> Items “a”, “b2”, “c”, “d” are selected
> 
> Btw. Thanks to Adrián Chaves Fernández for the easy way to test this bug ;)
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20121223/6f9e2e2f/attachment.htm>