update d->m_file in ReadOnlyPart::setUrl()
AbandonedPublic

Authored by pdabrowski on Feb 4 2020, 12:23 AM.

Details

Reviewers
elvisangelaccio
ngraham
dfaure
Group Reviewers
Frameworks
Summary

Properly update d->m_file so it can be safely used later in KParts::ReadOnlyPart::localFilePath().
See: D26140

BUG: 416989

Diff Detail

Repository
R306 KParts
Lint
Lint Skipped
Unit
Unit Tests Skipped
pdabrowski created this revision.Feb 4 2020, 12:23 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 4 2020, 12:23 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
pdabrowski requested review of this revision.Feb 4 2020, 12:23 AM
marten added a subscriber: marten.EditedFeb 4 2020, 8:54 AM

Yes, I'd concluded that the real place to fix the problem was at the KParts level, but not being a KParts expert wanted to leave that decision to its maintainers. +1 for the elegant fix.

One coding style change? - mostLocalUrl(QUrl url) -> mostLocalUrl(const QUrl &url)

Yes, I'd concluded that the real place to fix the problem was at the KParts level, but not being a KParts expert wanted to leave that decision to its maintainers. +1 for the elegant fix.

I'm in no case KParts expert. That's why I would like a maintainer to review this change.
Especially I'm not sure if it is ok to use KIO::StatJob here.

One coding style change? - mostLocalUrl(QUrl url) -> mostLocalUrl(const QUrl &url)

Good catch. I will fix it.

pdabrowski added inline comments.Feb 4 2020, 7:31 PM
src/readonlypart.cpp
80

TODO: mostLocalUrl(const QUrl &url)

pdabrowski updated this revision to Diff 75015.Feb 4 2020, 8:42 PM
pdabrowski marked an inline comment as done.
dfaure requested changes to this revision.Feb 4 2020, 10:28 PM

statJob->exec() creates a nested event loop, which processes timers and socket events etc. -- this can create unexpected re-entrancy and is a nasty source of bugs.
Therefore it should be avoided as much as possible, *especially* in libraries which don't have the full picture about what happens in the application.

The only safe way to do this is with an async job, i.e. signals and slots. This is tricky in general (when the caller expects things to happen immediately), but by luck this isn't the case here. Apps expect KParts to download remote files. So what could be done, is a statjob before the download, and if stat says there's a local path KParts can *then* set m_file instead of downloading.

But wait.... KParts::ReadOnlyPart::openUrl already does *exactly* that.
See the code under

// Maybe we can use a "local path", to avoid a temp copy?

which uses KIO::mostLocalUrl() the right way, async.

So I have to ask... what is this fixing, exactly? Why isn't openUrl called, or why doesn't it go into that code path, in your use case?

This revision now requires changes to proceed.Feb 4 2020, 10:28 PM

IIUC, this diff is fixing https://bugs.kde.org/show_bug.cgi?id=416989 (Konqueror -> Tools -> open terminal, doesn't work after D26140).

Looking at the code in dolphin/src/dolphinpart.cpp, openUrl() is reimplemented[1], and it does call ReadOnlyPart::setUrl(); how about checking if url is a local file and calling ReadOnlyPart::setLocalFilePath() in DolphinPart::openUrl()?

[1] https://cgit.kde.org/dolphin.git/tree/src/dolphinpart.cpp#n300

marten added a comment.Feb 5 2020, 4:57 PM

@ahmadsamir: Yes, the problem could be fixed in Dolphin, but that leaves a potential trap for any other KPart that reimplements openUrl().

As a minimum, if it is decided that the fix belongs in the application then KParts::ReadOnlyPart should have a caution in the API documentation that localFilePath() will only return a valid result if openUrl() or setLocalFilePath() has previously been called.

ahmadsamir added a comment.EditedFeb 5 2020, 5:07 PM

I thought of making ReadOnlyPart::setUrl() set d->m_file if the url is a local file... but I am guessing d->m_file and d->m_url are kept separate on purpose, so the idea seemed a bit off.

EDIT: I meant no offence by this comment.

I'm OK with "only" a documentation fix at the KParts level. If you reimplement openUrl, you get to handle that URL, obviously. That includes possibly doing the async job that resolves it to a local file path.

"making ReadOnlyPart::setUrl() set d->m_file if the url is a local file" would be fine if you mean QUrl::isLocalFile(), but NOT if you mean another KJob::exec(). So it wouldn't help.

pdabrowski abandoned this revision.Mon, Mar 2, 6:40 PM