D7580: Support loading by stream and restoring state on reload

[Friedrich W . H . Kossebau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R383:d1ee5a0e3908: Support loading by stream and restoring 
state on reload (authored by kossebau).

REPOSITORY
  R383 SVGPart

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7580?vs=19446=19814

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

AFFECTED FILES
  CMakeLists.txt
  svgbrowserextension.cpp
  svgbrowserextension.h
  svgpart.cpp
  svgpart.h

To: kossebau, #frameworks, dfaure

D7580: Support loading by stream and restoring state on reload

[Friedrich W . H . Kossebau]

kossebau edited the summary of this revision.
kossebau edited the test plan for this revision.

REPOSITORY
  R383 SVGPart

BRANCH
  supportstreamandreload

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

To: kossebau, #frameworks, dfaure

D7580: Support loading by stream and restoring state on reload

[Friedrich W . H . Kossebau]

kossebau added a comment.


  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,

INLINE COMMENTS

> dfaure wrote in svgpart.cpp:191
> When I reload in konqueror (with KHTML or WebEngine), the yOffset is 
> preserved.
> Looking at KonqView::restoreHistory this is because restoreState is only 
> called when reload==false.
> 
> It might be a good idea for kate/kdevelop to do the same, they have the 
> actual information of whether reload was pressed, while parts can only guess.

This is actually another item I am slightly unsure about and you now added to 
this unsureness. The API dox of OpenUrlArguments::reload() 

 flag says:
"part should reload the URL, i.e. the cache shouldn't be used (forced reload). "

So far I asked I would have said this flag responds to whether 
"If-Modified-Since" should be added to the HTTP request header or that ETag be 
used (`reload=false`) or whether not (`reload=true`). And that "reload" should 
have rather been named "forcedReload".

So in the case of Kate/KDevelop (or rather the ktexteditor preview plugin, as 
this is the one who cares about feeding the new document data to the kparts 
plugin), setting the reload flag to `true` would mean the forced reload also of 
embedded document resources, like images, fonts or whatever external resources 
a document might include, right?
But actually we just want the new version of the text document to be loaded 
again. Which either

- is passed via a file, which has a newer filestamp then, thus indicates there 
is newer data inside, or
- is feed via a data stream, where one would assume the pushing side already 
made sure only an updated data version is pushed.

So previously I would have thought `reload=false` would be the correct thing to 
do, as there is no need for forced reload. Have I been missing something?

REPOSITORY
  R383 SVGPart

BRANCH
  supportstreamandreload

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

To: kossebau, #frameworks, dfaure

D7580: Support loading by stream and restoring state on reload

[David Faure]

dfaure accepted this revision.
dfaure added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> svgpart.cpp:191
> +// we can't tell if caller has explicitely set xOffset/yOffset of 
> OpenUrlArguments
> +// so in case of same url we just assume a reload and ignore the 
> OpenUrlArguments xOffset/yOffset
> +if (!mHasExtendedRestoreArguments && (url() == mPreviousUrl)) {

When I reload in konqueror (with KHTML or WebEngine), the yOffset is preserved.
Looking at KonqView::restoreHistory this is because restoreState is only called 
when reload==false.

It might be a good idea for kate/kdevelop to do the same, they have the actual 
information of whether reload was pressed, while parts can only guess.

REPOSITORY
  R383 SVGPart

BRANCH
  supportstreamandreload

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

To: kossebau, #frameworks, dfaure

D7580: Support loading by stream and restoring state on reload

[Friedrich W . H . Kossebau]

kossebau added a comment.


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

REPOSITORY
  R383 SVGPart

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

To: kossebau, #frameworks, dfaure

D7580: Support loading by stream and restoring state on reload

[Friedrich W . H . Kossebau]

kossebau added a comment.


  In https://phabricator.kde.org/D7580#144557, @dfaure wrote:
  
  > In https://phabricator.kde.org/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?

REPOSITORY
  R383 SVGPart

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

To: kossebau, #frameworks, dfaure

D7580: Support loading by stream and restoring state on reload

[Friedrich W . H . Kossebau]

kossebau updated this revision to Diff 19446.
kossebau added a comment.


  add support for state restoring via BrowserExtension

REPOSITORY
  R383 SVGPart

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7580?vs=19091=19446

BRANCH
  supportstreamandreload

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

AFFECTED FILES
  CMakeLists.txt
  svgbrowserextension.cpp
  svgbrowserextension.h
  svgpart.cpp
  svgpart.h

To: kossebau, #frameworks, dfaure

D7580: Support loading by stream and restoring state on reload

[David Faure]

dfaure added a comment.


  In https://phabricator.kde.org/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 ;)

REPOSITORY
  R383 SVGPart

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

To: kossebau, #frameworks, dfaure

D7580: Support loading by stream and restoring state on reload

[David Faure]

dfaure added a comment.


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

REPOSITORY
  R383 SVGPart

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

To: kossebau, #frameworks, dfaure

D7580: Support loading by stream and restoring state on reload

[David Faure]

dfaure added a comment.


  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 
https://phabricator.kde.org/R226:e1f9ebc309b8ceaebfc51884ca30a3a8d8903ad2
  
  "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.

REPOSITORY
  R383 SVGPart

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

To: kossebau, #frameworks, dfaure

D7580: Support loading by stream and restoring state on reload

[Friedrich W . H . Kossebau]

kossebau edited the summary of this revision.

REPOSITORY
  R383 SVGPart

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

To: kossebau, #frameworks, dfaure

D7580: Support loading by stream and restoring state on reload

[Friedrich W . H . Kossebau]

kossebau added a comment.


  > 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::openStream`calls 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...

REPOSITORY
  R383 SVGPart

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

To: kossebau, #frameworks, dfaure

D7580: Support loading by stream and restoring state on reload

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> svgpart.cpp:118
> +mStreamedData.clear();
> +mItem = new QGraphicsSvgItem();
> +mItem->setSharedRenderer(mRenderer);

Shouldn't mItem be deleted first?
You seem to rely on closeUrl() being called before the next openStream, but I 
don't remember that there's any guarantee for that to happen.
As I see it, it's *either* openUrl/closeUrl *or* 
openStream/writeStream/closeStream.

REPOSITORY
  R383 SVGPart

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

To: kossebau, #frameworks, dfaure

D7580: Support loading by stream and restoring state on reload

[Friedrich W . H . Kossebau]

kossebau updated this revision to Diff 19091.
kossebau added a comment.


  handle repeated closeUrl call

REPOSITORY
  R383 SVGPart

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7580?vs=18868=19091

BRANCH
  supportstreamandreload

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

AFFECTED FILES
  svgpart.cpp
  svgpart.h

To: kossebau, #frameworks, dfaure

D7580: Support loading by stream and restoring state on reload

[Friedrich W . H . Kossebau]

kossebau created this revision.

REVISION SUMMARY
  The KTextEditor preview plugin* repeatedly feeds new
  versions to the same kpart instance, to allow instant
  preview of changes. To avoid stressing of the filesystem
  the stream API of the kpart is used if available.
  *https://frinring.wordpress.com/2017/08/21/look-what-you-have-donewwdo/
  
  This patch adds support for the stream API.
  Additionally it remembers the view state on closing an url,
  and if the same url is load again, the view state is restored.
  The latter allows continuous display of the same, but
  updated file as e.g. happening with the preview plugin.
  
  Open questions:
  a) use of the streaming API matching intentions?
  b) Restoring of view state overlaps with support in browser history,
  
for which there is the KParts::OpenUrlArguments property of ReadOnlyPart.
That one only supports x/y offset, so how would zoom and other custom
state properties be save and retrieved again, if they should?
  
  This patch should help to solve this questions in general, so it is
  known what to do for other kparts.

TEST PLAN
  Editing SVG files in Kate/Kdevelop using the preview plugin* will
  no longer reset the offset to 0,0 each time the view is updated on
  changes, also is the filesystem no longer used on updates.
  
  - kde:scratch/kossebau/ktexteditorpreviewplugin

REPOSITORY
  R383 SVGPart

BRANCH
  supportstreamandreload

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

AFFECTED FILES
  svgpart.cpp
  svgpart.h

To: kossebau, #frameworks, dfaure