Review Request: Support drag and drop file transfer from text-ui

[David Edmundson]

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


Very impressive. You're a non-stop coding machine atm.

One major major comment, which if I've read it right you'll kick yourself over (number 3).

I would like DrDanz (resident file transfer expert) to review this too.

I'm not sure if we want some confirmation before starting sending the file transfer, I don't want to accidentally start doing things the user isn't expecting if they miss. Maybe it could popup a menu (like dolphin at the end of a drag) "send link", "send as file" (and for remote files, it always just pastes a link).

Long term: Consider what can, and what should be library code for drags elsewhere.


lib/chat-widget.cpp
<http://git.reviewboard.kde.org/r/105511/#comment12177>

    I'm not 100% sure why you need this.
    
    Surely the drop will propogate to the parent (this) anyway, and we can catch it in our own event() method which we already have?
    
    I may be wrong, ignore me if I am.
    



lib/chat-widget.cpp
<http://git.reviewboard.kde.org/r/105511/#comment12176>

    generally use qobject_cast for QObjects 
    
    and add a Q_ASSERT afterwards, so we (as developers) find out instantly where it's going wrong if some idiot (me) in the future accidentally connects something else to the wrong slot.
    
    



lib/chat-widget.cpp
<http://git.reviewboard.kde.org/r/105511/#comment12174>

    WHOA!!!!!!!!!!!!!!!!!!!
    
    I'm pretty sure you just deleted any file we sent regardless of whether it was temp or not!
    
    So if I dragged a local file from my home directory to when the transfer ends you delete it!
    
    



lib/chat-widget.cpp
<http://git.reviewboard.kde.org/r/105511/#comment12175>

    filename isn't unique enough, use the URL as you have it.
    
    
    OR
    
    simply QList<Tp::ChannelPtr> ?
    



lib/chat-widget.cpp
<http://git.reviewboard.kde.org/r/105511/#comment12178>

    I think (though could be wrong) this won't work very well.
    
    A good web browser will fill in both the text and HTML part of the mimedata and thus never trigger this.
    
    If it did.. the message you send will be full of HTML code, which would look pretty bad to the user the other side.
    
    (it wouldn't render like HTML they'd see all the "<h1>" type tags.)
    
    *shrug* after merging we can experiment like crazy and find out what doesn't work.



lib/chat-widget.cpp
<http://git.reviewboard.kde.org/r/105511/#comment12171>

    braces {}



lib/chat-widget.cpp
<http://git.reviewboard.kde.org/r/105511/#comment12173>

    ...is that true? What if it's a mimetype we can't decode?


- David Edmundson


On July 10, 2012, 9:32 p.m., Dan Vratil wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105511/
> -----------------------------------------------------------
> 
> (Updated July 10, 2012, 9:32 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> - dropping a local URL starts a file transfer
> - dropping a remote URL appends it to the SendMessageBox
> - dropping text or HTML data appends it to the SendMessageBox
> - dropping image data creates a temporary file (PNG), starts a file transfer and when it's finished the file is removed again
> 
> 
> This addresses bug 288561.
>     http://bugs.kde.org/show_bug.cgi?id=288561
> 
> 
> Diffs
> -----
> 
>   app/main.cpp b8b2523 
>   lib/chat-text-edit.h 3e2dec0 
>   lib/chat-text-edit.cpp 2985aee 
>   lib/chat-widget.h 686b632 
>   lib/chat-widget.cpp e3439d8 
> 
> Diff: http://git.reviewboard.kde.org/r/105511/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dan Vratil
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20120710/73781837/attachment-0001.html>