Sigil-0.9.13 Released

[BeckyEbook]

Originally Posted by theducks

With 9.13 you SEE find next switch files, then scroll

I'm pretty sure that "guilty" is this commit.

@DiapDealer: Get from AppVeyor history 0.9.13-100 and 0.9.13-101.
0.9.13-100 is last "good". Preview is not reloaded at each change of position the cursor in the Code View (also with any search, because then, of course, also the position of the cursor changes).
From 0.9.13-101 forced call clearMemoryCaches() makes edit epub with preview (refresh, refresh, refresh…) uncomfortable.

How reproduce problem?
1. Open current version Sigil_User_Guide_2019.02.15.epub file
2. Go to "preferences.html" file.
3. Open Preview window.
4. In Code View find "image" and press "Find" and again, again, again...

In 0.9.13-100 Preview is refreshed immediately (of course depending on the computer, memory, etc.)
From 0.9.13-101 Preview is loading whole file again and only then jumps to the right place to display them. It takes precious fractions of seconds and we see an annoying blinking preview window.

Maybe this problem only exists in Windows? If MacOS/Linux better handle cache then just add the condition.

I really understand the need keep memory footprint small, but problem is clearly in BookViewPreview.cpp

Current code:

    // If this is not the very first load of this document, store the caret location
    if (!url().isEmpty()) {
        StoreCurrentCaretLocation();
        // to help keep memory footprint small clear any memory caches when a new page loads
 	if (url().path() != path) {
	    settings()->clearMemoryCaches();
	} 
    }

I built a version for x64 from the latest sources.
After commenting on the red line -- Preview returns to normal, although the memory usage increases as the editing progresses.
I know that this is not the perfect solution.

I think it needs to be detected differently if we are in CodeView still in the same file.

[DiapDealer]

We'll certainly look into that commit, but I don't want to start conflating different issues. A major performance issue was reported by a few after we released 0.9.11 (the first version to update Qt/QtWebKit and Python in quite some time). I'd like to make sure we've resolved THAT issue before we start looking at later memory foot-print reducing strategies and their impact.

@DiapDealer: Get from AppVeyor history 0.9.13-100 and 0.9.13-101.

No need. I'm always running the latest Appveyor build on one machine or another (currently 0.9.13-131). Which contains the commit in question. Making editing with Preview uncomfortable, I get. We'll work on that. But that's not a performance issue or a slow down in Sigil's response time (or a "not responding" situation). Which is what we're mainly trying to address.

So my question to theducks is simply this: is Sigil 0.9.13 itself slowing down for you, or is it simply doing something differently with regard to Preview synchronization that you find too annoying to continue using?

I'm not dismissing this issue. I just want to make sure it's not getting lumped in with the general performance/resource problem some are experiencing. Thanks, @BeckyEbook for providing a clear explanation of the issue being experienced. I appreciate the research.

EDIT: one doesn't even need to use the "Find" button. Clicking in the text in Code View will cause the jump to the top of the page before scrolling back to the correct position. But only if there's embedded fonts or images in that file. The text-only TOC, for instance, doesn't do any of that "flashing" when Preview is being synchronized to Code View.

[KevinH]

The point that seems to be missed in this discussion is that Preview ALWAYS loads the entire file each time. It does this everytime a change is made in code in the CodeViewEditor. It always has.

The clearing of the memory caches is only done if you ask to Preview a file that is from a different CodeViewEditor Tab/Resource. If you look at the code it checks the url against the currently loaded one to determine if it is okay to clear memory caches that hold fonts, javascripts, images and other resources. The current cache is worthless in that case anyway.

In addition, unlike a browser, all files are local and there is no network delay to load them. So most file io uses ram buffers anyway, so that the page cache is redundant in these cases.

So clearing of memory caches when they are not needed for the page you are about to load will help free up memory.

It does in fact help keep the memory footprint from constantly growing which is the issue we are trying to solve.

So please as DiapDealer said, let us separate annoyance from screen flashing to load a new position in Preview from a system slowdown that cause the wait cursor to appear and the system to become unusable until Sigil and the computer is restarted.

Also Becky, what Windows 10 update and exact build are you using? Are you seeing Sigil's memory use growing to over 1 gig from just an hour or so from working with it?

[DiapDealer]

Originally Posted by KevinH

The point that seems to be missed in this discussion is that Preview ALWAYS loads the entire file each time. It does this everytime a change is made in code in the CodeViewEditor. It always has.

Does merely moving the cursor to a new location in the current Code View Editor tab constitute a code change? I ask because that's what we seem to be experiencing on Windows. With files that contain images/embedded fonts, you can watch Preview reload everything (including a move to the top of the page), and then scroll down to the new cursor position just by repositioning the cursor in the current Code View Editor tab.

Originally Posted by KevinH

The clearing of the memory caches is only done if you ask to Preview a file that is from a different CodeViewEditor Tab/Resource. If you look at the code it checks the url against the currently loaded one to determine if it is okay to clear memory caches that hold fonts, javascripts, images and other resources. The current cache is worthless in that case anyway.

I can see from the code that that's what should be happening, but I'm not certain it's working as expected on Windows. As mentioned above, merely moving the cursor in the current Code View Editor tab seems to trigger a clearing/reloading of the memory caches that hold fonts, javascripts, images, etc...

I'll try to get some debugging output after work this evening to make certain of what's going on.

EDIT: just verified that the same effect doesn't happen on Linux. So if I had to guess, I'd say there's something about the windows paths being bandied about that is making:

if (url().path() != path)

Always resolve to True on Windows.

But yes .... this is all still a completely separate issue from the severe Windows 10 performance problems being reported by a handful of people that we've been trying to track down.

[KevinH]

I bet one version is using unix style path separators and the other is using windows path separators or one is escaped and one is not. If so, this should be an easy fix. So when you get a free moment, please use qDebug() to output both paths so that we can see what is actually going on.

So no, it should never flush the cache until it is asked to load a different tab. Moving a cursor should do nothing. Even editing in the same tab should do nothing, making the cache actually useful.

As soon as we see why that url path differs from path only on Windows we should be able to fix it (even if we need to tell Preview to remember its last url path itself).

[theducks]

Originally Posted by BetterRed

Apart from getting yourself sectioned did disabling it overcome the performance issues?

BR

It helped. Not quite as fast a 9.9, but tolerable.
windows changes

[DiapDealer]

Originally Posted by KevinH

I bet one version is using unix style path separators and the other is using windows path separators or one is escaped and one is not. If so, this should be an easy fix. So when you get a free moment, please use qDebug() to output both paths so that we can see what is actually going on.

So no, it should never flush the cache until it is asked to load a different tab. Moving a cursor should do nothing. Even editing in the same tab should do nothing, making the cache actually useful.

As soon as we see why that url path differs from path only on Windows we should be able to fix it (even if we need to tell Preview to remember its last url path itself).

It doesn't seem to be the path separator. I pushed some debug prints to my Sigil Fork from work and than ran the Appveyor build created from the commit.

On Windows, there seems to be a forward slash prepended to url().path() that doesn't exist in the passed-in "path" parameter. So they're never equal. For example on the default epub Sigil creates upon opening:

url().path() = /C:/Users/FPC PC/AppData/Local/Temp/Sigil-SKylwq/OEBPS/Text/Section0001.xhtml

and the path parameter = C:/Users/FPC PC/AppData/Local/Temp/Sigil-SKylwq/OEBPS/Text/Section0001.xhtml

Hence the decision to clear the cache is being made every single time.

Perhaps:

if (url().toLocalFile() != path)

would be a better cross-platform comparison?

[theducks]

Thanks for the info Becky
I (always) run with preview open on a second monitor. (various tools, like spellcheck,reports, saved searches, are also run on other monitors)
I love to spread things out

[KevinH]

Well that is a Problem!!

We can alawys special case this just for Windows and remove the beginning slash on the url part or prepend the initial / to the path part before comparison.

A QUrl path() should be to a local file in this case and should be the same url path returned by WebKit but it is obviously not on Windows. The path is the one from Webkit which does not have leading / on Windows.

I would just special case this for Windows and adjust one to properly match unless you know which of these two are technically incorrect?

We should probably push out a new Windows build (or point people at the appveyor builds) to stop this annoying behaviour.

Thanks for tracking this one down!

KevinH

Originally Posted by DiapDealer

It doesn't seem to be the path separator. I pushed some debug prints to my Sigil Fork from work and than ran the Appveyor build created from the commit.

On Windows, there seems to be a forward slash prepended to url().path() that doesn't exist in the passed-in "path" parameter. So they're never equal. For example on the default epub Sigil creates upon opening:

url().path() = /C:/Users/FPC PC/AppData/Local/Temp/Sigil-SKylwq/OEBPS/Text/Section0001.xhtml

and the path parameter = C:/Users/FPC PC/AppData/Local/Temp/Sigil-SKylwq/OEBPS/Text/Section0001.xhtml

Hence the decision to clear the cache is being made every single time.

Perhaps:

if (url().toLocalFile() != path)

would be a better cross-platform comparison?

[KevinH]

The first url passed to Preview is generated in MainWindow.cpp here:

m_PreviewWindow->UpdatePage(html_resource->GetFullPath(), text, location);

Perhaps we should convert that absolute path more properly to a QUrl before passing it in to be usd by Preview to update itself?

Would that avoid this problem?

[DiapDealer]

Originally Posted by KevinH

The first url passed to Preview is generated in MainWindow.cpp here:

m_PreviewWindow->UpdatePage(html_resource->GetFullPath(), text, location);

Perhaps we should convert that absolute path more properly to a QUrl before passing it in to be usd by Preview to update itself?

Would that avoid this problem?

It should. That way we could compare url().path() to path.path() safe in the knowledge that we're comparing the same object types.

For what it's worth, I've verified that

if (url().toLocalFile() != path)

Fixes the problem on Windows. It should on Mac/Linux as well, I should think. But we could attack it from the other direction as you've suggested, and make sure we're passing a valid QUrl::path() parameter to Preview. Makes no difference to me. Whatever you think is best

[KevinH]

Please just push your fix.

Thanks!

And how hard is it for people having issues wirh this particular bug to try one of your Appveyor automatic builds?

[DiapDealer]

Originally Posted by KevinH

Please just push your fix.

Thanks!

I'll push it to the master branch when I get home. I'd like to double-check that it doesn't break anything on Linux first. If it doesn't, I doubt it would cause problems on a Mac, either.

Originally Posted by KevinH

And how hard is it for people having issues with this particular bug to try one of your Appveyor automatic builds?

Not hard at all. I can post a download link to any of them. Or a link to the Sigil project on Appveyor would allow anyone to download a 64-bit Windows build of pretty much any individual commit.

[KevinH]

Good to know. FWIW, I have tested your Windows fix on macOS and it works just fine.

[DiapDealer]

Originally Posted by KevinH

Good to know. FWIW, I have tested your Windows fix on macOS and it works just fine.

In that case, I'll go ahead and push the fix to master now.