DevHeads.net

Review Request 126659: [kio_ftp] fix display of file/directory modification time/date

Review request for KDE Frameworks, kdelibs and David Faure.

Bugs: 354597
<a href="https://bugs.kde.org/show_bug.cgi?id=354597" title="https://bugs.kde.org/show_bug.cgi?id=354597">https://bugs.kde.org/show_bug.cgi?id=354597</a>

Repository: kio

Description
- QDate() treats the year literally (i.e. 90 is really year 90, not 1990), so subtracting 1900 is wrong.
- in the case when no year is mentioned in the server's reply (the year is implicit), it wasn't set to the current year, so the result was either 0 or -1.

Diffs
src/ioslaves/ftp/ftp.cpp 2179179

Diff: <a href="https://git.reviewboard.kde.org/r/126659/diff/" title="https://git.reviewboard.kde.org/r/126659/diff/">https://git.reviewboard.kde.org/r/126659/diff/</a>

Testing
Connected to an FTP server with dolphin (15.12.0). The modification times/dates are now shown correctly.

Thanks,

Wolfgang Bauer

Comments

Re: Review Request 126659: [kio_ftp] fix display of file/directo

By Wolfgang Bauer at 01/07/2016 - 08:21

(Updated Jan. 7, 2016, 1:21 nachm.)

Review request for KDE Frameworks, kdelibs and David Faure.

Changes
- Use QDate::currentDate() instead of QDateTime::currentDateTime(), we only need the date anyway
- initialize day, month, and year to the current date instead of 0

Bugs: 354597
<a href="https://bugs.kde.org/show_bug.cgi?id=354597" title="https://bugs.kde.org/show_bug.cgi?id=354597">https://bugs.kde.org/show_bug.cgi?id=354597</a>

Repository: kio

Description
- QDate() treats the year literally (i.e. 90 is really year 90, not 1990), so subtracting 1900 is wrong.
- in the case when no year is mentioned in the server's reply (the year is implicit), it wasn't set to the current year, so the result was either 0 or -1.

Diffs (updated)
src/ioslaves/ftp/ftp.cpp 2179179

Diff: <a href="https://git.reviewboard.kde.org/r/126659/diff/" title="https://git.reviewboard.kde.org/r/126659/diff/">https://git.reviewboard.kde.org/r/126659/diff/</a>

Testing
Connected to an FTP server with dolphin (15.12.0). The modification times/dates are now shown correctly.

Thanks,

Wolfgang Bauer

Re: Review Request 126659: [kio_ftp] fix display of file/directo

By Wolfgang Bauer at 01/07/2016 - 08:23

(Updated Jan. 7, 2016, 1:23 nachm.)

Review request for KDE Frameworks, kdelibs and David Faure.

Changes
update description as well

Bugs: 354597
<a href="https://bugs.kde.org/show_bug.cgi?id=354597" title="https://bugs.kde.org/show_bug.cgi?id=354597">https://bugs.kde.org/show_bug.cgi?id=354597</a>

Repository: kio

Description (updated)
- QDate() treats the year literally (i.e. 90 is really year 90, not 1990), so subtracting 1900 is wrong.
- Use QDate::currentDate() instead of QDateTime::currentDateTime(), we only need the current date anyway
- Initialize day, month, and year to the current date instead of 0. In the case when no year is mentioned in the server's reply (the year is implicit), it wasn't set to the current year at all, so the result was either 0 or -1.

Diffs
src/ioslaves/ftp/ftp.cpp 2179179

Diff: <a href="https://git.reviewboard.kde.org/r/126659/diff/" title="https://git.reviewboard.kde.org/r/126659/diff/">https://git.reviewboard.kde.org/r/126659/diff/</a>

Testing
Connected to an FTP server with dolphin (15.12.0). The modification times/dates are now shown correctly.

Thanks,

Wolfgang Bauer

Re: Review Request 126659: [kio_ftp] fix display of file/directo

By Wolfgang Bauer at 01/07/2016 - 08:46

(Updated Jan. 7, 2016, 12:46 p.m.)

Status
This change has been marked as submitted.

Review request for KDE Frameworks, kdelibs and David Faure.

Changes
Submitted with commit 68af1d7e89b7fed136d4cc62b76c1c6ded2d94eb by Wolfgang Bauer to branch master.

Bugs: 354597
<a href="https://bugs.kde.org/show_bug.cgi?id=354597" title="https://bugs.kde.org/show_bug.cgi?id=354597">https://bugs.kde.org/show_bug.cgi?id=354597</a>

Repository: kio

Description
- QDate() treats the year literally (i.e. 90 is really year 90, not 1990), so subtracting 1900 is wrong.
- Use QDate::currentDate() instead of QDateTime::currentDateTime(), we only need the current date anyway
- Initialize day, month, and year to the current date instead of 0. In the case when no year is mentioned in the server's reply (the year is implicit), it wasn't set to the current year at all, so the result was either 0 or -1.

Diffs
src/ioslaves/ftp/ftp.cpp 2179179

Diff: <a href="https://git.reviewboard.kde.org/r/126659/diff/" title="https://git.reviewboard.kde.org/r/126659/diff/">https://git.reviewboard.kde.org/r/126659/diff/</a>

Testing
Connected to an FTP server with dolphin (15.12.0). The modification times/dates are now shown correctly.

Thanks,

Wolfgang Bauer

Re: Review Request 126659: [kio_ftp] fix display of file/directo

By David Faure at 01/07/2016 - 08:25

Ship it!

Ship It!

- David Faure

On Jan. 7, 2016, 12:23 p.m., Wolfgang Bauer wrote:

Re: Review Request 126659: [kio_ftp] fix display of file/directo

By David Faure at 01/07/2016 - 07:15

src/ioslaves/ftp/ftp.cpp (line 1775)
<https://git.reviewboard.kde.org/r/126659/#comment62040>

The porting bug is here. In kdelibs4 tmptr was initialized to the current date, and used below.

Now we have two variables, "int year" and "currentTime.year".

To be closer to the orig code and to avoid risking that the day or month is still 0 as well, I would suggest to initialize year, day and month directly from currentTime, rather than to 0.

I guess we can also remove the "currentTime.setTime(QTime(0,0,0)) because that's unused (right?)

src/ioslaves/ftp/ftp.cpp (line 1797)
<https://git.reviewboard.kde.org/r/126659/#comment62039>

yes, this is a QDate behaviour change in Qt5.

- David Faure

On Jan. 7, 2016, 10 a.m., Wolfgang Bauer wrote:

Re: Review Request 126659: [kio_ftp] fix display of file/directo

By David Faure at 01/07/2016 - 08:25

"likely just as wrong" - well with 0 the QDateTime would be invalid, making the whole information disappear (even e.g. the valid time information). So using the current datetime (as kdelibs4 did) seems better.

Agreed about currentDate().

- David

On Jan. 7, 2016, 12:23 p.m., Wolfgang Bauer wrote:

Re: Review Request 126659: [kio_ftp] fix display of file/directo

By Wolfgang Bauer at 01/07/2016 - 08:21

According to the documentation, QDate's behaviour was the same in Qt4.
But kio_ftp didn't use QDate for this in kdelibs4... ;-)

Well, if we don't know the values, taking them from currentTime is likely just as wrong as setting them to 0.
But ok, I'll change it.

Right, it is unused AFAICS.
Actually we could also just use QDate::currentDate() instead of QDateTime::currentDateTime() IMHO.

- Wolfgang

On Jan. 7, 2016, 11 vorm., Wolfgang Bauer wrote: