D20467: Allow to close a document whose file was deleted on disk

[Christoph Cullmann]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:2a0cf7044f75: Allow to close a document whose file was 
deleted on disk (authored by meven, committed by cullmann).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20467?vs=56042=56091

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

AFFECTED FILES
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h
  src/document/katedocument.cpp
  src/document/katedocument.h

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Christoph Cullmann]

cullmann accepted this revision.
cullmann added a comment.
This revision is now accepted and ready to land.


  I think this should go in.
  
  For application supporting the interface, the document is removed.
  For application not supporting the interface, at least the file is closed and 
you have a empty document again.
  
  Actually for KWrite that is the wanted case, you don't want to end the 
application.

REPOSITORY
  R39 KTextEditor

BRANCH
  arcpatch-D20467

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

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Méven Car]

meven edited the summary of this revision.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Méven Car]

meven added a comment.


  kdevelop supports this interface already 
https://cgit.kde.org/kdevelop.git/tree/shell/ktexteditorpluginintegration.h?id=e2bcd581bb8a9bb0005a9fcdce3167fc6be77e40#n56
  
  But not kile as far as greping kile source for KTextEditor::Application has 
shown.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Christoph Cullmann]

cullmann added a comment.


  The current implementation at least closes the file, in all applications.
  It just doesn't remove in in all of them from the document list.
  I think that is ok enough, more can't be done in KTextEditor.
  Extra reviews for extending the applications are welcome.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Dominik Haumann]

dhaumann added a comment.


  Hm, the correct fix is to implement this function for KWrite.
  
  And: There is also KDevelop and Kile...
  Having only a half-working solution sounds suboptimal to me.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Christoph Cullmann]

cullmann added a comment.


  I think the solution with the bool setting is good enough.
  If nobody else disagrees, I would accept this later.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Méven Car]

meven requested review of this revision.
meven added a comment.


  In D20467#448537 , @cullmann wrote:
  
  > One could try to first do a closeUrl and just do the 
closeDocumentInApplication() in addition afterwards.
  
  
  Great suggestion !
  
  I have tested the current code with success in KWrite and Kate.
  
  I have disabled the data loss confirmation as I intended using a trick with 
m_fileChangedDialogsActivated.
  I am not too fond of this solution, so I would happily use feedback.
  Maybe add a boolean parameter to closeUrl like closeUrl(prompt = true) ?

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Méven Car]

meven updated this revision to Diff 56042.
meven added a comment.
This revision is now accepted and ready to land.


  Allow to close the file in kwrite, disable confirmation prompt about data loss

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20467?vs=56032=56042

BRANCH
  arcpatch-D20467

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

AFFECTED FILES
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h
  src/document/katedocument.cpp
  src/document/katedocument.h

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Christoph Cullmann]

cullmann added a comment.


  One could try to first do a closeUrl and just do the 
closeDocumentInApplication() in addition afterwards.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Méven Car]

meven added a comment.


  In D20467#448495 , @cullmann wrote:
  
  > If you really want to close the document aka removing it from the 
application's document list, you need the interface.
  
  
  That's what we want for kate.
  
  > If you just want to set it back to "untitled document" you can use closeUrl
  
  That would be the path to use for kwrite.
  
  Is there a proper way to differentiate between the two ?
  
  I would like also to remove the confirmation dialog about loosing data.
  ReadWritePart has a closeUrl(bool prompt) that could be of use.
  I'd rather avoid extending the interface.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Christoph Cullmann]

cullmann added a comment.


  If you really want to close the document aka removing it from the 
application's document list, you need the interface.
  If you just want to set it back to "untitled document" you can use closeUrl

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Méven Car]

meven added a comment.


  In D20467#448493 , @cullmann wrote:
  
  > That is because KWrite doesn't implement that interface function, 
unfortunately.
  
  
  I am looking for an alternative, perhaps I don't need to use the interface.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Christoph Cullmann]

cullmann added a comment.


  That is because KWrite doesn't implement that interface function, 
unfortunately.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Méven Car]

meven planned changes to this revision.
meven marked an inline comment as done.
meven added a comment.


  The patch does not work properly for kwrite : in kwrite currently the close 
button has no effect.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Méven Car]

meven updated this revision to Diff 56032.
meven added a comment.


  Use new signal syntax

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20467?vs=55999=56032

BRANCH
  arcpatch-D20467

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

AFFECTED FILES
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h
  src/document/katedocument.cpp
  src/document/katedocument.h

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Dominik Haumann]

dhaumann accepted this revision.
dhaumann added a comment.
This revision is now accepted and ready to land.


  Looks good to me.

INLINE COMMENTS

> katedialogs.cpp:1298
> +m_message->addAction(closeFile, false);
> +connect(closeFile, SIGNAL(triggered()), this, 
> SIGNAL(closeTriggered()));
> +

Could you switch this to new style signal slot syntax? In a separate review 
request would also be fine.

REPOSITORY
  R39 KTextEditor

BRANCH
  arcpatch-D20467

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

To: meven, cullmann, #kate, #frameworks, dhaumann
Cc: dhaumann, ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, 
michaelh, bruns, demsking, cullmann, sars

D20467: Allow to close a document whose file was deleted on disk

[Méven Car]

meven marked an inline comment as done.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks
Cc: ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, 
bruns, demsking, cullmann, sars, dhaumann

D20467: Allow to close a document whose file was deleted on disk

[Méven Car]

meven updated this revision to Diff 55999.
meven added a comment.


  Add a comma

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20467?vs=55987=55999

BRANCH
  arcpatch-D20467

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

AFFECTED FILES
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h
  src/document/katedocument.cpp
  src/document/katedocument.h

To: meven, cullmann, #kate, #frameworks
Cc: ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, 
bruns, demsking, cullmann, sars, dhaumann

D20467: Allow to close a document whose file was deleted on disk

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> katedialogs.cpp:1296
> +
> closeFile->setIcon(QIcon::fromTheme(QStringLiteral("document-close")));
> +closeFile->setToolTip(i18n("Close the file discarding its 
> content."));
> +m_message->addAction(closeFile, false);

`file discarding` -> `file, discarding`

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks
Cc: ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, 
bruns, demsking, cullmann, sars, dhaumann

D20467: Allow to close a document whose file was deleted on disk

[Méven Car]

meven added a comment.


  In D20467#448127 , @ngraham wrote:
  
  > > There is still a confirmation dialog afterwards currently.
  > >  I might be in favor of removing it.
  >
  > +1, once you've made the decision to close the deleted file, the 
confirmation dialog is unnecessary and annoying.
  
  
  I might do it in a next review, since that needs some editing in kate as well 
to keep things small and clean.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks
Cc: ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, 
bruns, demsking, cullmann, sars, dhaumann

D20467: Allow to close a document whose file was deleted on disk

[Nathaniel Graham]

ngraham added a comment.


  > There is still a confirmation dialog afterwards currently.
  >  I might be in favor of removing it.
  
  +1, once you've made the decision to close the deleted file, the confirmation 
dialog is unnecessary and annoying.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks
Cc: ngraham, kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, 
bruns, demsking, cullmann, sars, dhaumann

D20467: Allow to close a document whose file was deleted on disk

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks
Cc: kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, 
bruns, demsking, cullmann, sars, dhaumann

D20467: Allow to close a document whose file was deleted on disk

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks
Cc: kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, 
bruns, demsking, cullmann, sars, dhaumann

D20467: Allow to close a document whose file was deleted on disk

[Christoph Cullmann]

cullmann added reviewers: Kate, Frameworks.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann, #kate, #frameworks
Cc: kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, 
bruns, demsking, cullmann, sars, dhaumann

D20467: Allow to close a document whose file was deleted on disk

[Méven Car]

meven edited the summary of this revision.

REPOSITORY
  R39 KTextEditor

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

To: meven, cullmann
Cc: kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, 
bruns, demsking, cullmann, sars, dhaumann

D20467: Allow to close a document whose file was deleted on disk

[Méven Car]

meven created this revision.
meven added a reviewer: cullmann.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
meven requested review of this revision.

REVISION SUMMARY
  There is still a
  
  BUG: 406305
  FIXED-IN: 19.09

TEST PLAN
  Manually tested

REPOSITORY
  R39 KTextEditor

BRANCH
  master

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

AFFECTED FILES
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h
  src/document/katedocument.cpp
  src/document/katedocument.h

To: meven, cullmann
Cc: kwrite-devel, kde-frameworks-devel, gennad, domson, michaelh, ngraham, 
bruns, demsking, cullmann, sars, dhaumann