[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|NEW                         |ASSIGNED
  Status Whiteboard|                            |review--
           Keywords|                            |patch
         AssignedTo|telepathy-bugs at lists.freede |daniele.domenichelli at gmail.
                   |sktop.org                   |com
                 CC|                            |ollisal at gmail.com

--- Comment #3 from Olli Salli <ollisal at gmail.com> 2011-05-19 03:52:09 PDT ---
(In reply to comment #2)
> In my opinion the bug is the use of QFileInfo...
> 

AFAIK, a filename passed to QFileInfo doesn't have to refer to an existant file
(there is an exists() accessor). The use of QFileInfo is intended to weed out
any path components in case somebody passes those to us, so in essence it's
used just as "basename". Thus, I would continue using it, unless you've
discovered some actual problem with it.

> Anyway I made some changes and uploaded here [1] a first version of the patch
> for review... bonus feature: last commit adds documentation to the
> FileTransferChannelCreationProperties class (referring to the version modified
> by me, not to the original one)
> 
> [1]https://www.gitorious.org/drdanz-telepathy-kde/telepathy-qt4/commits/filetransfer-uri


    bool hasUri;
63        QString uri;

I don't think an empty URI makes sense. So I don't think you need the separate
hasUri variable.

Your added setters for the mandatory params have this:

   if (!isValid()) {
114            // there is no point in updating content hash if not valid, as
we miss filename, content
115            // type and size
116            return *this;
117        }

Aside from the out of place comment, wouldn't this mean you can't actually use
these setters to make an invalid object valid? That's actually fine, though. If
you're changing the mandatory params definining the file transfer, you probably
need to re-set the optional ones too. This was accomplished by the old API
forcing you to create a new instance (which is not a problem as the class is
very lightweight). What's the rationale for 

Another misplaced comment in setContentType:

    135        if (contentType.isEmpty()) {
136            // suggestedFileName is a mandatory parameter, therefore we
cannot set an empty string
137            return *this;
138        }

Besides, please do warn in such occasions which you make a no-op based on
arbitrary criteria like this.

> Use isValid() method to check if mandatory properties are set

This commit makes all operations on default-constructed CreationProperties to
instantly SIGSEGV, including isValid(), with no way to detect that from the
outside. Additionally, this change would make the class wildly inconsistent
with our other tiny value-semantic classes, for which isValid() means "this is
not the null object, as used as a placeholder when there is no actual value".
What's the point in this change? You can still ditch the object you were
constructing and return a null object if something goes wrong e.g. in resolving
the URI to a file even with the old, consistent approach.

> Add constructor to FileTransferChannelCreationProperties accepting uri and description

Can't you use QUrl to parse the URI? (And validate it's one in the first place,
otherwise warn and return an invalid instance). That class even has a
toLocalFile() accessor which would be useful.

Also, as description is an optional parameter, please stick to the named
parameter idiom and instead of having it as a create() param, have it set by a
setter method, which leads to more readable code. If this would lead to some
overload conflict with previously existing create() methods, please call the
new one createForURI or somesuch. You could actually use QUrl as an (overload)
parameter type directly.

In short, please redo the branch, this time focusing on what features you
actually want to add (URI passing), doing that well, and not arbitrarily
changing the structure and semantics of the class otherwise. For history
readability, this is best done by rebasing the commit I actually liked (adding
documentation) on top of master, ditching most of the others while stashing the
good bits, and starting again from there.

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