[Feature] Live document preview plugin
ClosedPublic

Authored by kossebau on Aug 21 2017, 3:57 PM.

Details

Summary

KTextEditor plugin (to support at least both Kate & KDevelop)
which (pre)views the current state of the editing copy of a text file
in the target format in a toolview, using KParts.

For detailed introduction see
https://marc.info/?l=kwrite-devel&m=150333078206625

Diff Detail

Repository
R40 Kate
Branch
previewplugin
Lint
No Linters Available
Unit
No Unit Test Coverage
kossebau created this revision.Aug 21 2017, 3:57 PM
Restricted Application added a subscriber: kwrite-devel. · View Herald TranscriptAug 21 2017, 3:57 PM

Thats kind of cool and given its even only very little code and a plugin I would like to have it, if others don't disagree.

For the code: I kind of like would to have at least some minimal documentation what the individual members are and do ;=()

kossebau updated this revision to Diff 18520.Aug 22 2017, 1:08 AM

update to latest fixes:

  • reused temporary file not truncated
  • improved handling of locked state
sars added a subscriber: sars.Aug 22 2017, 5:45 AM

Nice plugin :)

I just tried the patch and noticed that after editing a html file and the preview is updated the focus is lost and further editing is no longer possible until the focus is restored to the view.

This is great!

In D7455#138324, @sars wrote:

Nice plugin :)

I just tried the patch and noticed that after editing a html file and the preview is updated the focus is lost and further editing is no longer possible until the focus is restored to the view.

I have a similar issue.
Using the https://phabricator.kde.org/D7431 patch which adds RTF support to Okular.
Whenever I edit an RTF document I have to wait for Okular to render the document. Focus is not lost, but the text edit view freezes while Okular renders (or converts the document).
This probably affects other document types that can be opened with Okular.

kossebau updated this revision to Diff 18536.Aug 22 2017, 1:07 PM

add some api dox, better variable names

In D7455#138324, @sars wrote:

Nice plugin :)

I just tried the patch and noticed that after editing a html file and the preview is updated the focus is lost and further editing is no longer possible until the focus is restored to the view.

So parts being focus-greedy and stealing it, just because they loaded a document? Hm. not so nice of them. Which html kpart are you using, the webkit or the webengine one? Guess I should add a qCDebug to help with that :)
No idea yet how to prevent this in general.

Guess I should add a qCDebug to help with that :)

You could also consider adding the KPart's about action to Kate's help menu

No idea yet how to prevent this in general.

Maybe QWidget::setAttribute(Qt::WA_ShowWithoutActivating); helps?

This is great!

In D7455#138324, @sars wrote:

Nice plugin :)

I just tried the patch and noticed that after editing a html file and the preview is updated the focus is lost and further editing is no longer possible until the focus is restored to the view.

I have a similar issue.
Using the https://phabricator.kde.org/D7431 patch which adds RTF support to Okular.
Whenever I edit an RTF document I have to wait for Okular to render the document. Focus is not lost, but the text edit view freezes while Okular renders (or converts the document).
This probably affects other document types that can be opened with Okular.

One feature I have on my TODO list is to allow disabling the live updates and only update on-demand. That should help with cases where the rendering is expensive or distracts.

Though the kparts in general also need to be improved surely :) Am preparing a list where kparts want to be better when also used here in this preview, using the SVGPart and kuiviewerpart as my example kparts.

Thats kind of cool and given its even only very little code and a plugin I would like to have it, if others don't disagree.

For the code: I kind of like would to have at least some minimal documentation what the individual members are and do ;=()

"individual members" means class members or components/classes of this plugin? :)
Added some initial API dox, so please now point out where exactly you would like some more info, where the code itself is not self-explaining enough or not following known patterns.

BTW, I would like to have some minimal replies to https://marc.info/?l=kwrite-devel&m=150333078206625&w=2 :)

Guess I should add a qCDebug to help with that :)

You could also consider adding the KPart's about action to Kate's help menu

Good idea. Or to the toolview context menu, will consider.

No idea yet how to prevent this in general.

Maybe QWidget::setAttribute(Qt::WA_ShowWithoutActivating); helps?

Thanks, never seen before, will try later.

I only have minor nitpicks, all in all the code looks very clean, good work.

One more serious thing: Did you try in Kate with multiple mainwindows (View > New Window)? I think this would be a good test to stress the plugin a bit more. From my POV, this can go in. Of course, would be nice to get the focus issue sorted out, but this can still be done in a separate commit.

addons/preview/kpartview.cpp
38 ↗(On Diff #18536)

Maybe 500ms are also ok? Then again, maybe 300ms are also fine if it does not matter...

addons/preview/kpartview.h
20 ↗(On Diff #18536)

This #defined sounds almost too general (like are there other KPartViews? Maybe make it KTEXTEDITOR_KPARTVIEW_H

42 ↗(On Diff #18536)

comma istead of full stop: , both ...

64 ↗(On Diff #18536)

lowercase ownership.

One more serious thing: Did you try in Kate with multiple mainwindows (View > New Window)? I think this would be a good test to stress the plugin a bit more.

Tested it a little now that you mentioned it, but could not see any flaws. Besides knowning that previews for the same document have another copy due to singleviewsingledoc model of kparts, oh well (still hoping for a kpart 2.0 one day :) ).

@dhaumann @cullmann @sars @progwolff Happy that you embrace the preview plugin idea and this code with open arms :)
Just: you have been jumping straight to the dessert and skipping the main dish (which I admit needs a little digesting power)! This code here was more meant to give you an idea how the current approach by the preview plugin is like. While actually I was hoping you first would give feedback on the "RFC: Live document preview plugin (technical preview (pun-o-pun))" email I had sent. (My mistake: should have made this not-to-miss in diff title and setting a flag).

This code here still has some rough edges (like the already mentioned missing feature to disable live preview for expensive rendering, but also things like needed proper communication with the kparts to restore view position on a reload/update, so the preview does not jump to 0,0 offset or 100% zooming every time). I have been planning to sort this out while discussion is happening on the topics in that email, especially on creating a separate repo for shared ktexteditor-plugins :)

So please take some time and some drink and consume that email. Fine if you just give feedback on some of the items there :)

kossebau planned changes to this revision.Aug 23 2017, 9:33 AM
kossebau retitled this revision from [Feature] Live document preview plugin to [WIP] [Feature] Live document preview plugin.

As I am not in this mailing list, I will give you some feedback here.

who is in for having a live preview when editing some Markdown file? Or Dot
graph sources, SVG sources, Qt UI sources, you name it...

Especially for UI files this is a great idea. I never liked it having to open up QtCreator just for moving a button one column to the right. With this plugin it can be done inside the text editor, yet with visual feedback that helps making sure the file is still valid

Using KParts plugins:

In my perfect world, any rendering of the target format (like SVG) would be
done from a target document built over the source document, enriched with
tags with info which parts of the target document are created from which
parts of the source document, so there could be a mapping. E.g. to highlight
the counterpart in the source/target when selecting something. And of course
the target model would also support multiple target views.
Just, all that would need people coding this first for any document type. And
right now we do not even have simpler plugins/kparts for all interesting
types (like I had to write a quick'n'dirty kpart for markdown).

A little OT, but you might want to review D7382

So for a
pragmatism-driven initial solution, going for kparts with their single-model-
single-view/widget design and the existing implementations solves quite a few
use-cases now.
And reusing kparts instead of own preview plugins will result in sharing
improvements made to the individual kparts with everyone else using them.
But also see below on some kpart-using issues.

I think using KParts is a good idea. Maintaining individual plugins for the proposed feature is not feasible.
Also, with KParts this feature can support future document types which we don't think about now. There will be no need in updating the preview widget. A new KPart can be used without changes.

Making it a tool-view plugin:

Given the MDI UI for text documents in Kate & KDevelop, together with the
options for split views, there might use-cases to have previews for more than
one document at the same time. One option could be to have the preview as
part of the document view element, like the text-search-bar or the I/O
message boxes. The other option is to have a tool-view which shows the
preview of the currently focussed document (not sure if there could be
multiple ones per tool-view type?).
Being familiar with writing tool-view plugins and a tool-view not needing to
write additional code for showing/hiding the preview or selecting the
position, I for now implemented it as tool-view. A lock-to-document toggle
option allows to cover the use-case of having preview for a given document,
while switching focus to other document views. Surely could have different
solutions one day.

I think it would indeed be better to have a preview for each document.
Context switching between mutiple documents with only one preview would probably result to waiting times and/or a high CPU load when the corresponding KPart opens/converts/generates/renders the document.

Correctly working only for single source document -> single target document:

There are target documents which make use of multiple source documents (e.g.
HTML page pulling in CSS files). There could be some (k)io-mechanism to
connect to other loaded documents and get their live data, but that needs
some infrastructure work first. As there is no metadata in kparts available
to filter out such multi-source document cases, there is no protection
against partially broken previews right now, as e.g. can happen with HTML
files. Not sure what to do here, so done nothing :)

Guess this can be put aside until a first version of this plugin is accepted

Avoiding to stress the filesystem:

Best preview often is continuous live preview. And when one types a letter
and wants a preview update, ideally only the one-letter diff would be passed
to the preview module and that would update the preview accordingly.
As a matter of fact though all kparts currently have no idea about the
KTextEditor working memory model (i.e. the KTextEditor API). For I/O of
documents/files they themselves only support reading complete blobs from the
filesystem (with KParts module transparently handling remote urls via
temporary files passed to the actual kpart plugin). While the KPart API has
some alternative streaming methods (ReadOnlyPart::openStream() & Co) as
option to implement, seems no (interesting) kpart supports it (cmp. https://
lxr.kde.org/ident?_i=doOpenStream).
With the markdown kpart I am already using the streaming API. For any other
kpart that ideally should be added as well, not sure why there is no
primitive default implementation instead.
Right now the preview plugin first tries the streaming API (as only the call
to openStream() would tell if streaming is supported?) and then falls back to
passing the data blob via a QTemporaryFile.
Not sure how much of an issue that is.

Care must be taken here to pass the correct mime type to the KPart.
E.g. Okular can open different document types and falls back to plain text if the mime type is unknown.

Knowing what preview is wanted:

Right now selection of the kpart to use for the preview is done based on
"KTextEditor::Document::mimeType()". Which only seems to work for documents
which already are stored as file in the filesystem.
Any proposal how to fix this for new documents not yet saved? Something based
on currently selected syntax highlighting?

Selected Mode and Highlighting are the only hints available, I guess.
To automatically determine the mime type of a file whithout a set highlighting/mode is out of scope of this plugin. Might be something Kate could do in general.

Selecting KParts plugin to use:

Using the mimetype string, the preview code currently simply queries the
currently configured preferred service:
"KMimeTypeTrader::self()->preferredService(mimeType, "KParts/ReadOnlyPart");
Which might be an issue in non-Plasma workspaces, given they do not have any
configuration options for kparts in their system settings UI.
Perhaps also people would like to prefer a different kpart for the live
preview? Needs more thinking how to enable any configuration.
Next there is also the issue that with the mimetype "text/plain" we get the
kate part as preview module :) As first primitive approach the code for now
simply filters out any kparts which list "text/plain" in their supported
mimetypes. But that runs the chance of blocking also kparts which support
importing plain text into whatever richer format they are about. Like the
Okular plugin.

Plus one might actually want to preview a text/plain document in Okular.
Maybe just blacklist katepart?

Where should the preview plugin sources live?

The Preview plugin is similar to the Snippets plugin, usable both by Kate &
KDevelop (as marked by ServiceTypes=KTextEditor/Plugin,KDevelop/Plugin).
Just, the Snippets plugin is right now part of the Kate sources. Adding the
Preview plugin there as well is slightly unfortunate, as it both means for
the developer working on the plugin they have to checkout and build the whole
of kate (or hack the buildsystem locally) and for the user the chance that
packagers put the plugin in a package which pulls all of Kate and its deps,
even if ones only want the plugin for KDevelop.
So having a separate repo for plugins shared by Kate & KDevelop (and perhaps
other KTextEditor apps?) would be nice to have.
I learned there once was a thread started on this ml discussing such a
separate repo, on Aug 25, 2015, "Moving KTextEditor plugins out of kate.git?"
http://markmail.org/message/x7e5aq4c4he57pzk#query:+page:
1+mid:s4x3dl6ztbhjujpb+state:results
Time to continue it almost exactly 2 years later :)

Sounds like a good idea to me. But this is something the core developers of these projects have to discuss.

kossebau marked 2 inline comments as done.EditedAug 23 2017, 11:57 AM

((Opps, added the inline replies in another window that somehow had the already made comment still in the text field, so it slipped in again when I clicked Submit. Removed.))

addons/preview/kpartview.cpp
38 ↗(On Diff #18536)

I found 500ms to be too much for some Markdown editing I used it with, that was a delay that made me thinkg "come on, update" when I typed in the sources and peeked over to the preview to see how things are shaping up.
But that was just me, surely could get some more serious research what is a proper (default) value.
Perhaps the whole update logic cpuld also be overhauled, to only do an update once there was at least xxx ms idle time. And otherwise update in bigger intervals.
To cover both the use-case peek-over-to-see-current-state, where ~300 ms might be what it takes to refocus eyes, and the use-case see-in-the-corner-of-eyes-something is changing-while-editing, where 500-2000(?) ms might be good enough.

Plan to also add a spinner icon to reflect the state going-to-sync-in-a-moment-not-uptodate-yet.

addons/preview/kpartview.h
20 ↗(On Diff #18536)

Indeed., keeping in mind. Though actually the class name itsellf might also see an improvement, not too happy with it yet. Any proposals?

kossebau added a comment.EditedAug 23 2017, 12:58 PM

As I am not in this mailing list, I will give you some feedback here.

Thanks for that!

A little OT, but you might want to review D7382

Sorry,, no time for that :/

Making it a tool-view plugin:

I think it would indeed be better to have a preview for each document.
Context switching between mutiple documents with only one preview would probably result to waiting times and/or a high CPU load when the corresponding KPart opens/converts/generates/renders the document.

Indeed. So hopefully the usage experience collected with the tool-view based solution will inspire people to go that step further one day :)

Avoiding to stress the filesystem:

Care must be taken here to pass the correct mime type to the KPart.
E.g. Okular can open different document types and falls back to plain text if the mime type is unknown.

The stream API of KParts requires to pass the wanted mimetype, but Okular right now only supports the file-based one, where the direct calls do not require an explicit mimetype being passed. Can you tell if the Okular kpart inspects the ReadOnlyPart::arguments() and reads the mimeType property?
Local changes pass this already, not yet committed, part of other experiments.

Knowing what preview is wanted:

Selected Mode and Highlighting are the only hints available, I guess.
To automatically determine the mime type of a file whithout a set highlighting/mode is out of scope of this plugin. Might be something Kate could do in general.

Yes, I expect this from Kate or rather KTextEditor. Would be also handy when going to save a new file the first time, to have the proper mimetype/file suffix already applied,

Selecting KParts plugin to use:

Plus one might actually want to preview a text/plain document in Okular.

If they want, they should be able, whatever I think about this :)

Maybe just blacklist katepart?

Might also work as pragmatic hack. There shall be no other text kpart!1! :)
A more general long-term solution might be to make some distinction between "source" editors/viewers and "final"(?) format editors/viewers.
That would also help in other cases where a choice from plugins/apps is made for a given mimetype, where one is not interested in lower levels of presentation (say text file->katepart or octet-stream -> oktetapart), but only in the final/target display (Qt UI file, SVG, etc). Like with thumbnailers or preview (ha) for file managers. Might need to be also noted with the mimetypes, where currently being a subtyp does not tell if the base type is used as container format or as base format where the subtype just has some specialisation on.

kossebau updated this revision to Diff 19239.Sep 6 2017, 3:33 PM

update to latest version:

  • option to do manual preview updates instead of automatic ones
  • be inactive if being hidden
  • initial session restore support
  • namsespace for simple helper classes
  • small code shuffling for cleaner structure
kossebau retitled this revision from [WIP] [Feature] Live document preview plugin to [Feature] Live document preview plugin.Sep 6 2017, 3:57 PM
kossebau edited the summary of this revision. (Show Details)

Thanks everyone for your feedback so far. I hope I have picked everything up, either as fix or as TODO item in the code. Please check again if any fix matches your expectations and any TODO item covers your needs.

While there are lots of improvements possible, the current state has worked good enough for me in real world usage. It is now the different kparts which would need to see more work to improve their usage in this preview plugin. So I would like to get the current state of the preview plugin out to people to share the joy of this feature (& to work on improving it :) ) and have a motivation/testing option to also improve the kparts they (would) use.

Given KA 17.12 is still some months away, I am tempted to make some initial stand-alone release of the plugin for now, and have it then merged into the kate repo (or some ktexteditor-addons if people create that). That would also allow/motivate people to fix kparts already now, even if they do not run kate from master.

What do you think?

When it comes to improving kparts, myself I have already adapted kuiviewer part (master branch) and, as reference implementation, uploaded a patch for the SVG kpart in D7580. And there is the Markdown kpart, which was extra created for this, and which is going to see a first release Sept 12th.

kossebau updated this revision to Diff 19244.Sep 6 2017, 4:40 PM

add missing code commnent not synced to disk before creating diff

kossebau added a comment.EditedSep 6 2017, 4:42 PM

BTW, this patch is as before just a maintained copy the same code in kde:ktexteditorpreviewplugin (update Sep. 17th: no longer a scratch repo, but normal one). So for testing you can as before just clone and build that repo.

Given KA 17.12 is still some months away, I am tempted to make some initial stand-alone release of the plugin for now, and have it then merged into the kate repo (or some ktexteditor-addons if people create that). That would also allow/motivate people to fix kparts already now, even if they do not run kate from master.

Given no feedback my temptation now wins over me. So preparing some independent release(s) now for the start.

cullmann accepted this revision.Sep 17 2017, 12:42 PM

Given no feedback my temptation now wins over me. So preparing some independent release(s) now for the start.

Good idea.

In any case: I think it is ok to merge this, given its an optional plugin, it can't hurt to have it in our git and out for people with nightly builds.

This revision is now accepted and ready to land.Sep 17 2017, 12:42 PM

Given no feedback my temptation now wins over me. So preparing some independent release(s) now for the start.

Good idea.

In any case: I think it is ok to merge this, given its an optional plugin, it can't hurt to have it in our git and out for people with nightly builds.

Thanks for +1 for merging.

Though I would like to delay this to around 1st of November (before KA freezes start), so I do not have to care about syncing the code between the repos.
I am currently preparing a 0.1.0 release of the plugin at Sept. 24th/25th, which should help to make this code not only available to people with nightly Kate builds, but a wider audience.

@cullmann, @dhaumann : okay if I add a component "plugin-preview" to product "kate" on bugs.kde.org already now, so issues are already tracked at the potential target place? (can do myself, jumped on a diamond once which gave me the powers ;) )

@cullmann, @dhaumann : okay if I add a component "plugin-preview" to product "kate" on bugs.kde.org already now, so issues are already tracked at the potential target place? (can do myself, jumped on a diamond once which gave me the powers ;) )

I will map no comment to an "okay" this Sunday then, unless you object to it before :)

Yes, that is ok.

Thanks, so doing now already (have three bugs I need to file to myself already)

This revision was automatically updated to reflect the committed changes.