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

pdabrowski abandoned this revision. REPOSITORY R306 KParts REVISION DETAIL https://phabricator.kde.org/D27148 To: pdabrowski, elvisangelaccio, ngraham, #frameworks, dfaure Cc: ahmadsamir, marten, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

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

dfaure added a 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

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

ahmadsamir added a comment. 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. REPOSITORY R306 KParts REVISION DETAIL

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

marten added a comment. @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

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

ahmadsamir added a comment. 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],

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

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. 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

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

pdabrowski marked an inline comment as done. REPOSITORY R306 KParts REVISION DETAIL https://phabricator.kde.org/D27148 To: pdabrowski, elvisangelaccio, ngraham, #frameworks, dfaure Cc: marten, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

pdabrowski updated this revision to Diff 75015. REPOSITORY R306 KParts CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27148?vs=74970=75015 REVISION DETAIL https://phabricator.kde.org/D27148 AFFECTED FILES src/readonlypart.cpp src/readonlypart.h To: pdabrowski,

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

ngraham added reviewers: Frameworks, dfaure. REPOSITORY R306 KParts REVISION DETAIL https://phabricator.kde.org/D27148 To: pdabrowski, elvisangelaccio, ngraham, #frameworks, dfaure Cc: marten, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

pdabrowski added inline comments. INLINE COMMENTS > readonlypart.cpp:80 > > +QUrl ReadOnlyPart::mostLocalUrl(QUrl url) const { > +KIO::StatJob* statJob = KIO::mostLocalUrl(url); TODO: mostLocalUrl(const QUrl ) REPOSITORY R306 KParts REVISION DETAIL https://phabricator.kde.org/D27148

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

pdabrowski added a comment. In D27148#605830 , @marten wrote: > 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

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

marten added a comment. 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. REPOSITORY R306 KParts REVISION DETAIL https://phabricator.kde.org/D27148

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

pdabrowski created this revision. pdabrowski added reviewers: elvisangelaccio, ngraham. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. pdabrowski requested review of this revision. REVISION SUMMARY Properly update d->m_file so it can be safely used later in