[patch] Refresh previews on branch toggle?

[Scott Kostyshak]

On Mon, Dec 09, 2019 at 04:14:53PM -0500, Scott Kostyshak wrote:
> On Mon, Dec 09, 2019 at 12:44:20PM -0500, Richard Kimberly Heck wrote:
> > On 12/9/19 10:40 AM, Scott Kostyshak wrote:
> > > On Sun, Dec 08, 2019 at 11:03:20PM -0500, Richard Kimberly Heck wrote:
> > >> On 12/8/19 9:20 PM, Scott Kostyshak wrote:
> > >>> Attached is a patch that refreshes previews whenever the user activates
> > >>> or deactivates a branch. The motivation for this patch is that if a
> > >>> child document is previewed in the master document, and if the child
> > >>> document has the branch inside it as well, the preview will be incorrect
> > >>> when the branch is toggled (without this patch).
> > >>>
> > >>> However, this patch will cause all previews to refresh. I'm concerned
> > >>> that the inefficiency is not worth the benefit. Any thoughts on this
> > >>> patch and any thoughts on an ideal solution? For example, would
> > >>> removePreviews() and updatePreviews() ideally have arguments that allow
> > >>> to specify which type of previews to update?
> > >> It makes sense to me.
> > >>
> > >> Do we have to remove previews first?
> > > Good question. It doesn't work if I don't include that line, and it's
> > > consistent with Buffer::reload(). The following contains a clue for why
> > > I think it's needed (copied from RenderPreview.h):
> > >
> > >   /** Remove a snippet from the cache of previews.
> > >    *  Useful if previewing the contents of a file that has changed.
> > >    */
> > >   void removePreview(Buffer const &);
> > >
> > > From the perspective of the master document, the LaTeX export
> > > corresponding to the child document has not changed (it is just
> > > \input{child.tex}). So if we only "update" the previews, I think that
> > > would only regenerate previews for insets where the LaTeX as viewed by
> > > the master document has changed.
> > >
> > > The patch does not actually call the function I reference above. It uses
> > > removePreviews() (note the extra 's'), which creates an entirely new
> > > previewLoader. Perhaps it would be more efficient to loop through the
> > > insets and call removePreview() on them (or only the ones that depend on
> > > a file?).
> > 
> > It probably doesn't matter very much, since all the regeneration happens
> > in the background.
> 
> Good point. Strangely, with my patch there is a period of time where the
> GUI is unresponsive. During the time it is unresponsive, it shows the
> old preview, then it shows no preview, then it shows the new preview. I
> would have guessed that it would immediately show no preview, then it
> would process in the background, then it would show the new preview. I
> am amazed by how good the preview machinery is though, so I think my
> expectation was greedy.
> 
> The delay seems to be (not surprisingly) due to updatePreviews(), not
> removePreviews().

The delay is enough that I don't think the patch should go in to master.
On the one hand, the incorrect preview is annoying and activating a
branch is not that common (so the user would not experience many
delays); but on the other hand I'm not sure how many people run into
this issue (it requires using both preview and child documents and
branches existing in both master and child).

Perhaps a reasonable approach would be to only update the previews if we
detect that the branch exists in a child document.

Scott
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.lyx.org/pipermail/lyx-devel/attachments/20191211/462f0214/attachment.asc>