Dispatcher: avoid multiple cre rerenderings when many settings changed by poire-z · Pull Request #9651 · koreader/koreader

poire-z · 2022-10-19T12:15:50Z

When a gesture/profile was updating multiple cre settings, each setting handler would emit UpdatePos which each would force a re-rendering.
When this might be happening, postpone the rerendering until all are set.
Needs some bit of refactoring to the events at play: introduce "DocumentRerendered" event, and use it where we used "UpdatePos" or "UpdateToc" to mean exactly that.

Discussion and first experiments at #9594 (comment)
Not thoroughly tested, and it's a bit hard to guess what edgy mess this kind of event tweaking can cause (I just changed what I needed trying to keep the logic, there may be extra unnecessary calls that were already there around ReaderFooter and the non-linear flow stuff). (Also, the undocumented/unavailable thru menu footer.settings.disabled=true was and is still causing a crash.)

Will remove logger.warn() before merge.

@mergen3107 : here's a zip with the files changed in this PR, thanks for testing :)
dispatcher_batched1.zip

This change is

When a gesture/profile was updating multiple cre settings, each setting handler would emit UpdatePos which each would force a re-rendering. When this might be happening, postpone the rerendering until all are set. Needs some bit of refactoring to the events at play: introduce "DocumentRerendered" event, and use it where we used "UpdatePos" or "UpdateToc" to mean exactly that.

NiLuJe · 2022-10-19T13:48:05Z

footer.settings.disabled

What's that? :o]

NiLuJe · 2022-10-19T13:49:43Z

+    have changed (ie. font, line height, margin... change)
    Note that xpointer should not be changed.
+    The only handler of this event should be this one.
+    A "DocumentRerendered" event will be then set if it has changed.


nit: sent?

NiLuJe · 2022-10-19T13:50:07Z

    Note that xpointer should not be changed.
+    The only handler of this event should be this one.
+    A "DocumentRerendered" event will be then set if it has changed.
+    Provide force=true to get it emited even if nothing has changed.


nit: emitted

NiLuJe · 2022-10-19T13:55:15Z

Looks good at a quick glance (but I haven't looked at the other UpdatePos callers either ;)).

poire-z · 2022-10-19T14:07:25Z

footer.settings.disabled

What's that? :o]

You tell me :)

NiLuJe · 2022-10-19T14:13:51Z

footer.settings.disabled

What's that? :o]

You tell me :)

Foiled by fixing a comment :D.

Turns out, that, as usual, when a weird old thing doesn't make sense, it turns out the culprit is... the testsuite (adc7ddc). -_-".

NiLuJe · 2022-10-19T15:21:43Z

    if settings.settings ~= nil and settings.settings.show_as_quickmenu == true then
        return Dispatcher:_showAsMenu(settings)
    end
+    local has_many = util.tableSize(settings) > (settings.settings ~= nil and 2 or 1)


Possibly stupid idea (especially given the possible constraints on the table format): it would feel mildly more elegant to have whatever actually builds the table keep track of how many things it puts in it instead of relying on tableSize after the fact ;).

Dunno, it's usually 1 item, or a few items, so tableSize() just doing a pair() should be quick.
Otherwise, it's for the Masters of Dispatcher @yparitcher @hius07 to say (and implement :) if it could make sense to have settings.settings.order created/removed/one-time-migrated, so that it exists only when multiple items, and absent when single item - and we could just test its presence.

Currently one-action profile can be sorted or unsorted.
I think it would be good to disable "Sort" menu item for one-action profiles/gestures.

But many-actions profile/gesture can be unsorted, so order field cannot be a flag here.

And there is one more this check

koreader/frontend/dispatcher.lua

Lines 554 to 558 in d5bda32

local count = util.tableSize(settings)

if count == 0 then return _("Nothing") end

if count > 1 and settings.settings ~= nil then

count = count - 1

end

there are many places where we add or remove items to the table, so i think it is cleaner to have just one place where it is checked than to make sure to update some counter variable every time the table is changed. after this PR I will clean up and merge both usages into some Dispatcher:_itemsCount() function that returns a number.

mergen3107 · 2022-10-19T19:13:21Z

@poire-z
Thank you for the files!
So I installed the 91-nightly and then used the files to replace contents in /data/data/org.koreader.launcher/files, but I got crash with a bomb and empty window. If I reinstall the 91, it still crashes. If I delete first then install it again, it works.

mergen3107 · 2022-10-19T19:17:17Z

Here is the pidcat when I try to open patched 91:
2_crash1_pidcat.txt

mergen3107 · 2022-10-19T19:21:50Z

I fresh installed the 91, and then applied the first patch you provided before: #9594 (comment)

It works! Profiles are applying event better now, all actions have popups and are stacked. They appear and disappear all at once.

poire-z · 2022-10-19T19:39:09Z

Not familiar with Android pecularities...
May be check how your frontend/apps/reader/modules/readerfooter.lua looks like ?

           NativeThread  E  Failed to run script: frontend/apps/reader/modules/readerview.lua:17: module 'apps/reader/modules/readerfooter' not found:
                         E      no field package.preload['apps/reader/modules/readerfooter']
                         E      no file 'apps/reader/modules/readerfooter.lua' (checked with asset loader)
                         E      no file 'common/apps/reader/modules/readerfooter.lua'
                         E      no file 'rocks/share/lua/5.1/apps/reader/modules/readerfooter.lua'
                         E      no file 'frontend/apps/reader/modules/readerfooter.lua'
                         E      no file 'apps/reader/modules/readerfooter.lua'
                         E      no file '/data/user/0/org.koreader.launcher/files/apps/reader/modules/readerfooter.lua'
                         E      no file 'common/apps/reader/modules/readerfooter.so'
                         E      no file 'common/apps/reader/modules/readerfooter.dll'
                         E      no file '/usr/lib/lua/apps/reader/modules/readerfooter.so'
                         E      no file 'rocks/lib/lua/5.1/apps/reader/modules/readerfooter.so'
                         E      no file 'apps/reader/modules/readerfooter.so'
                         E      no file '/data/user/0/org.koreader.launcher/files/apps/reader/modules/readerfooter.so'
    ActivityTaskManager  I  START u0 {flg=0x50804000 cmp=org.koreader.launcher/.CrashReportActivity (has extras)} from uid 10143

mergen3107 · 2022-10-19T20:02:55Z

Hmm, this is interesting. I replaced all files one by one by first making a backup of existing files, then copying the new ones from your new archive. Except from different permissions (644 instead of 600) files are not affected by anything (Unix LF is there, content is OK). Maybe my WinSCP is having problems with batch copy-replace, since before that I just copied frontend and plugins directories with "overwrite all".

KOReader now works with new files too. What should I test more? Profiles' activation is instantaneous, notifications show up properly. Except that there is not notification for the font change. I have four actions in the each profile (font face, font size, font weight, line spacing), and only three notifications show up.

poire-z · 2022-10-19T20:51:43Z

Good!

Except that there is not notification for the font change. I have four actions in the each profile (font face, font size, font weight, line spacing), and only three notifications show up.

I guess this is normal: the code that set the font does not trigger a notification, unlike all others. I guess that's because you usually set the font from the menu, so you know with the checkbox the font that you are setting, so no need to bother with a notification (displayed over that opened menu, and possible cause of the unfamous papercut refresh glitches).
But well, from a gesture, having that notification might be welcome - but I'm not sure we'll bother :) (does not look obvious to implement).

What should I test more?

Nothing specific really. Just use KOReader, tweak some setting(s) from time to time, just use it - if you don't notice any bug (like footer not updated, getting stuck), that will be fine and enough :)

mergen3107 · 2022-10-20T06:49:55Z

OK, thank you :)

yparitcher

Not tested, looks ok

ghost · 2022-10-20T18:39:45Z

lgtm

Fix regression introduced by 48eb023 #9651: on re-rerendering, the new pages count didn't reach the Statistics plugin. So, if you change font size or margins, the stats for the page read since then were badly accounted, until a change of book or restart... I noticed that in Book map, strange nobody else did over the last 2.5 weeks.

koreader#9651) When a gesture/profile was updating multiple cre settings, each setting handler would emit UpdatePos which each would force a re-rendering. When this might be happening, postpone the rerendering until all are set. Needs some bit of refactoring to the events at play: introduce "DocumentRerendered" event, and use it where we used "UpdatePos" or "UpdateToc" to mean exactly that.

Fix regression introduced by 2f749ca koreader#9651: on re-rerendering, the new pages count didn't reach the Statistics plugin. So, if you change font size or margins, the stats for the page read since then were badly accounted, until a change of book or restart... I noticed that in Book map, strange nobody else did over the last 2.5 weeks.

poire-z added this to the 2022.11 milestone Oct 19, 2022

poire-z requested review from NiLuJe and yparitcher October 19, 2022 12:16

NiLuJe reviewed Oct 19, 2022

View reviewed changes

yparitcher approved these changes Oct 20, 2022

View reviewed changes

remove logs

f14e18f

poire-z marked this pull request as ready for review October 25, 2022 11:12

poire-z merged commit 48eb023 into koreader:master Oct 25, 2022

poire-z deleted the dispatcher_batched branch October 25, 2022 11:16

hius07 mentioned this pull request Oct 25, 2022

Dispatcher: preserve profile name, show actions count #9682

Merged

poire-z mentioned this pull request Nov 11, 2022

Fix page stats messed up after rerendering #9775

Merged

This was referenced Dec 31, 2022

(UX) progress bar(s) improvement #9979

Closed

ButtonTable, ReaderDogear and TextEditor tweaks and fixes #9985

Merged

	local count = util.tableSize(settings)
	if count == 0 then return _("Nothing") end
	if count > 1 and settings.settings ~= nil then
	count = count - 1
	end

Uh oh!

Uh oh!

Conversation

poire-z commented Oct 19, 2022 • edited by Frenzie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NiLuJe commented Oct 19, 2022

Uh oh!

NiLuJe Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

NiLuJe Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

NiLuJe commented Oct 19, 2022

Uh oh!

poire-z commented Oct 19, 2022

Uh oh!

NiLuJe commented Oct 19, 2022

Uh oh!

NiLuJe Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

poire-z Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

hius07 Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

hius07 Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hius07 Oct 19, 2022

Choose a reason for hiding this comment

Uh oh!

yparitcher Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

mergen3107 commented Oct 19, 2022

Uh oh!

mergen3107 commented Oct 19, 2022

Uh oh!

mergen3107 commented Oct 19, 2022

Uh oh!

poire-z commented Oct 19, 2022

Uh oh!

mergen3107 commented Oct 19, 2022

Uh oh!

poire-z commented Oct 19, 2022

Uh oh!

mergen3107 commented Oct 20, 2022

Uh oh!

yparitcher left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

poire-z commented Oct 19, 2022 •

edited by Frenzie

Loading

hius07 Oct 19, 2022 •

edited

Loading