[2 Bugs] + [Enhancement] Book Browser, CSS, URLs, Oh My! - Page 3

DiapDealer · 10-03-2018, 12:17 PM

We're already closing Preview on all deletes when Book View is open as well (which causes odd--but not really problematic--graphical issues when Preview is tabified with other widgets), so I'd hate to have Book View and Code View flip-flopping in the background as well.

But that would probably be a definitive workaround as opposed to an arbitrary delay in SDeleteFile that may or may not be sufficient for all machines.

Let me play around with both ideas tonight (since you can't recreate the issue) and see what I can find out. Thanks for delving into it.

KevinH · 10-03-2018, 12:27 PM

Sounds good.

FYI, if you want to try the "delay only if delete fails and try again approach", there is a Misc/SleepFunctions.h which is used in MainWindow.cpp and other places to allow a delay to allow other Qt Threads to process. Adding that to SDeleteFiles and only using it if the remove fails may be something to test as loading css files should come early in the webkit loading process.

DiapDealer · 10-03-2018, 01:04 PM

Thanks. I'll check it out.

DiapDealer · 10-03-2018, 05:15 PM

Well it doesn't seem the delayed retry strategy is a viable workaround. I started with a 5 second delay just to see where I stood, and ended up with the same result: only one of the css files was deleted even after the delay. I didn't bother increasing the delay since 5 seconds was already an unacceptable amount of time.

KevinH · 10-04-2018, 11:08 AM

Quote:

Originally Posted by DiapDealer

Well it doesn't seem the delayed retry strategy is a viable workaround. I started with a 5 second delay just to see where I stood, and ended up with the same result: only one of the css files was deleted even after the delay. I didn't bother increasing the delay since 5 seconds was already an unacceptable amount of time.

Wow! I really thought that would do the trick. Maybe we should try adding a call to qApp->processEvents() before the sleep and shrinking sleep time down (all we really need to do is get time for other threads/processes to start).

Or maybe my "sequence of events" guess is all wet. Though it did explain why this only happens in BV (which uses FlowTabs) and not PV.

DiapDealer · 10-04-2018, 05:00 PM

Quote:

Originally Posted by KevinH

Wow! I really thought that would do the trick. Maybe we should try adding a call to qApp->processEvents() before the sleep and shrinking sleep time down (all we really need to do is get time for other threads/processes to start).

You may be on to something, there! I added qApp->processEvents() before a 100ms sleep on the second try and everything seems to work. I added several css files and linked them all to the chapter file that I had open in book view and was able to delete them all successfully. Every single one after the first file needed the the second try to succeed, but they all were deleted.

Next I tried foregoing the second deletion attempt altogether and just put the qApp->processEvents()/100ms sleep before the one and only attempt to delete it. That worked equally well.

So what's your advice? Limit the processEvents/sleep to the second deletion attempt after a failure? Or a wholesale processEvents/sleep with one, and only one, deletion attempt? The former would likely limit any potential delays to the Windows platform since the other two platforms don't seem to need the backup. It seems more robust to me, as well.

KevinH · 10-04-2018, 05:54 PM

I think we should add the process events and sleep only if the first delete attempt fails since this race only impacts images and css that are actually linked to the front Tab whereas SDeleteFiles is usd in many places and with many file types where no sleep is ever needed or wanted.

Nce work!

DiapDealer · 10-04-2018, 06:13 PM

Quote:

Originally Posted by KevinH

I think we should add the process events and sleep only if the first delete attempt fails since this race only impacts images and css that are actually linked to the front Tab whereas SDeleteFiles is usd in many places and with many file types where no sleep is ever needed or wanted.

I agree. I noticed right after posting the above that doing it for every delete noticeable affected the time it took to close Sigil (temp file cleanup). I'm sure it would affect other areas as well.

I'll clean up all the debug print stuff and get the fix pushed in the near future.

Thanks for the assist!

DiapDealer · 10-04-2018, 06:42 PM

Just pushed the fix (hopefully) for bug #2.

Doitsu · 10-06-2018, 06:11 AM

Quote:

Originally Posted by DiapDealer

So far, so good on Windows. No disappearing icons and multiple file moves (including non-contiguous) work as expected. I'll test on Linux later.

I've just tested your latest fix on my Arch Linux machine and the CSS bug no longer occurs. (Moving non-contiguous files also works fine.)

DiapDealer · 10-06-2018, 06:33 AM

Thanks for testing. Were you experiencing the css issue (bug #2) on Linux, though? I was under the impression you couldn't reproduce it.

Doitsu · 10-06-2018, 07:08 AM

Quote:

Originally Posted by DiapDealer

Thanks for testing. Were you experiencing the css issue (bug #2) on Linux, though? I was under the impression you couldn't reproduce it.

You're right, before your fix I was only able to reproduce bug#1. I tested the second issue for good measure.

DiapDealer · 10-06-2018, 07:16 AM

Quote:

Originally Posted by Doitsu

You're right, before your fix I was only able to reproduce bug#1. I tested the second issue for good measure.

Good to know!

For what it's worth, the fix would happily try to accommodate Linux should it decide to start exhibiting the problem.

Similar Threads
Thread	Thread Starter	Forum	Replies	Last Post
bugs in book browser	Reinhard	Sigil	25	07-31-2018 02:07 PM
Possible Error / Enhancement - Book Editor, Spell Check Right-Click Sub-Menu	WizkidTX	Editor	7	07-22-2015 09:33 PM
PRS-T1 Two tiny bugs when reading a book	opitzs	Sony Reader	10	07-08-2013 07:10 PM
[Program Enhancement] Adding Empty Book	Gunnerp245	Calibre	3	12-24-2012 07:32 AM
Enhancement (Delete formats from Book Details Panel)	JohnnyBook	Library Management	5	07-21-2012 08:17 AM

10-03-2018, 12:17 PM	#31
DiapDealer Grand Sorcerer Posts: 27,549 Karma: 193191846 Join Date: Jan 2010 Device: Nexus 7, Kindle Fire HD	We're already closing Preview on all deletes when Book View is open as well (which causes odd--but not really problematic--graphical issues when Preview is tabified with other widgets), so I'd hate to have Book View and Code View flip-flopping in the background as well. But that would probably be a definitive workaround as opposed to an arbitrary delay in SDeleteFile that may or may not be sufficient for all machines. Let me play around with both ideas tonight (since you can't recreate the issue) and see what I can find out. Thanks for delving into it.

10-03-2018, 12:27 PM	#32
KevinH Sigil Developer Posts: 7,644 Karma: 5433388 Join Date: Nov 2009 Device: many	Sounds good. FYI, if you want to try the "delay only if delete fails and try again approach", there is a Misc/SleepFunctions.h which is used in MainWindow.cpp and other places to allow a delay to allow other Qt Threads to process. Adding that to SDeleteFiles and only using it if the remove fails may be something to test as loading css files should come early in the webkit loading process.

10-03-2018, 01:04 PM	#33
DiapDealer Grand Sorcerer Posts: 27,549 Karma: 193191846 Join Date: Jan 2010 Device: Nexus 7, Kindle Fire HD	Thanks. I'll check it out.

10-03-2018, 05:15 PM	#34
DiapDealer Grand Sorcerer Posts: 27,549 Karma: 193191846 Join Date: Jan 2010 Device: Nexus 7, Kindle Fire HD	Well it doesn't seem the delayed retry strategy is a viable workaround. I started with a 5 second delay just to see where I stood, and ended up with the same result: only one of the css files was deleted even after the delay. I didn't bother increasing the delay since 5 seconds was already an unacceptable amount of time.

10-04-2018, 05:54 PM	#37
KevinH Sigil Developer Posts: 7,644 Karma: 5433388 Join Date: Nov 2009 Device: many	I think we should add the process events and sleep only if the first delete attempt fails since this race only impacts images and css that are actually linked to the front Tab whereas SDeleteFiles is usd in many places and with many file types where no sleep is ever needed or wanted. Nce work!

10-04-2018, 06:42 PM	#39
DiapDealer Grand Sorcerer Posts: 27,549 Karma: 193191846 Join Date: Jan 2010 Device: Nexus 7, Kindle Fire HD	Just pushed the fix (hopefully) for bug #2.

10-06-2018, 06:33 AM	#41
DiapDealer Grand Sorcerer Posts: 27,549 Karma: 193191846 Join Date: Jan 2010 Device: Nexus 7, Kindle Fire HD	Thanks for testing. Were you experiencing the css issue (bug #2) on Linux, though? I was under the impression you couldn't reproduce it.