#17 Don't automatically close explicit QIODevice

pending
nobody
None
5
2014-02-09
2013-11-07
k-ziegler
No

The doc for QuaZip::close() states:

The underlying QIODevice is also closed, regardless of whether it was set explicitly or not.

An explicitly set QIODevice should not be closed on QuaZip::close(). At least the caller should be allowed to override an 'autoClose' property of sorts.

While this is generally true imho, QSaveFile for example does not have a public close() function at all and thus cannot be used as an explicit QuaZip device. (I would agree though that changing the visibility of close() is doubtful to say the least.)

This may have to be sync'd with QuaZip::open() somehow:

If the ZIP file is accessed via explicitly set QIODevice, then this device is opened in the necessary mode. If the device was already opened by some other means, then the behaviour is defined by the device implementation, but generally it is not a very good idea. For example, QFile will at least issue a warning.

Discussion

  • Sergey A. Tachenov

    While implementing this isn't that hard (although not that easy either), I don't see a point of this. If it was possible to write to streaming devices like QTcpSocket, that would be a great feature (write a header of some protocol then the ZIP file, then something more if you need). But since it's impossible anyway, and QuaZIP utilizes seeking heavily, you'll just end up with a non-closed device with unknown position. What's the point? You can reopen it if you wish and get at least predictable position by just doing that.

    And I can't even imagine why you would need it in the reading mode.

     
  • k-ziegler

    k-ziegler - 2013-11-07

    Thx for your quick reply.

    What's the point?

    The point is that QuaZip doesn't cooperate with all QIODevices that come with Qt.

    Please refer to my remark regarding QSaveFile in the starting post. Trying to inject a QSaveFile into QuaZip will crash the application on QuaZip::close() as it implicitly calls QIODevice::close() - which isn't public in case of QSaveFile.

    Hmm. As I write these lines, I come to think QSaveFile was poorly designed. In fact I even consider to file a bug at Qt Project for its LSP violation. Your opinion?

    From an abstract viewpoint, since QuaZip doesn't inherit QIODevice but has one injected eventually, it feels natural to me it doesn't alter the injection's state behind the scenes. I understand this can be argued about. Also from a practical stand you made some valid points.

    As for reading mode: I can't think of a reason either.

     
  • Sergey A. Tachenov

    Oh, I didn't get the fact that QSaveFile inherits from QIODevice. I just thought that it was a weird QIODevice look-alike. You're right, this is definitely bad design, and I'm surprised that C++ actually allows that, as it breaks the "is-a" relationship. I don't think you should file a bug, though, because Qt doesn't change its ABI. But it should be considered for Qt 6, and I think those guys are already aware of what they did there. Or maybe not, Qt is infamous for poor I/O API design (they didn't bring back QIODevice::flush() in Qt 5, for example).

    OK, I think I'm going to implement this "auto close" flag, but leave it on by default for the sake of compatibility. On top of that, I'll modify the close operation so that it checks if the device is a QSaveFile, and doesn't call close() if it is.

     
  • k-ziegler

    k-ziegler - 2013-11-07

    OK, I think I'm going to implement this "auto close" flag, but leave it on by default for the sake of compatibility.

    Cool, thx. Default should definitely stay as-is.

    On top of that, I'll modify the close operation so that it checks if the device is a QSaveFile, and doesn't call close() if it is.

    Do you really want to do this?

    As a compromise to avoid crash in case the user doesn't explicitly unset the "auto close" flag, you could consider to ask the meta object system for existence of a public close() method. Bad enough, but slightly more general.

     
  • Sergey A. Tachenov

    Well, it's an ugly workaround either way, but more general is better. I just forgot that meta system works well enough with regular methods too. OK, so be it.

     
  • Sergey A. Tachenov

    Oops, I totally forgot about this one. Expect to get it done by 0.6.2 (I hope).

     
  • Sergey A. Tachenov

    OK, implemented in r225, will make its way into the next release.

    Call QuaZip::setAutoClose(false) to prevent auto-closing. In addition, if the device was already opened, QuaZIP no longer tries to open it, but instead checks if it's opened in the correct mode.

    I couldn't get the workaround with the meta object system working. It looks like it only works with signals, slots and Q_INVOKABLE methods, while close() is a regular method that isn't accessible through the meta object system. So I had to resort to an ugly hack with qobject_cast<QSaveFile*> to call commit() instead of close() if the auto-close is enabled. I really hope it's the last time Qt breaks the is-a relationship.

     
  • Sergey A. Tachenov

    • status: open --> pending
     

Log in to post a comment.