KWrite: Implement skeleton KTextEditor::Application
ClosedPublic

Authored by michalhumpula on May 25 2019, 4:52 PM.

Details

Summary

Everything else is implemented to enable closing kwrite on command in vi-mode.

The question is, whether all other Application methods should be implemented too? I did the documents() only because the Qt runtime was complaining about it's non existence.

BUG: 392858

Diff Detail

Repository
R40 Kate
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
michalhumpula created this revision.May 25 2019, 4:52 PM
Restricted Application added a project: Kate. · View Herald TranscriptMay 25 2019, 4:52 PM
Restricted Application added a subscriber: kwrite-devel. · View Herald Transcript
michalhumpula requested review of this revision.May 25 2019, 4:52 PM

I think a skeleton is ok, but is quit really just close? I thought it should try to quit the app not only close one window.

I think a skeleton is ok, but is quit really just close? I thought it should try to quit the app not only close one window.

Kate does something like this too. Calls save session and then closes main window. The qApp then quits automatically. I've tested it and it even recognizes the document in dirty state and asks for saving.

You can do View > New Window. Both windows then show the same file and share the text buffer. I think Christoph was referring to this situation when asking about close vs quit.

I thought it should try to quit the app not only close one window.

It doesn't close here the application, just one window...but without this patch :-) But that may OK. How else can you remove one view?

Um, I don't really understand the description of this patch, but it's something regarding close/quite KWrite.

I have here a small patch that removes the "Close Document Action" completely. But I fear nobody will like this.
KWrite is described as a simple application, and it has only one document to manage. For me make this close document no sense. Why should someone want this? You end up with an empty new document. When you want one, use the "New" action.

BTW: Open a new view disables the "Close Document Action" atm only in one view, not both.

I think this is correct: you can do File > open multiple times, then you have multiple KWrite windows. To the user it looks as if this are multiple processes. Calling quit() in this case would also quit all other KWrite main windows, which is clearly wrong.

I suggest the following:

  • if there is only one KWrite window in the KWrite process, quit().
  • if there are multiple windows, call close().

But since this is the KTextEditor:: Application interface, just calling quit() here should be correct.

The other part of the fix should be done in the vi mode q command.

Uh, this is getting more complicated then I though it would be.

So, if I call QApplication::quit() from the KWrite::quit the ktexteditor doesn't even get a chance to notify user about unsaved changes. For the specific vi-mode usecase I think calling close on active window is perfectly ok.

But... since the KTextEditor::Application object is registered multiple times when KWrite object is created , then this patch creates a mayhem when multiple windows are open. So the only way out I can see is to create the regular KWrite Application as in kate. And that is this update

Good part:

  • there are no static properties in KWrite anymore.

Bad part:

  • ktexteditor vi-mode close command is unable to handle dummy main window correctly and causes segfault

Ugly:

  • in order to implement KTextEditor::Application properly the kwrite would have to track which KWrite is active and all the other stuff.

So apparently this went quickly over my paygrade. No idea where to go from here.

You are right that one needs one unique KTextEditor::Application, otherwise the semantics are bogus.
I think this patch is a good start to create one.
If you have not further time or fun to work on this, I can give it a try.

Time is so far ok, but it's going to be a bigger change then what I initially expected and I'm not sure if it's going to be in a good way for kwrite as minimal editor.

If you have the time, some proper application object should do KWrite no harm.

anthonyfieroni added inline comments.
kwrite/kwriteapplication.cpp
86

don't removed in same loop, just call clear afterwards.

meven added a subscriber: meven.May 27 2019, 8:50 AM
meven added inline comments.
kwrite/kwriteapplication.cpp
81

Unused I believe.

82

Avoid foreach, you can use for future proofing :

for (KWrite *w: qAsConst(m_windows)) {

Reference:
https://www.kdab.com/goodbye-q_foreach/

michalhumpula updated this revision to Diff 59020.EditedJun 2 2019, 1:36 PM

This one implements the basic KTextEditor interface.

Important notes, it's not segfaulting anymore :) Did a valgrind on it and it shows only stuff in KCrush and EditorPrivate, so pointer bookeeping should be correct.

Also the vi quit command now works as expected :-)

It's a little big bigger, so if you want to split it to parts, just tell me, where to draw the lines between patches.

ngraham retitled this revision from KWrite: Implement sceleton KTextEditor::Application to KWrite: Implement skeleton KTextEditor::Application.Jun 2 2019, 10:43 PM

I think as an initial version this is ok.

But you need to fix the list alteration during iteration

for(auto kwrite: m_kwrites) {

    if (!kwrite->close()) {
        return false;
    }

    m_kwrites.removeAll(kwrite);
    delete kwrite;
}
cullmann requested changes to this revision.Jun 5 2019, 11:56 AM
This revision now requires changes to proceed.Jun 5 2019, 11:56 AM

So, I'm copiing the KWrite QList for the context of the iteration. As far as I understand how QList behaves, this should be ok. Funkcionality is still the same as with the previous patch version.

cullmann accepted this revision.Jun 8 2019, 7:18 PM

Ok for me, is a good initial start!
Thanks for the work on this.

This revision is now accepted and ready to land.Jun 8 2019, 7:18 PM
This revision was automatically updated to reflect the committed changes.