Dispatcher: allow sorting actions

[yparitcher]

mostly borrowed from @hius07 in #9526

Lightly tested.

---

This change is 

[hius07]

Thank you. A couple of notes:
(1) After adding an action, Sort is immediately checked even I haven't sorted anything yet.
(2) In Gesture manager action menu now has 11 items (Pass through (default) and Pass through are above Sort) that causes two pages of the menu.

[hius07]

More changes to the Profiles module are required. I may proceed with them after you finish the Dispatcher.

[yparitcher]

(1) After adding an action, Sort is immediately checked even I haven't sorted anything yet.

Dispatcher now automatically sorts items by when they were added. (adding an item adds it to the sortlist, and removing an item deletes it). This allow not having to worry if the sort list is synced.
Items that were created before this PR are unsorted and will only be sorted if setting a sort order. (to get this behavior one can hold the sort item)

(2) In Gesture manager action menu now has 11 items (Pass through (default) and Pass through are above Sort) that causes two pages of the menu.

I know. :) This still needs to be dealt with.

More changes to the Profiles module are required. I may proceed with them after you finish the Dispatcher.

Please give me a few more days, as this is not finished yet (quickmenu etc)

[yparitcher]

(2) In Gesture manager action menu now has 11 items (Pass through (default) and Pass through are above Sort) that causes two pages of the menu.

Still looking for ways to arrange/fix this

[hius07]

For profiles we need to see the profile name here.

[poire-z]

In Gesture manager action menu now has 11 items

Still looking for ways to arrange/fix this

Can we get a screenshot to know what you're talking about ? :)

[hius07]

With the latest commits the conception has changed: the profile can be either executed or shown as QuickMenu, not both options. I'm fine with that.
@mergen3107, what is your experience? Are there profiles that sometimes you want to execute in full and sometimes to see as a QuickMenu to execute a single action only?

[mergen3107]

So far I only used profiles in full, because my profiles are basically font face, font weight, font size and line spacing. So if I activated a profile and found out that, for example, line spacing doesn’t fit here, I’d just go to bottom menu to tweak it, not profile’s quick menu.

However, in a broader sense I’d imagine someone with a night mode + font + brightness changing profile wanting to activate only night mode with fonts, but not brightness.

[yparitcher]

Rosh Hashana is this monday & tuesday so i will not have time to finish this untill late next week at the earliest.

[hius07]

Not directly oncerns this PR, but Dispatcher and creoptions and koptoptions. Are there any reasons to give the same names to actions in cre and pdf documents? Eg line_spacing and kopt_line_spacing, font_size and kopt_font_size.
This may cause a crash:
-assign the action "Reflowable : set font size to 22" to a gesture
-open pdf document, set reflow on
-apply the gesture

error: malloc of array (44546 x 62170 bytes) failed
./luajit: ./ffi/mupdf.lua:78: could not allocate pixmap: malloc of array (44546 x 62170 bytes) failed (1)
stack traceback:
	[C]: in function 'error'
	./ffi/mupdf.lua:78: in function 'merror'
	./ffi/mupdf.lua:1002: in function 'render_for_kopt'
	./ffi/mupdf.lua:1029: in function 'reflow'

The crash is caused by

koreader/frontend/document/pdfdocument.lua

Lines 61 to 66 in c3cd88e

	local default_font_size = 22
	-- the koptreader config goes from 0.1 to 3.0, but we want a regular font size
	function PdfDocument:convertKoptToReflowableFontSize(font_size)
	if font_size then
	return font_size * default_font_size
	end

We try to set font size to 22*22 and get OOM crash.

[hius07]

Dunno is it worth to show action's value in QuickMenu. Some of the original values are not much informative and require getting text labels from creoptions/koptoptions.

[mergen3107]

I think values are good! Makes more sense when you see what actually you are going to apply.

Also why the first two items different? Only these have verbs, others just say what the option is named and its value

[mergen3107]

I mean, is this intentional and you plan to add verbs to all of them?

[hius07]

Also why the first two items different? Only these have verbs, others just say what the option is named and its value

Current original strings from Dispatcher are displayed.

[hius07]

@mergen3107, maybe it is of interest for you: #8590 (comment) and further

[yparitcher]

In Gesture manager action menu now has 11 items

Still looking for ways to arrange/fix this

Can we get a screenshot to know what you're talking about ? :)

Gestures

Profiles

[poire-z]

Can we get a screenshot to know what you're talking about ? :)

Gestures

Indeed :/ Moreover, they are 2 non-actions between a lot of actions (items or submenus).
May be move Show as QuickMenu and Sort at the end, so they are both displayed on the 2nd page and feel more independant ?
It would be nice to have a separator above these 2 on that 2nd page, to make it clear they don't really follow the actions submenus - so, may be add a 3rd item (before these 2) on the 2nd page, something like "About QuickMenu and sorting" explaining what/why/how.
(Anyway, add the separator, as when in Landscape mode, we don't get 10 items per page but less.)

[yparitcher]

Ready on my end.
Only lightly tested, as I have don't use profiles, and don't have a use for a QuickMenu

[yparitcher]

Not directly oncerns this PR, but Dispatcher and creoptions and koptoptions. Are there any reasons to give the same names to actions in cre and pdf documents? Eg line_spacing and kopt_line_spacing, font_size and kopt_font_size. This may cause a crash: -assign the action "Reflowable : set font size to 22" to a gesture -open pdf document, set reflow on -apply the gesture

The reason dispatcher has different names is that the names are based on the names in creoptions and koptoptions which call different event base on which engine is being used.

Using Crengine settings in a MuPdf document is Undefined Behavior.

Reflowable : set font size is a crengine setting/event. calling it in a Mupdf document will not cause any event to fire (mupdf does not listen to that event), however any setting from creoptions and koptoptions will also update the configurable object (so as not to break the bottom menu). The configurable.font size is the same name for both cre and mupdf causing the config to think that the font size is 22 eventually leading to an OOM crash (It does not crash for me in the emulator, it just make the font size big.)

These interactions were discussed when dispatcher was first implemented, and the consensus was that if a user should be smart enough not to shoot himself in the foot, as technically the user can set many contradictory events.

[yparitcher]

updated screenshots
Gestures


profiles

[hius07]

Thanks, works good!

A couple of propositions:
-to see the profile name in the title of its QuickMenu
-since we always "execute" profile (even when showing its QM), no need of additional sign \u{F144} in the profile name in the Dispatcher

A fix required:
-after editing actions and going up to the profile properties, the label of the action near "Edit actions:" doesn't change.

[yparitcher]

A fix required: -after editing actions and going up to the profile properties, the label of the action near "Edit actions:" doesn't change.

Fixed text -> text_func

[yparitcher]

-since we always "execute" profile (even when showing its QM), no need of additional sign \u{F144} in the profile name in the Dispatcher

Done

[yparitcher]

A couple of propositions: -to see the profile name in the title of its QuickMenu

This one is hard, we don't know the name as we do not have context into what called Dispatcher allowing it to receive events from anywhere (and i would like to keep it that way)
also for example if a gesture opens a profile as a QuickMenu do we want the profile or the gesture?

[yparitcher]

-to see the profile name in the title of its QuickMenu

Done

[hius07]

Great!

[yparitcher]

@hius07
Enjoy polishing up profiles after this is merged, but please give me a chance to review any interactions with dispatcher :)
It might take me a few day as tonight/tomorrow is Yom Kippur and next week is Sukkot

[hius07]

It is now very clear/ logical Profiles - Dispatcher interaction, I think it is worth to wait for users' response on using it, before any further development.

[yparitcher]

It is now very clear/ logical Profiles - Dispatcher interaction, I think it is worth to wait for users' response on using it, before any further development.

Unless maybe make registering a profile to Dispatcher optional?

[poire-z]

Not familiar with this PR code, so just mentionning this situation I got when playing with multiaction gesture.
I remember that at some point, I had added "Increase font size by # + 1", then later removed it.
It is still executed though:

I guess because it's the "sort order" list that is used - and it may not be updated when we uncheked an action in the main menu. I have currently in my gestures.lua:

        ["multiswipe_southeast_northeast_northwest"] = {
            ["font_base_weight"] = 1,
            ["font_kerning"] = 0,
            ["font_size"] = 27,
            ["h_page_margins"] = {
                [1] = 30,
                [2] = 30,
            },
            ["settings"] = {
                ["order"] = {
                    [1] = "font_base_weight",
                    [2] = "font_kerning",
                    [3] = "increase_font",
                    [4] = "font_size",
                    [5] = "font_kerning",
                    [6] = "h_page_margins",
                },
            },
        },

(Also the duplicated font_kerning in settings.order.)

[yparitcher]

@poire-z
If you added/ sorted between @hius07 original PR and this one there may be some funky interactions, See if you can reproduce 1. on a new gesture/profile 2. if you re-add and remove "Increase font size by # + 1".

[poire-z]

Checked again with current master, same behaviour.
Moreover, I hadn't even used "Sort".
Added in that order: Increase font size +1, font weight, font kerning. When executed, font-size was first (in the notification list)
Removed Increase font size (first by setting default value 0 - which was not the way to do it, so then by long-press on the menu item). When executed, font-size is now last (in the notification list).
In my gestures.lua:

        ["multiswipe_northwest_southwest_northwest"] = {
            ["font_base_weight"] = 1,
            ["font_kerning"] = 1,
            ["settings"] = {
                ["order"] = {
                    [1] = "font_base_weight",
                    [2] = "font_kerning",
                    [3] = "increase_font",
                },
            },
        },

on a new gesture/profile

I only used Tap and gestures > multiswipe.

[yparitcher]

(first by setting default value 0

The important part. :)
Fixed in #9628

[poire-z]

I confirm that the issue did not happen when you don't do my "first by setting default value 0" :)
And I confirm your fix does fix this when I do that.

Minor and probably unrelated issue and probably not worth thinking about it (@hius07 ?):
you can't set "Increase font size" to 0 when using the +/- buttons. But you can when using the (new there?) Use default value: 0 button - and you can save it.
And although it's supposed to be 0 - and possibly have no effect, it does increase my font size by 5. It's possible 5 is some default increment when nothing/zero is given.

[hius07]

It is unexpected for SpinWidget that the default value is less than min value.

koreader/frontend/dispatcher.lua

Line 145 in da65ac8

increase_font = {category="incrementalnumber", event="IncreaseFontSize", min=0.5, max=255, step=0.5, title=_("Increase font size by %1"), rolling=true},

koreader/frontend/dispatcher.lua

Line 703 in da65ac8

default_value = 0,

Anyways, I think there should not be any default value for this case.

[hius07]

I'd propose to change

koreader/frontend/dispatcher.lua

Line 699 in f3620b6

value = location[settings] ~= nil and location[settings][k] or 0,

to value = location[settings] ~= nil and location[settings][k] or settingsList[k].min,

and remove line

koreader/frontend/dispatcher.lua

Line 705 in f3620b6

default_value = 0,

Applicable for all incrementalnumber settings (increase/decrease font size, frontlight, warmth).