955476 - File Transfer Backend

Status: NEW

(Chat Core :: General, defect)

Description

[Atul Jangra [:atuljangra]]

*** Original post on bio 2039 at 2013-07-12 01:51:00 UTC ***

As we decided to have xmpp/non-xmpp code differently. This is the bug that handles the non-xmpp part of the file transfer implementation

Comment 1

[Atul Jangra [:atuljangra]]

Attachment: Backend for ft

*** Original post on bio 2039 as attmnt 2578 at 2013-07-12 02:37:00 UTC ***

The interface prplIInterface contains many things which are not used yet. But they were designed with the aim of using them in future.
Also, I've removed the changes in status messages during file transfer. I think we need a minimal UI and this should be th minimalistic version.

Comment 2

[Atul Jangra [:atuljangra]]

*** Original post on bio 2039 at 2013-07-12 02:38:33 UTC ***

Assigning to myself.

Comment 3

[Patrick Cloke [:clokep]]

Comment on attachment 8354346 [details] [diff] [review]
Backend for ft

*** Original change on bio 2039 attmnt 2578 at 2013-07-12 11:58:32 UTC ***

>diff --git a/chat/components/public/prplIFileTransfer.idl b/chat/components/public/prplIFileTransfer.idl
>+interface prplIFileTransfer : nsIDownload {
I haven't looked at this closely.

>+interface ibFileTransferOffer: nsISupports {
>+  /* Name of the file corresponding to this transfer. */
>+  readonly attribute AUTF8String sender;
This comment seems wrong.

>diff --git a/chat/modules/jsProtoHelper.jsm b/chat/modules/jsProtoHelper.jsm
>@@ -457,16 +463,20 @@ const GenericConvIMPrototype = {
> 
>     if (aState == Ci.prplIConvIM.NOT_TYPING)
>       delete this.typingState;
>     else
>       this.typingState = aState;
>     this.notifyObservers(null, "update-typing", null);
>   },
> 
>+  fileReceived: function(aSubject) {
>+    this.notifyObservers(aSubject, "file-transfer-offer", null);
>+  },
Should this be defined in the interface?

>+  
Nit: Trailing whitespace, most editors can remove this automatically.

>diff --git a/instantbird/content/conversation.xml b/instantbird/content/conversation.xml
I have not looked over UI changes.

>diff --git a/purple/purplexpcom/src/purpleConvIM.h b/purple/purplexpcom/src/purpleConvIM.h
>@@ -50,17 +50,20 @@ public:
>-
>+  NS_IMETHODIMP SendFile(nsIURI * aFileURI) {
>+    return purpleConversation::SendFile(aFileURI);
>+  }
>+  
Nit: Whitespace.

Comment 4

[aleth [:aleth]]

Comment on attachment 8354346 [details] [diff] [review]
Backend for ft

*** Original change on bio 2039 attmnt 2578 at 2013-07-12 13:18:56 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 5

[aleth [:aleth]]

*** Original post on bio 2039 at 2013-07-12 13:18:56 UTC ***

Comment on attachment 8354346 [details] [diff] [review] (bio-attmnt 2578)
Backend for ft

Some general issues:

1) If you attach file transfers to open conversations the way you are doing, what happens e.g. if the user closes the conversation tab while the file transfer is running? What's the expected behaviour?

2) The prplIFileTransfer idl file needs an initial comment describing how exactly the file transfer interface is supposed to be used. (What calls what when etc).

3) Have you thought about what happens when a file transfer fails? If we are leaving this for later, do you have something in mind for how it will be handled? Shouldn't we, for now, pick up the corresponding notification and show an error system message?
Does FileLink get triggered from conversation.xml or is there going to be an intermediate backend layer? How does this interact with your answer to 1)?

>+interface prplIFileTransfer : nsIDownload {
>+  /**
>+   * generic file transfer.
>+   */
>+  const short FILE_TRANSFER_DOWNLOAD = 0;

I don't understand this comment.

>+  /**
>+   * Transfer completed.  (completed)
>+   */
>+  const short FILE_TRANSFER_FINISHED = 1;
>+
>+  /**
>+   * Transfer failed due to error. (completed)
>+   */
>+  const short FILE_TRANSFER_FAILED = 2;
>+
>+  /**
>+   * Transfer was canceled by the receiver. (completed)
>+   */
>+  const short FILE_TRANSFER_CANCELED_RECEIVER = 3;
>+
>+  /**
>+   * Transfer was canceled by the sender. (completed)
>+   */
>+  const short FILE_TRANSFER_CANCELED_SENDER = 4;

I think the extra (completed) here is confusing/not needed.

>+  /**
>+  * Transfer was paused by the user.
>+   */
>+  const short FILE_TRANSFER_ACKED = 7;

This comment seems wrong.

>+   * Whether the file tranfer associated is upload or download.

Typo

>+  /**
>+  * This attribute is for getting the current status (from the above mentioned constants)
>+   */

"This attribute is for getting" is not needed, it's obviously an attribute.

>+  /* The conversation used for this transfer */
>+  readonly attribute prplIConversation conv;

Why do we need to keep this reference? When reporting back to the UI, I think you use Services.obs.notifyObservers?

>+  /**
>+   * Restart the current file tranfer.
>+   *
>+   * @throws NS_ERROR_UNEXPECTED if it cannot be restarted or if it is
>+   * already in progress
>+   */
>+  void restart();

What is the use case for this?

>+  /**
>+   * Returns the current state of the filetransfer going on
>+   */
>+  short getCurrentStatus();

Drop "going on"

>+
>+  /**
>+   * Sets the current state of the filetransfer going on
>+   */

ditto

>+  /**
>+   * Request the sender to re-send the file.
>+   *
>+   * @throws NS_ERROR_UNEXPECTED if the current user is sending the file.
>+   */
>+  void requestRestart();

What's the difference to restart() above?


>diff --git a/instantbird/content/conversation.xml b/instantbird/content/conversation.xml
>--- a/instantbird/content/conversation.xml
>+++ b/instantbird/content/conversation.xml
>@@ -45,17 +45,18 @@
>                          tooltip="buddyTooltip"
>                          onclick="onNickClick(event);"
>                          onkeypress="onNicklistKeyPress(event);"/>
>           </xul:vbox>
>         </xul:hbox>
>         <xul:splitter class="splitter" anonid="splitter-bottom"/>
>         <xul:vbox anonid="conv-bottom" class="conv-bottom">
>           <xul:textbox anonid="inputBox" class="conv-textbox" multiline="true" flex="1"
>-                       onclick = "delete document.getBindingParent(this)._completions;"/>
>+                       onclick = "delete document.getBindingParent(this)._completions;"
>+                       ondrop="onDrop(event);"/>

Please add an ondrop handler to the conversation browser too.

>+      <method name="onDrop">
>+        <parameter name="aEvent" />
>+        <body>
>+        <![CDATA[
>+          var dataTransfer = aEvent.dataTransfer;

Please use let and not var everywhere unless var is really needed.
Can it happen that dataTransfer is undefined?

>+          if (types.contains("application/x-moz-file"))
>+            aEvent.preventDefault();

I don't understand this. Why don't we return early here if it's not a file? If it's correct, please add a comment.

>+          // Converting every dropped item into a file/url

Comments end in .

>+            // Need to implement this properly once we have backend work completed

Please add TODO and explain what is missing. What do you mean by "backend work" here? What is blocking you?

>+            var url = Services.io.newFileURI(file);
>+            if(!file)

Space after if. When does (!file) happen?

>+            // Send a system message;
>+            this._conv.systemMessage("File dropped on the inputbox on conversation window " + url.spec);

Hopefully this can be removed now things are working?

>+      <method name="fileReceived">
>+        <parameter name="aSubject"/>
>+        <body>
>+        <![CDATA[
>+          aSubject.QueryInterface(Ci.ibFileTransferOffer);
>+          let msgString = "User wants to send you a file: " + aSubject.fileName + " of size " + aSubject.fileSize + ". Do you accept it?";

All user-facing strings must be localised. Please use the correct properties file.

>+          let acceptButton = {
>+            accessKey: 'A',

This applies also to the accessKey.

>+            callback: function () { aSubject.reject(); }

callback: function () aSubject.reject()

Comment 6

[aleth [:aleth]]

*** Original post on bio 2039 at 2013-07-12 13:22:58 UTC ***

"Hopefully this can be removed now things are working?" I should clarify: What I mean is that you should add logging to the backend (i.e. the file transfer progress should be reported to the account debug log). This can be done before any complicated progress reporting is added to the UI.

Comment 7

[Patrick Cloke [:clokep]]

*** Original post on bio 2039 at 2013-07-12 13:32:07 UTC ***

(In reply to comment #4)
> >+            callback: function () { aSubject.reject(); }
> 
> callback: function () aSubject.reject()

This is usually only used when we want to return the value, if not...please use the { }. (Also, Nit: no space after function.)

Comment 8

[aleth [:aleth]]

*** Original post on bio 2039 at 2013-07-12 13:59:23 UTC ***

(In reply to comment #6)
Actually, looking at it again what I should have suggested is 

callback: aSubject.reject

This avoids the unnecessary function definition (and there is no need to pass along the return value, should it exist, by hand).

Comment 9

[Atul Jangra [:atuljangra]]

*** Original post on bio 2039 at 2013-07-12 20:43:41 UTC ***


> Some general issues:
> 
> 1) If you attach file transfers to open conversations the way you are doing,
> what happens e.g. if the user closes the conversation tab while the file
> transfer is running? What's the expected behaviour?
I've not taken care of this currently, but yes this needs to be done.

> 
> 2) The prplIFileTransfer idl file needs an initial comment describing how
> exactly the file transfer interface is supposed to be used. (What calls what
> when etc).
Yes, Currently I haven't used most of the functions and attributes defined in the interface. I've added a method in prplIConversation for sending the file from the conversation. There are many things which may not be of any use in future, we'll just remove them. Initially I just added which I thought *maybe* needed somewhere in future.
> 
> 3) Have you thought about what happens when a file transfer fails? If we are
> leaving this for later, do you have something in mind for how it will be
> handled? Shouldn't we, for now, pick up the corresponding notification and show
> an error system message?
> Does FileLink get triggered from conversation.xml or is there going to be an
> intermediate backend layer? How does this interact with your answer to 1)?
I suppose that there will be an intermediate backend layer which will handle that. But we'll be discussing this in the md chat list.

> 
> >+interface prplIFileTransfer : nsIDownload {
> >+  /**
> >+   * generic file transfer.
> >+   */
> >+  const short FILE_TRANSFER_DOWNLOAD = 0;
> 
> I don't understand this comment.
Something like born state of a file transfer. I plan to use all these consts to interact with UI.

> 
> >+  /**
> >+  * This attribute is for getting the current status (from the above mentioned constants)

> >+  /* The conversation used for this transfer */
> >+  readonly attribute prplIConversation conv;
> 
> Why do we need to keep this reference? When reporting back to the UI, I think
> you use Services.obs.notifyObservers?
I am basically associating a file transfer with a conversation. Thus this is needed to keep track current transfers.  Association with account is also done with this. Thus I guess this is needed :) 
> 
> >+  /**
> >+   * Restart the current file tranfer.
> >+   *
> >+   * @throws NS_ERROR_UNEXPECTED if it cannot be restarted or if it is
> >+   * already in progress
> >+   */
> >+  void restart();
> 
> What is the use case for this?
I'll remove this function. This is not needed. We don't want to bug the user about re-starting a file transfer.
> 
> >+      <method name="onDrop">
> >+        <parameter name="aEvent" />
> >+        <body>
> >+        <![CDATA[
> >+          var dataTransfer = aEvent.dataTransfer;
> 
> Please use let and not var everywhere unless var is really needed.
> Can it happen that dataTransfer is undefined?
I discussed this on IRC, this is always defined in case of drag and drop,

> 
> >+            var url = Services.io.newFileURI(file);
> >+            if(!file)
> 
> Space after if. When does (!file) happen?
It is just a check which I thought would be useful.


> >+      <method name="fileReceived">
> >+        <parameter name="aSubject"/>
> >+        <body>
> >+        <![CDATA[
> >+          aSubject.QueryInterface(Ci.ibFileTransferOffer);
> >+          let msgString = "User wants to send you a file: " + aSubject.fileName + " of size " + aSubject.fileSize + ". Do you accept it?";
> 
> All user-facing strings must be localised. Please use the correct properties
> file.
 Yes, I'll add this support in the next patch

Comment 10

[Atul Jangra [:atuljangra]]

Attachment: Backend for ft

*** Original post on bio 2039 as attmnt 2589 at 2013-07-13 00:33:00 UTC ***

Addressed the changes.

Comment 11

[Atul Jangra [:atuljangra]]

Comment on attachment 8354346 [details] [diff] [review]
Backend for ft

*** Original change on bio 2039 attmnt 2578 at 2013-07-13 00:33:54 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 12

[aleth [:aleth]]

*** Original post on bio 2039 at 2013-07-13 13:46:27 UTC ***

(In reply to comment #8)
> > Some general issues:
> > 
> > 1) If you attach file transfers to open conversations the way you are doing,
> > what happens e.g. if the user closes the conversation tab while the file
> > transfer is running? What's the expected behaviour?
> I've not taken care of this currently, but yes this needs to be done.

I'm not asking you to take care of this right now, I'm asking you what you think the behaviour should be!
It seems to matter as you are associating each file transfer with a particular conversation.

> > 2) The prplIFileTransfer idl file needs an initial comment describing how
> > exactly the file transfer interface is supposed to be used. (What calls what
> > when etc).
> Yes, Currently I haven't used most of the functions and attributes defined in
> the interface. I've added a method in prplIConversation for sending the file
> from the conversation. There are many things which may not be of any use in
> future, we'll just remove them. Initially I just added which I thought *maybe*
> needed somewhere in future.

I asked you for a short introductory comment describing how this interface you have written is supposed to be used. This is standard documentation which will be important for anyone who tries to use this code later. The following is too vague: 
> * This is the interface to deal with the file transfer.
As a reader, when I read this I ask myself
- the file transfer of a single file, or...?
- what does "to deal with" mean?

If you aren't clear about this, it may help you to sketch (on a piece of paper) how you currently expect file transfer in general to work. Starting from the UI, or an incoming file transfer offer, what you expect to happen. Something along the lines of 'the UI calls ..., generating an instance of... Then the following things can happen... If they succeed, the UI is notified by... if not by ...'. You should think about where in the code the various file transfer mechanisms will be tried, and where we fall back to Filelink if they fail or are not available. I think you have a reasonable overview of what is possible in terms of notifications, observers, and callbacks, so you can be fairly specific. If not, you can ask ;)

Hopefully this will also give you a better roadmap for how to add FileLink (which I believe is your next step). The point of starting with a minimal XMPP and UI implementation was to provide the general structure of the file transfer implementation (so there is something to fall back to FileLink *from* ;) ).

It's OK to say something like "this is a minimal implementation for testing, later on I expect it to work like this" etc as long as you have a clear idea of where you are going.

> > >+interface prplIFileTransfer : nsIDownload {
> > >+  /**
> > >+   * generic file transfer.
> > >+   */
> > >+  const short FILE_TRANSFER_DOWNLOAD = 0;
> > 
> > I don't understand this comment.
> Something like born state of a file transfer. I plan to use all these consts to
> interact with UI.

Now I am even more confused. What is the difference between
   * File transfer which has not been started or initialised.
  const short FILE_TRANSFER_DOWNLOAD = 0;
and 
   * state for uninitialized object.
  const short FILE_TRANSFER_NOTSTARTED = -1;

> > >+            var url = Services.io.newFileURI(file);
> > >+            if(!file)
> > 
> > Space after if. When does (!file) happen?
> It is just a check which I thought would be useful.

But if there are situations where you think this can happen, doesn't it need error handling? If you are only being cautious and can't actually think of a circumstance where this would occur, you should still report to the error console at least, if you are worried enough to add this check in the first place.

Comment 13

[aleth [:aleth]]

Comment on attachment 8354358 [details] [diff] [review]
Backend for ft

*** Original change on bio 2039 attmnt 2589 at 2013-07-13 14:09:37 UTC ***

>"Have you thought about what happens when a file transfer fails? If we are
>leaving this for later, do you have something in mind for how it will be
>handled? Shouldn't we, for now, pick up the corresponding notification and show
>an error system message?"
>I suppose that there will be an intermediate backend layer which will handle
>that. But we'll be discussing this in the md chat list.

Even if you don't have clear idea of how FileLink will be started, this patch should show something of what happens when a file transfer fails. After all, currently your XMPP file transfer can fail. Then what? How is the conversation (or maybe later the backend layer) informed?

>+  /**
>+  * Transfer was acked by the receiver.
>+   */

What does this mean? If the receiver accepts a transfer, aren't we either going to be FILE_TRANSFER_TRANSFERRING or FILE_TRANSFER_QUEUED?

>+  /* Method to add or remove an observer */
>+  void addObserver(in nsIObserver aObserver);
>+  void removeObserver(in nsIObserver aObserver);

Who is going to add an observer? Is this part of the interaction with the download panel or something? I ask because currently you seem to be using this.conv.notifyObservers instead of the conv adding itself as an observer.

>+          if (!types.contains("application/x-moz-file"))
>+            return;
>+          aEvent.preventDefault();

I don't think you answered my question about this.

>+#LOCALIZATION NOTE
>+# This is shown when a buddy wants to send a file to the user.
>+# %1$S is the name of the file that is being transferred, %2$S size of the same file.
>+fileTransferOffer.message=Do you want the accept the file: name %1$S size %2$S from the user.

Maybe "%username would like to send you the file %filename (Size: %size bytes)"?
Of course we'll want to format the size more nicely, but this would probably do for now.

>+fileTransferOffer.accept.label=Allow

"Accept" is clearer.

Comment 14

[Benedikt Pfeifer]

Comment on attachment 8354358 [details] [diff] [review]
Backend for ft

*** Original change on bio 2039 attmnt 2589 at 2013-07-17 10:25:18 UTC ***

I don't think I've got meaningful comments beside the few things that can be found below. I hope that you don't mind that I clear the feedback request from me.

> diff --git a/instantbird/content/conversation.xml b/instantbird/content/conversation.xml
>  
> +      <method name="fileTransferOffer">
> +          box.appendNotification(msgString, 'file-transfer-offer-received', null, box.PRIORITY_INFO_MEDIUM,
> +                                 [acceptButton, rejectButton]);

You'd want to react on the notification closing most likely. There's a "close" event sent to the notification when it goes away afaik. Note that appendNotification returns the new notification on which you can then add your event listener.

> diff --git a/instantbird/locales/en-US/chrome/instantbird/instantbird.properties b/instantbird/locales/en-US/chrome/instantbird/instantbird.properties

> \ No newline at end of file

Add a new line at end of files please.

> diff --git a/purple/purplexpcom/src/purpleConvChat.h b/purple/purplexpcom/src/purpleConvChat.h
> --- a/purple/purplexpcom/src/purpleConvChat.h
> +++ b/purple/purplexpcom/src/purpleConvChat.h
> @@ -55,16 +55,19 @@ public:
>      return purpleConversation::Close();
>    }
>    NS_IMETHOD AddObserver(nsIObserver *aObserver) {
>      return purpleConversation::AddObserver(aObserver);
>    }
>    NS_IMETHOD RemoveObserver(nsIObserver *aObserver) {
>      return purpleConversation::RemoveObserver(aObserver);
>    }
> +  NS_IMETHODIMP SendFile(nsIURI * afileURI){
> +    return purpleConversation::SendFile(afileURI);
> +  }
>  
Shouldn't the parameter name be rather aFileURI?

Comment 15

[Patrick Cloke [:clokep]]

Comment on attachment 8354358 [details] [diff] [review]
Backend for ft

*** Original change on bio 2039 attmnt 2589 at 2013-09-09 23:58:02 UTC ***

Clearing out my review queue, please re-request f? after meeting aleth's comments if you plan to continue to work on this.

Comment 16

[Florian Quèze [:florian]]

Comment on attachment 8354358 [details] [diff] [review]
Backend for ft

Some feedback has already been provider by aleth and Mic here.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.