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

[Piotr Dabrowski]

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()

[David Faure]

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 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.

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()

[Ahmad Samir]

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
  https://phabricator.kde.org/D27148

To: pdabrowski, elvisangelaccio, ngraham, #frameworks, dfaure
Cc: ahmadsamir, marten, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

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

[Jonathan Marten]

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 the API documentation that 
localFilePath() will only return a valid result if openUrl() or 
setLocalFilePath() has previously been called.

REPOSITORY
  R306 KParts

REVISION DETAIL
  https://phabricator.kde.org/D27148

To: pdabrowski, elvisangelaccio, ngraham, #frameworks, dfaure
Cc: ahmadsamir, marten, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

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

[Ahmad Samir]

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], 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

REPOSITORY
  R306 KParts

REVISION DETAIL
  https://phabricator.kde.org/D27148

To: pdabrowski, elvisangelaccio, ngraham, #frameworks, dfaure
Cc: ahmadsamir, marten, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

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

[David Faure]

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 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?

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()

[Piotr Dabrowski]

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()

[Piotr Dabrowski]

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, elvisangelaccio, ngraham, #frameworks, dfaure
Cc: marten, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Nathaniel Graham]

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()

[Piotr Dabrowski]

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

To: pdabrowski, elvisangelaccio, ngraham
Cc: marten, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Piotr Dabrowski]

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 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 )
  
  Good catch. I will fix it.

REPOSITORY
  R306 KParts

REVISION DETAIL
  https://phabricator.kde.org/D27148

To: pdabrowski, elvisangelaccio, ngraham
Cc: marten, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Jonathan Marten]

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

To: pdabrowski, elvisangelaccio, ngraham
Cc: marten, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

[Piotr Dabrowski]

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 
KParts::ReadOnlyPart::localFilePath().
  See: D26140 
  
  BUG: 416989

REPOSITORY
  R306 KParts

REVISION DETAIL
  https://phabricator.kde.org/D27148

AFFECTED FILES
  src/readonlypart.cpp
  src/readonlypart.h

To: pdabrowski, elvisangelaccio, ngraham
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns