[Differential] [Commented On] D4422: Fix KCompressionDevice to work with Qt >= 5.7

2017-02-04 Thread David Faure 
dfaure added a comment.


  Yeah using qiodevice_p.h is out of the question ;)
  
  For info the commit log syntax for autoclosing is
  Differential Revision: https://phabricator.kde.org/D4422

REPOSITORY
  R243 KArchive

REVISION DETAIL
  https://phabricator.kde.org/D4422

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: aacid, dfaure
Cc: dfaure, anthonyfieroni, #frameworks

[Differential] [Commented On] D4422: Fix KCompressionDevice to work with Qt >= 5.7

2017-02-04 Thread Albert Astals Cid 
aacid added a comment.


  In https://phabricator.kde.org/D4422#82977, @dfaure wrote:
  
  > So your patch adds a member variable which duplicates 
QIODevice::d->devicePos, but there's no other solution without adding more API 
to QIODevice.
  >
  > I deem QIODevice API incomplete, and your patch correct given the 
circumstances.
  
  
  Yeah, one can use the _p.h header that exposes devicePos but i thought 
depending on the private implementation wasn't cool.

REPOSITORY
  R243 KArchive

REVISION DETAIL
  https://phabricator.kde.org/D4422

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: aacid, dfaure
Cc: dfaure, anthonyfieroni, #frameworks

[Differential] [Commented On] D4422: Fix KCompressionDevice to work with Qt >= 5.7

2017-02-03 Thread Albert Astals Cid 
aacid added a comment.


  In https://phabricator.kde.org/D4422#82875, @anthonyfieroni wrote:
  
  > I understand code, it will work now, but to me it's a workaround.
  
  
  Why is it a workaround?
  
  > What about to call QIODevice::seek(0) in constructor, other unchanged to 
original code ?
  
  Please, test stuff before suggesting it, saying "No" to every random thing 
you think may work is not fun.

REPOSITORY
  R243 KArchive

REVISION DETAIL
  https://phabricator.kde.org/D4422

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: aacid
Cc: anthonyfieroni, #frameworks

[Differential] [Commented On] D4422: Fix KCompressionDevice to work with Qt >= 5.7

2017-02-03 Thread Anthony Fieroni 
anthonyfieroni added a comment.


  I understand code, it will work now, but to me it's a workaround. What about 
to call QIODevice::seek(0) in constructor, other unchanged to original code ?

REPOSITORY
  R243 KArchive

REVISION DETAIL
  https://phabricator.kde.org/D4422

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: aacid
Cc: anthonyfieroni, #frameworks

[Differential] [Commented On] D4422: Fix KCompressionDevice to work with Qt >= 5.7

2017-02-03 Thread Albert Astals Cid 
aacid added a comment.


  In https://phabricator.kde.org/D4422#82848, @anthonyfieroni wrote:
  
  > > When subclassing QIODevice, you must call QIODevice::seek() at the start 
of your function to ensure integrity with QIODevice's built-in buffer.
  >
  > http://doc.qt.io/qt-5/qiodevice.html#seek
  >
  > For me, i'm not test it, it should be
  >
  >   if (!QIODevice::seek(pos)) {
  >   return false;
  >   }
  >   qint64 ioIndex = this->pos();
  >   .
  >   .
  >   .
  >
  
  
  No

REPOSITORY
  R243 KArchive

REVISION DETAIL
  https://phabricator.kde.org/D4422

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: aacid
Cc: anthonyfieroni, #frameworks

[Differential] [Commented On] D4422: Fix KCompressionDevice to work with Qt >= 5.7

2017-02-03 Thread Anthony Fieroni 
anthonyfieroni added a comment.


  > When subclassing QIODevice, you must call QIODevice::seek() at the start of 
your function to ensure integrity with QIODevice's built-in buffer.
  
  http://doc.qt.io/qt-5/qiodevice.html#seek
  
  For me, i'm not test it, it should be
  
if (!QIODevice::seek(pos)) {
return false;
}
qint64 ioIndex = this->pos();
.
.
.

REPOSITORY
  R243 KArchive

REVISION DETAIL
  https://phabricator.kde.org/D4422

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: aacid
Cc: anthonyfieroni, #frameworks