Port away from KIO::NetAccess leftovers
Details
Details
- Reviewers
wbauer - Group Reviewers
KDE Applications - Commits
- R433:a7c0490cc3a3: Port away from deprecated KIO::NetAccess
Compiles and installs, works at least with usual downloads.
Diff Detail
Diff Detail
- Repository
- R433 KGet
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
First of all, thank you for your contributions! ;-)
TBH, I'll need some time to look through this though...
Comments from other people are of course very welcome!
Just one thing I'd like to tell you right now, because of
https://cgit.kde.org/kget.git/commit/?id=e779f1896c8a2892f1e9acfab5425252eeeb06b6
AFAIK, KStandardDirs guaranteed that the target directory exists, while QStandardDirs doesn't.
I did fix a couple of porting problems related to that in other areas myself...
It looks to me that this commit might have introduced new ones in kget, i.e. certain things might seize to work if ~/.local/share/kget/ doesn't exist yet.
That's something I need to test myself anyway, but maybe you could help. ;-)
Comment Actions
This should be fixed now. Thoroughly tested on a clean profile for generic testing downloads of zip files and torrents.
https://cgit.kde.org/kget.git/commit/?id=f12296b9241a218a9f88169e0cb450eda4dddb99
Sorry for that breakage.
Comment Actions
Great, thank you very much! (for the testing as well)
I suppose it could be optimized a bit (e.g. it's not really necessary to test the existance of the directories, as QDir::mkPath() does that anyway, David Faure told me that once in one of the review requests I made... ;-) )
But please don't bother, it's more important that it works I think.
Sorry for that breakage.
Don't worry. That's a more common pitfall, and that's actually why I decided to mention it here even though it's unrelated to this patch.
Regarding this, I think it should be fine, except maybe one thing that I mentioned inline.
And I'm not sure the additional debug output is necessary, but it certainly doesn't harm either I think. (so no need to remove it from my point of view)
| core/mostlocalurl.cpp | ||
|---|---|---|
| 39–55 | Maybe use KIO::mostLocalUrl + job->exec() instead? As mentioned in the KIO::NetAccess::mostLocalUrl docs: | |
| core/mostlocalurl.cpp | ||
|---|---|---|
| 39–55 | That was some kind of copy-paste from the current implementation in kdelibs4support, but I'll try to rewrite it asap. Thanks. | |
Comment Actions
See inline, the if (job->error()) branch in particular looks quite broken to me.
OTOH, I'm not sure if trying to do something else in case of an error actually makes sense, it's probably sufficient to just return the original URL in case the job fails.
I.e., I think this could be reduced to just:
if(job->exec()) {
return job->mostLocalUrl();
}
return url;A general remark: the { should usually be on the same line as the if. Although, the original code probably isn't completely consistent either...
| core/mostlocalurl.cpp | ||
|---|---|---|
| 42 | This leads to a compiler error here: error: 'KJobWidgets' has not been declared But, do we actually need that at all? | |
| 44 | Coding style: job->exec(); (without spaces) Also, I'd probably rather test the return value of job->exec() directly like you did in the rest of this patch, instead of testing job->error() in the next line. | |
| 46–47 | What is this supposed to do? Also, coding style nitpick: no spaces inside the () | |
| 52–54 | Wrong indentation. This else belongs to the if (job->error()) { in line#44 and should be aligned to that. | |
Comment Actions
That was actually a try inspired by the similar code in Dolphin. I've tried to implement everything and might be failed. :)
Comment Actions
Should be good now, thanks!
Just noticed another minor thing.
And I would maybe just omit the additional if's and debug output altogether like the original KIO::NetAccess:synchronousRun code did (it's probably not very useful to know that deleting a temporary file failed...).
I'll leave that decision to you though. ;-)
| core/mostlocalurl.cpp | ||
|---|---|---|
| 40 | Please change to job->exec(), i.e. remove the spaces around ->. | |
| transfer-plugins/kio/transferKio.cpp | ||
| 25–26 | Is this change intentional? | |
Comment Actions
Sorry for the delay again, I was busy with other things...
Thanks for the additional changes.
Please push it, master only of course.