Allow to close a document whose file was deleted on disk
ClosedPublic

Authored by meven on Apr 11 2019, 1:19 PM.

Details

Summary

There is no confirmation window about data lose.

Screenshot:

FEATURE: 406305
FIXED-IN: 19.08

Test Plan

Manually tested

Diff Detail

Repository
R39 KTextEditor
Branch
arcpatch-D20467
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10738
Build 10756: arc lint + arc unit
meven created this revision.Apr 11 2019, 1:19 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptApr 11 2019, 1:19 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
meven requested review of this revision.Apr 11 2019, 1:19 PM
meven edited the summary of this revision. (Show Details)Apr 11 2019, 1:20 PM
ngraham edited the summary of this revision. (Show Details)Apr 11 2019, 1:57 PM
ngraham edited the summary of this revision. (Show Details)

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.

meven added a comment.Apr 11 2019, 3:03 PM

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.

ngraham added inline comments.Apr 11 2019, 4:26 PM
src/dialogs/katedialogs.cpp
1296 ↗(On Diff #55987)

file discarding -> file, discarding

meven updated this revision to Diff 55999.Apr 11 2019, 4:47 PM

Add a comma

meven marked an inline comment as done.Apr 11 2019, 4:47 PM
dhaumann accepted this revision.Apr 12 2019, 4:49 AM
dhaumann added a subscriber: dhaumann.

Looks good to me.

src/dialogs/katedialogs.cpp
1298

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

This revision is now accepted and ready to land.Apr 12 2019, 4:49 AM
meven updated this revision to Diff 56032.Apr 12 2019, 7:08 AM

Use new signal syntax

meven planned changes to this revision.Apr 12 2019, 7:10 AM
meven marked an inline comment as done.

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

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

meven added a comment.Apr 12 2019, 7:40 AM

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.

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

meven added a comment.Apr 12 2019, 7:49 AM

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.

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

meven updated this revision to Diff 56042.Apr 12 2019, 8:43 AM

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

This revision is now accepted and ready to land.Apr 12 2019, 8:43 AM
meven requested review of this revision.Apr 12 2019, 8:49 AM

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

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

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.

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.

meven added a comment.Apr 12 2019, 1:01 PM

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.

meven edited the summary of this revision. (Show Details)Apr 12 2019, 1:03 PM
cullmann accepted this revision.Apr 12 2019, 7:34 PM

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.

This revision is now accepted and ready to land.Apr 12 2019, 7:34 PM
This revision was automatically updated to reflect the committed changes.