[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

--- Comment #6 from Olli Salli <ollisal at gmail.com> 2011-05-23 09:18:23 PDT ---
(In reply to comment #5)
> (In reply to comment #3)
> > (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.
> 
> I thought that the string had to correspond to a real file, but I made some
> tests and I was wrong, so I'll revert this. The only problem I found with it is
> that if the string ends with "/", fileName is empty, but I guess that's
> actually not a real problem.
> 

In this case the path doesn't refer to a file we could send anyway, so the best
thing we can do is to warn and return the null object if such a path is passed
in.

> 
> > 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?
> 
> This is in the 3rd commit, where isValid() is not modified yet.
> isValid() returns false only if mPriv was not created, that means that the
> empty constructor was used.
> The comment is in that place for coherence with the other setters in the same
> file...

I meant that all comments say "updating content hash" and that set of missing
things even in setters other than the content hash one.

> 
> In the 5th commit the behaviour of isValid is modified to return true if
> mandatory parameters are set, the default constructor is modified so that
> constructs an empty mPriv and all the "isValid" checks in setters (and also in
> getters) are removed, in order to actually use these setters to make the object
> valid.

mPriv is a pointer. Your modification to the default constructor's initial list
is a no-op: you're default-constructing the pointer as before, though you've
now written that explicitly. The default value for pointers is NULL, which
triggers a crash in all accessors the way you modified them.

Again, it would be conceptually a lot cleaner if you had the null object (as
created by the default constructor, and others if invalid args are passed), and
no way to try and mend the object to make it valid. This is the semantics for
all other similar classes in TpQt4 as well in addition to being the existing
behavior for this specific class.

> 
> It was probably better to invert the order of the commits, but the first 4
> commits were supposed to be "light" commits that don't modify the behaviour of
> the class and I wasn't really convinced by the 5th commit...
> 
> > > 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.
> 
> This commit also modifies the behaviour of the default constructor, to
> construct an empty mPriv, and you can use it and set mandatory parameters
> later.
> Therefore it shouldn't SIGSEGV

It does, see above

> Actually I thought that the meaning of "isValid()" was "This is something that
> you can use to create a valid channel request.

Yes, it's not that. Sorry for the missing documentation. The other way around
is true, though: !isValid() means that "passing this in for a request makes no
sense at all, because it's the null object".

> 
> I believe that some check like this is needed, because creating the
> CreationProperties by URI won't set the mandatory parameters if the file is not
> a local file (Specifications explicitly affirm that "For outgoing files, this
> URI MAY use a different scheme, such as http:, if a remote resource is being
> transferred to a contact."
> 
> Suggestions?
> 
> 

Then you need to have an another create which accepts non- file: URLs and the
required supplementary parameters, and warn and return the default-constructed
object in the just-an-URL one for non-local URLs.

> 
> > If you're changing the mandatory params definining the file transfer, you
> > probably need to re-set the optional ones too.
> 
> Not necessarily, see previous comment and the documentation (6th commit) for
> default constructor and constructor by uri

Not necessarily, but sometimes/often you do. Allowing potentially conflicting
changes to the mandatory parameters after creation doesn't bring any advantage,
just confusion possibilities, as long as you have enough creation methods to
cover all cases (such as apparently local and remote URLs separately).

> 
> 
> > 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        }
> 
> Where should the comment be? I just copied the style of the comments in the
> other methods...
> 

The style might be copied, but not the contents verbatim. This comment is from
the content type setter, yet it refers to the suggested file name.

> 
> 
> > 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.
> 
> That was supposed to be a constructor that can be used (if the file is a local
> file) to set at the same time all the properties. Most properties can be set
> from the file, the only one that cannot be read is the description, therefore I
> added it, as it didn't see any reason not to do it...
> Anyway I'll remove it.
> 

Thanks. A good reason to not include it is consistency with the other create
methods, which don't have a way to pass in any of the optional parameters (or
description specifically) either.

> 
> > 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.
> 
> URI passing is actually just the 2nd commit...
> But in order to add a constructor that accepts URI I believe that some changes
> are required, otherwise it will have problems with remote files.

Rather add two constructors as suggested above, please.

A general guideline: You can only add new API. You can't change the behavior
and/or semantics of existing API. Other than breaking strict backwards
compatibility, doing so would make the class inconsistent with the rest of
TpQt4.

> 
> Anyway in next I will also add the getter in FileTransferChannel and the setter
> in IncomingFileTransferChannel...

Great!

> 
> Should I add the signal URIDefined as well or is it done automatically? In
> FileTransferChannel or in IncomingFileTransferChannel? (In the specs it's in
> FileTransferChannel, but I guess it makes no sense if the setter is only in
> IncomingFileTransferChannel...)

Ah, true, there is no general change notification for the property, just
notification for it being defined in the first place. The signal is not in any
way automatically added to the high-level class: the autogeneration is limited
to the low-level Tp::FileTransferChannelInterface class. Thus you need to add a
new signal to IncomingFileTransferChannel and connect the FTCInterface
generated signal to it. You don't actually need a general-purpose change
notification signal in that case.

We should place the signal in IFTC only, because the spec explicitly states
that the property must remain fixed for the lifetime of an outgoing file
transfer, so we don't need the signal or the setter 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.