More fixes to compile without implicit conversion from ASCII/ByteArray
ClosedPublic

Authored by ahmadsamir on Oct 1 2019, 6:12 PM.

Diff Detail

Repository
R241 KIO
Branch
ahmad/ascii-cast2 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17223
Build 17241: arc lint + arc unit
ahmadsamir created this revision.Oct 1 2019, 6:12 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 1 2019, 6:12 PM
ahmadsamir requested review of this revision.Oct 1 2019, 6:12 PM
kossebau edited reviewers, added: Frameworks; removed: kossebau.Nov 24 2019, 1:25 PM
dfaure requested changes to this revision.Nov 24 2019, 3:40 PM

Thanks!

src/core/forwardingslavebase.cpp
91

QLatin1String is sufficient for comparisons (and cheaper)

src/core/kremoteencoding.cpp
95

KF6 TODO: return QByteArray

src/core/slavebase.h
979

This method should be const

src/ioslaves/ftp/ftp.cpp
1232

Should this be remoteEncoding()->decode(...) given that the method will then use q->remoteEncoding()->encode()?

(I'm a bit confused with kremoteencoding, I could be wrong)

src/kcms/kio/kcookiespolicies.cpp
367

fromLatin1 is enough for adviceToStr, like you did on line 177 (so it's inconsistent)

(repeats 3 more times)

src/kcms/kio/smbrodlg.cpp
122

You could remove the QChar() around I guess.

src/kioexec/main.cpp
199

Ouch.

QFileInfo info(it->path)

should be enough.

262

Same here

This revision now requires changes to proceed.Nov 24 2019, 3:40 PM
ahmadsamir updated this revision to Diff 70271.Nov 24 2019, 7:56 PM
ahmadsamir marked 8 inline comments as done.
ahmadsamir retitled this revision from More (and last) fixes to compile without implicit conversion from ASCII/ByteArray to More fixes to compile without implicit conversion from ASCII/ByteArray.

Address review comments.

Rebase, and fix some more instances of casting from ASCII that crept up since the diff was created.

ahmadsamir added inline comments.Nov 24 2019, 7:56 PM
src/core/slavebase.h
979

A B C, nothing changed inside make it const, D E F.

src/ioslaves/ftp/ftp.cpp
1232

Yes, that makes much more sense; using remoteEncoding()->decode() makes this bit of code consistent with the reset of how the ftp ioslave handles encoding.

src/kcms/kio/kcookiespolicies.cpp
367

Apparently I was confused by the I18N_NOOP calls in adviceToStr, but looking again now, the translations aren't saved in the config file...

src/kcms/kio/smbrodlg.cpp
122

You're just being polite.

src/kioexec/main.cpp
199

In my defence, since that looks a bit too crazy even to me, I was worried about files with names that have encoding issues, e.g. the dreaded �; but looking up just a little at the code I see that fileList was written by the code, so if the encoding did go south, that ship has sailed and already has sunk :)

Ping. (I want to get this committed to close the gates before any more implicit conversions sneak in :)).

dfaure requested changes to this revision.Nov 26 2019, 8:07 PM
dfaure added inline comments.
src/core/slavebase.h
979

Yep ;)

Hmm, also it's in the middle of member variables, better not mix them up.
Can you make the method private, if it's only used by slavebase.cpp?
[if not, then it needs documentation and @since and a better name...]

src/filewidgets/kfileplacesmodel.cpp
262

This is more commonly written as "static const"

src/kioexec/main.cpp
199

In any case, encoding issues are for QString<->QByteArray conversions. Any time we can stick to QString<->QString like here, we're safe :)

src/kpasswdserver/autotests/kpasswdservertest.cpp
304

[strange mix of QLatin1String and QStringLiteral, but harmless]

This revision now requires changes to proceed.Nov 26 2019, 8:07 PM
yurchor added inline comments.
src/filewidgets/kfileplacesmodel.cpp
262

This does not work. Our translation system cannot extract from something like

I18NC_NOOP(fsBookmarks, QStringLiteral("Desktop"))

all the strings below with I18NC_NOOP should be left as is. Thanks.

dfaure added inline comments.Nov 26 2019, 9:26 PM
src/filewidgets/kfileplacesmodel.cpp
262

Oh, right, indeed. Duplication FTW :-)

ahmadsamir updated this revision to Diff 70379.Nov 26 2019, 9:43 PM
ahmadsamir marked 5 inline comments as done.
  • Keep I18NC_NOOP working
  • QLatin1String where appropriate
  • Rename mProtocolToStr() to protocolName(), and make it private (only used in slavebase)
ahmadsamir added inline comments.Nov 26 2019, 9:45 PM
src/filewidgets/kfileplacesmodel.cpp
262

Fixed (and noted for the future). Thanks.

dfaure added inline comments.Nov 26 2019, 10:11 PM
src/filewidgets/kfileplacesmodel.cpp
273

The QStringLiteral here might create trouble for the translation extraction scripts?

I think the proper fix is to change the signature of createSystemBookmark to take two QByteArray arguments. Or you could even keep const char* for the context (the first one of the two) since it's dropped, no point in creating a QByteArray out of it.

src/kpasswdserver/autotests/kpasswdservertest.cpp
325

[same as above]

Per dfaure's recommendation, change createSystemBookmark 2nd and 3rd params to take a const char * (since this is the translation context which will be dropped) and a QByteArray, respectively.

dfaure accepted this revision.Nov 27 2019, 11:55 AM
This revision is now accepted and ready to land.Nov 27 2019, 11:55 AM
This revision was automatically updated to reflect the committed changes.
ahmadsamir marked an inline comment as done.