https://bugs.kde.org/show_bug.cgi?id=510913
--- Comment #2 from Duncan <[email protected]> --- Created attachment 190706 --> https://bugs.kde.org/attachment.cgi?id=190706&action=edit Set a timeout to prevent next/prev-image double-triggers (workaround patch) Workaround patch for those who can apply and build from source. Despite the attachment submission warning steering patch submissions to kde's gitlab, being a gentooer but not a dev I'm familiar only with the consumer side of git not the commit and merge request side, and while I do build from source, that's still a lot of first-time hoops to jump thru... But I've been testing this for a week now and it does effectively work around the bug, so I might as well at least submit it here. While this patch does work around the problem (with a trade-off, see below) it's not a "proper" fix, which would (I believe) require an audit of input-event-accepts (in qt/kde components as well as gwenview... should be easy for devs used to working with those components who are thus familiar with their input-event-accept default behavior) as based on the symptoms there's two components processing them, with only one accepting the event (which prevents further processing), so if the not-accepting component runs first it triggers next/prev-image but doesn't remove the event from the event-queue and thus doesn't stop the second one from processing it too so we get a double-trigger and the bug, while if the accepting component runs first it removes the event from the queue, thus preventing the second one from processing it too, so we only get a single-trigger, as desired. The tradeoff in this hack-around is that it uses a qTimer (and a qMutex) to disable additional next/prev-image triggers until a timeout. While that does prevent an unwanted double-trigger, there is of course a responsiveness trade-off, depending on the configured timeout -- a faster timeout will allow gwenview to process additional next/prev-image requests sooner, but at the risk of not blocking the double-trigger for long enough. Gwenview happens to use two other timers to control pre-loading, one set for 100 ms, the other for 1000 (one's for viewer mode the other for browse mode, I didn't pay attention to which is the longer one), so I wanted a value within that range if possible, and of course tried the shorter 100 ms value first. On my decade-old hardware, 100 ms simply wasn't enough. Testing /seemed/ to yield /marginally/ fewer double-triggers, but it was marginally fewer if any and possibly nearly all placebo-effect. 500 ms was *much* better, reliably preventing double-trigger on a mostly-idle system, but I'd still occasionally see one if the system was heavily loaded (gentoo, so building updates for instance, keeping in mind that this is for ~1 Mpx images, not the huge ones the bug reporter found triggered the bug on his far newer/faster system). So I tried 1000 ms and wasn't able to trigger a double-trigger any longer even at heavy load, at least with the ~1 Mpx images I was using. In any case I'd really hate to go higher than a second as disabling further next/prev-image navigation for even a second is going to seem sluggish to people with fast systems, and ideally, if a double-trigger /does/ occur, the timeout should at least prevent the unpatched situation I frequently see now where I have to repeatedly navigate forward and back and forward and back until I get it doing just *one* image to get to the one it's skipping between the images on either side of it. Even with the tradeoff I think it's worth it here, however, because double-trigger image navigation can be /quite/ irritating, and this work/hack-around should prevent it on most systems under anything like reasonable loads. A raspberry-pi-class system under heavy-enough load with large enough images, OTOH... But, it *is* a tradeoff, one that a real fix, finding the (apparent) missing input-event-accept and adding it instead of doing this hack-around, would avoid. Not being a dev I don't even know how to expose the timeout as either a GUI setting or a read environmental var to allow a user to configure it for their system, and must choose a value to hard-code into the patch as that's all I know how to do. Tho of course users applying the patch themselves can tweak and test their own values if they choose... and with any luck a proper dev will take this as a starting point and run with it to do a proper fix. Meanwhile, in theory, for a timer work-around patch, only the qTimer should be needed (not the qMutex too), as a timer-active conditional can replace the qMutex conditional. But as the qTimer docs say, a static oneshot call like I used (and like the existing two preload timers use) is significantly less complex to setup than the full qTimer that would be necessary in ordered to be able to call timer-active on it, and as a non-dev the simpler static single-shot along with the qMutex was easier to hack into working. Plus, qMutex, like qTimer, is part of qtbase, so at least it doesn't pull in additional deps. The patch does have, however, a happy bonus that I hadn't anticipated, tho in hindsight it does make sense. As the original reporter mentioned as a related aside, currently, holding down a next/prev key in ordered to have auto-repeat continuously scan forward/back an image at a time does not work, as the input-events come in faster than gwenview can handle them and it stalls the display update until the key is released, at which point it jumps many images at once! While the original reporter didn't mention it, I had found here that if I configured a slow enough auto-repeat, say 4-5/sec (on this system trouble started at 6/sec) instead of the usual 20 or 30, while gwenview still couldn't necessarily switch images at the given auto-repeat rate (maybe it could do 1-2/sec), that did allow enough time between incoming key events to allow gwenview to dump the ones the image-display pipeline couldn't manage, thus allowing gwenview to continuously advance an image at a time (not sure if the double-trigger bug would sometimes hit and advance two images at a time or not, but in general it'd do the expected one at a time), even if not at the full key auto-repeat rate. With the patch, the overload of extra input-events are dropped by the timeout mechanism as if they were double-triggers, so holding down a next/prev-image key does result in continuous image scanning forward or back as expected, at the timeout frequency if the system can handle it (well, slightly lower frequency due to processing because as-coded the patch starts the timer at the /end/ of the function), thus 1/sec at the 1000 ms timeout value hard-coded into the patch, regardless of the configured key auto-repeat rate. No more stalled display pipeline and then jumping many images ahead at key release! =:^) -- You are receiving this mail because: You are watching all bug changes.
