[Bug 37034] Telepathy-Qt4 doesn't set the URI property for file transfers.

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=37034

Olli Salli <ollisal at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|review--                    |review-

--- Comment #8 from Olli Salli <ollisal at gmail.com> 2011-05-29 10:01:16 PDT ---
Reviewing again.

> Clean suggestedFileName when it is stored instead of when it is used in FileTransferChannelCreationProperties

Good, this makes sense!

244 request.insert(QLatin1String(TELEPATHY_INTERFACE_CHANNEL_TYPE_FILE_TRANSFER
".URI"),
245 properties.uri());

This will unconditionally (even if !hasURI) insert the URI key to the request.
This is wrong; it'd make all existing users of the API pass the URI key with an
empty value (which is invalid) in the request.

On that subject, where actually is the hasURI method mentioned in the commit
message? Otherwise the commit adding the URI accessors looks fine.

> Add constructor that accepts URI property to
> FileTransferChannelCreationProperties

No! This is again inconsistent with the other optional properties. *Only the
mandatory properties should be set by the constructor, the others by the
setters following the named parameter idiom*. When we're passing the mandatory
properties (suggested fn, content type, size) explicitly, the URI is *not* a
mandatory property. Hence I don't see any reason this constructor should be
there.

Revert, please.

> Set URI property if suggestedFileName is a local file when creating 
> FileTransferChannelCreationProperties

NO! Again, you're changing behavior of the (only) previously existing API here.
Previously, the intended usage of the API was to pass the suggested file
basename (not a path). The use of QFileInfo to strip out any path components
was only a robustness measure (Andre, as the original writer of this code, if
you're reading this, correct me if I'm wrong). Thus you've now made the
previously intended usage emit a warning, or in some cases (if the file is
actually in our working directory), additionally set a new request property.

Revert, please.

: contentType(QLatin1String("application/octet-stream")), //TODO set
contentType from content of the file

I don't think you can in the general case, without reimplementing file(1) and
then some. Hence, you must retain the content type as a separate parameter.

            // TODO set contentHash and contentHashType in this method

You can't, because the content hash type doesn't depend on the file: it depends
on the content hash types supported by the protocol / connection manager in
question. Multiple ones might be applicable. Thus, you'd need a separate
constructor parameter to choose the desired content hash type, and as the
content hash is optional that'd again be inconsistent with the other optional
parameters.

Otherwise a constructor which takes a path, or an URL to a local file
(whichever seems more convenient to you from an application POV) is a good
idea.

92            } else {
93                warning() << path << "is not a local file.";
94            }

As the required properties (at least size) couldn't be set in this case, you
must arrange for the constructor to return an !isValid() object. My suggestion:
check in this public constructor for mPriv->suggestedFileName being empty, in
which case make mPriv NULL.

Since you're online I'm posting this part now so you can get started. I'll
review the FTChannel additions in a separate comment and the docs afterwards
(you'll probably adjust them a bit for these changes, right?).

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.