As I see it, it's *either* openUrl/closeUrl *or* openStream/writeStream/closeStream.

Hm, when I before did a quick search for how to use the stream API, I came across
https://api.kde.org/frameworks/khtml/html/classKHTMLPart.html#details
which made me think the closeStream is more in the idea of QIODevice::close, to hint that there is no further data coming in.

The API dox of ReadOnlyPart enforced that idea with me:

• ReadOnlyPart::closeStream(): "Terminate the sending of data to the part."
• ReadOnlyPart::doCloseStream(): "This is called by closeStream(), to indicate that all the data has been sent. Parts should ensure that all of the data is displayed at this point."

And with that in mind the existing ReadOnlyPart code matched my expectations, as ReadOnlyPart::openStreamcalls closeUrl, similar to how ReadOnlyPart::openUrl calls it.

So openUrl would be only matched by openStream/writeStream/closeStream, with the latter triple being the push variant to the first. And closeUrl would be used with both, to tell the part to unload the current content, disconnect from the content source if needed and display some void, with the url being reset to invalid/none. So the whole purpose of closeStream would be to just signal that no more data will come in, so e.g. any "loading more" indicator can be hidden.

I now found the original discussion thread for the stream API (KParts API: thinking about the future..., just 15 years old :) ) which does not shed that much more light onto the idea of the API, though I could read into it my existing understanding of the API :)

But that's why I included you @dfaure into this review, as there is no current implementation/usage of the API, so chance I just see what I need here :) And hope you(r memories) could correct me were wrong. Though now I see your initial memories conflicting with the API dox & code? Meh...

Oops, yes, you're completely right. I got distracted by the word close, but it's a different meaning in closeStream and closeUrl. Apologies for the confusion.

As to existing implementations of this stuff, it just happens that I wrote one recently, see konqueror.git branch webengine_streaming, commit e1f9ebc30

"how would zoom and other custom state properties be save and retrieved again" -> using BrowserExtension's saveState/restoreState as usual, no? I'm not 100% sure about the interaction with streaming, but normally that happens after opening the url anyway, so it should be unrelated.

In D7580#142891, @dfaure wrote:

Oops, yes, you're completely right. I got distracted by the word close, but it's a different meaning in closeStream and closeUrl. Apologies for the confusion.

Ah, that's a relief :) So I/we do not have to invent something new for the ktexteditor preview plugin use-case, good.

As to existing implementations of this stuff, it just happens that I wrote one recently, see konqueror.git branch webengine_streaming, commit e1f9ebc30

:) Good again, implementation pattern looks pretty similar, encouraging.

"how would zoom and other custom state properties be save and retrieved again" -> using BrowserExtension's saveState/restoreState as usual, no? I'm not 100% sure about the interaction with streaming, but normally that happens after opening the url anyway, so it should be unrelated.

Oh, somehow missed those methods. Possibly was blinded by KParts::OpenUrlArguments::xOffset()/yOffset() and since then assumed that state restoring was supposed to be done only via those arguments.
Hm... not perfect possibly: for the ktexteditor preview plugin I plan in the future to also support restoring state, when switching preview between files or for app session restoring support. The kparts there are created not with "Browser/View" option, given the preview plugin does not support navigation. And for some reasons at least in my own kpart implementations (okteta, kmarkdownwebview) I assumed one would only create a BrowserExtension subclass instance if the part is created with "Browser/View" option? lxr.kde.org now shows me any other kpart implementation (at least kmplayer, kbibtex, dolphin, gwenview, okular) create the instance unconditionally.
So guess I should not be blinded by the name "BrowserExtension" and think it is only for browser/view usage, but instead think more of it as "extension, motivated by needs at least in browser but also elsewhere"? And only enable/activate the browser-integration specific parts if the part is created with "Browser/View"?

In D7580#142971, @kossebau wrote:

"how would zoom and other custom state properties be save and retrieved again" -> using BrowserExtension's saveState/restoreState as usual, no? I'm not 100% sure about the interaction with streaming, but normally that happens after opening the url anyway, so it should be unrelated.

Oh, somehow missed those methods. Possibly was blinded by KParts::OpenUrlArguments::xOffset()/yOffset() and since then assumed that state restoring was supposed to be done only via those arguments.

That's just the default implementation of BE::restoreState(), which loads url+offsets and calls setArguments before openUrl. But this is virtual, so more can be done.

Hm... not perfect possibly: for the ktexteditor preview plugin I plan in the future to also support restoring state, when switching preview between files or for app session restoring support. The kparts there are created not with "Browser/View" option, given the preview plugin does not support navigation.

I'm not sure what's the relation. If you need any functionality from BrowserExtension, make sure it's created, I don't see how that means the app must "support navigation".

And for some reasons at least in my own kpart implementations (okteta, kmarkdownwebview) I assumed one would only create a BrowserExtension subclass instance if the part is created with "Browser/View" option? lxr.kde.org now shows me any other kpart implementation (at least kmplayer, kbibtex, dolphin, gwenview, okular) create the instance unconditionally.

KHTML (which is a more interesting example because it shows original intent by the designers of KParts, rather than what other people might have interpreted later) also creates the extension unconditionally. After all, it's just a child object, it doesn't cost much to create it, even if the app will not make use of it. The servicetype Browser/View was more about selecting a different kxmlgui file.

So guess I should not be blinded by the name "BrowserExtension" and think it is only for browser/view usage, but instead think more of it as "extension, motivated by needs at least in browser but also elsewhere"?

Yeah that's more like it. Originally the idea was KParts::ReadOnlyPart is as basic as possible, and extensions provide additional functionality, the browser being the first user, and other uses being free to require different extensions, but of course further thinking would have shown that extensions should be rather named after the functionality they provide than one specific use case. Other use cases can very well have the same needs, so one extension per use case is kind of stupid (code duplication).

And only enable/activate the browser-integration specific parts if the part is created with "Browser/View"?

No, I wouldn't recommend that. Create the extension always, let the app decide whether to use it.

In D7580#144414, @dfaure wrote:

In D7580#142971, @kossebau wrote:

"how would zoom and other custom state properties be save and retrieved again" -> using BrowserExtension's saveState/restoreState as usual, no? I'm not 100% sure about the interaction with streaming, but normally that happens after opening the url anyway, so it should be unrelated.

Oh, somehow missed those methods. Possibly was blinded by KParts::OpenUrlArguments::xOffset()/yOffset() and since then assumed that state restoring was supposed to be done only via those arguments.

That's just the default implementation of BE::restoreState(), which loads url+offsets and calls setArguments before openUrl. But this is virtual, so more can be done.

Okay, matches the picture I have got meanwhile once I read more through the API dox and code (history) of Konqeror, KHTML and KParts.

Though sadly there is a bummer with void BrowserExtension::restoreState(QDataStream& stream):

Used by the browser to restore the view in the state it was when we left it.
If you saved additional properties, reimplement it but don't forget to call the parent method (probably first).

When I read this initially, I guessed this method is just about the view state. But is also bound to data-pulling by the kpart, given that the default implementation explicitely calls openUrl() with the url stored in the datastream. Which might make sense for simple-to-use API with the non-stream use cases. But leaves out the stream-based data-pushing usage.

Seems I have to sit down and draft some generic stream usage test cases and derive from that some proposal for a KParts API extension of some kind. No resources for that right now, might only do later this year. So no session support or view state memory in the KTextEditor Preview plugin then for now, too bad, would have been nice to have.

And only enable/activate the browser-integration specific parts if the part is created with "Browser/View"?

No, I wouldn't recommend that. Create the extension always, let the app decide whether to use it.

Okay, thanks for clarification.

Will complete this patch next to at least support classic browser view state restoring as via BE::restoreState().

BTW: can you tell if Falkon will/does support kparts?

In D7580#144518, @kossebau wrote:

When I read this initially, I guessed this method is just about the view state. But is also bound to data-pulling by the kpart, given that the default implementation explicitely calls openUrl() with the url stored in the datastream. Which might make sense for simple-to-use API with the non-stream use cases. But leaves out the stream-based data-pushing usage.

It does indeed. Two incompatible features...

I think all we need is for the part to remember that it opened the URL via the stream api, and add that to the data saved by saveState(). Then in restoreState() we can skip openUrl() when that bool is true. It'll be up to the caller to redo the openStream/writeStream/closeStream sequence.

BTW: can you tell if Falkon will/does support kparts?

It doesn't, and I don't think it will, but you can try to convince David Rosca ;)

In D7580#144557, @dfaure wrote:

In D7580#144518, @kossebau wrote:

When I read this initially, I guessed this method is just about the view state. But is also bound to data-pulling by the kpart, given that the default implementation explicitely calls openUrl() with the url stored in the datastream. Which might make sense for simple-to-use API with the non-stream use cases. But leaves out the stream-based data-pushing usage.

It does indeed. Two incompatible features...

I think all we need is for the part to remember that it opened the URL via the stream api, and add that to the data saved by saveState(). Then in restoreState() we can skip openUrl() when that bool is true. It'll be up to the caller to redo the openStream/writeStream/closeStream sequence.

Might work, yes. One issue: changing the content layout of the state QDataStream is currently not protected by any versioning, so existing stored history (like from session restore) would be parsed wrongly if suddenly the base implementation of restore also tries to extract such a bool? Might need some handling, though not sure if serious data would be lost.

As said, myself might look into improving this only later this year, so if you (or someone else) feels inspired to play with this, please go ahead already.

BTW: can you tell if Falkon will/does support kparts?

It doesn't, and I don't think it will, but you can try to convince David Rosca ;)

Too bad. Having a cross-media/format navigator like with Konqueror had been one of the things that attracted me to KDE.
Okay, so nothing where I should extend my testing coverage to.

With the latest update then this patch would represent the blue-print for KParts plugins as I see it when it comes to supporting both state restoring and support for streams, at least for what is possible currently with the existing KParts API.

Any patterns which you see running against the ideas of the API? Or can this patch be applied as is, and be pointed to as template?

In D7580#145144, @kossebau wrote:

With the latest update then this patch would represent the blue-print for KParts plugins as I see it when it comes to supporting both state restoring and support for streams, at least for what is possible currently with the existing KParts API.

Any patterns which you see running against the ideas of the API? Or can this patch be applied as is, and be pointed to as template?

@dfaure Ping?

Sorry to be a bit pushy, but begin of upcoming week I plan to release 0.1.0 of the ktexteditor preview plugin, and I would like to have the svgpart code in master usable as reference for hopefully further kpart adaptions or even new implementations inspired by the preview plugin (and as reference for a new kpart plugin kapptemplate I want to create).

Thanks for quick reaction :) Will push now, even if we discovered another item which needs some more clarification. Would do a follow-up fix then if we find one is needed,