Repository navigation
[Presets] add new presets.lua module and dictionary presets - #13774
Conversation
|
I took the liberty to re-arrange some of these settings, here is a little draft for all to comment on, both presets and priority running side-by-side, left is what it looks like on traditional e-ink, right on android/emulator should I combine the external dicts into one as well? as I did for "lookup history" |
|
No problem with me with the menu reorg. |
I don't think it is worth adding another menu just for two items. I would vote for three and more items, otherwise it is more of an obstacle (extra tap) than help |
|
would anyone have a clue as to why, status bar presets (without me changing anything yet) crash when one tries to load them on the emulator but NOT when doing it from kindle, both running edit: initiating |
This comment was marked as outdated.
This comment was marked as outdated.
The problem will be that you changed the data structure somewhere along the way during development, or you didn't change it as such but accidentally wrote what's effectively some nonsense to the settings file. |
|
I thought so at first, then i created new ones and still kaboom. Not to worry though, hius07 has already been heavily on the case. |
|
This must have been how Alfred Nobel felt when he discovered his creation was being used for evil... In good faith, I have added a method to cycle through presets; but heed my warning, it will not make it to |
b1634a5 to
163d6e5
Compare
add dependency to ReaderFooter and use it remove unused dependencies add documentation and move event handler to presets.lua add missing dependency fixes to loadPresets add message to succesful preset loading from touchmenu
163d6e5 to
3da2fd0
Compare
| if not presets or not next(presets) then | ||
| Notification:notify(_("No presets available"), Notification.SOURCE_ALWAYS_SHOW) | ||
| return false | ||
| end |
There was a problem hiding this comment.
Hello all, I am losing my mind trying to understand why this particular bit of code is running twice (note, the bottom notification doesn't do the same).
if not presets or not next(presets) then
local logger = require("logger")
logger.dbg("About to show no presets notification")
Notification:notify(_("No presets available"), Notification.SOURCE_ALWAYS_SHOW)
logger.dbg("After showing no presets notification")
logger.dbg("AAA No presets available for " .. preset_key)
return false
endDEBUG key event => code: 1073742048 (Ctrl), value: 1, time: 1400855.826972
DEBUG key event => code: 113 (Q), value: 1, time: 1400856.165875
DEBUG AutoSuspend: onInputEvent
DEBUG AutoTurn: onInputEvent
DEBUG Executing actions for hotkey: alt_plus_q in Reader with events: {
cycle_dictionary_preset = true
} --[[table: 0x0122c60750]]
DEBUG ReaderDictionary:onCycleDictionaryPresets() called
DEBUG About to show no presets notification
DEBUG show widget: table: 0x01249ce560
DEBUG setDirty nil from widget table: 0x01249ce560 w/ NO region; dithering: nil
DEBUG setDirty via a func from widget table: 0x01249ce560
DEBUG After showing no presets notification
DEBUG AAA No presets available for dict_presets
DEBUG ReaderDictionary:onCycleDictionaryPresets() called
DEBUG About to show no presets notification
DEBUG show widget: table: 0x01245080c0
DEBUG setDirty nil from widget table: 0x01245080c0 w/ NO region; dithering: nil
DEBUG setDirty via a func from widget table: 0x01245080c0
DEBUG After showing no presets notification
DEBUG AAA No presets available for dict_presets
DEBUG painting widget: table: 0x01249ce560
DEBUG Found font: NotoSansCJKsc-Regular.otf in ./fonts/noto/NotoSansCJKsc-Regular.otf
DEBUG painting widget: table: 0x01245080c0
DEBUG _refresh: Enqueued ui update for region 171 5 197 53 dithering: false
DEBUG _refresh: Enqueued ui update for region 171 5 197 106 dithering: false
DEBUG refresh on logical rectangle 197x106+171+5
DEBUG key event => code: 113 (Q), value: 0, time: 1400856.245468
DEBUG AutoSuspend: onInputEvent
DEBUG AutoTurn: onInputEvent
DEBUG key event => code: 1073742048 (Ctrl), value: 0, time: 1400856.456355
things of interest
- only one hotkey action being executed
- two different widget addresses created:
show widget: table: 0x01249ce560
show widget: table: 0x01245080c0
- two notifications shown, does not happen with any other notification added in this PR
- removing the
SOURCE_ALWAYS_SHOWdoes not change anything.
There was a problem hiding this comment.
Twice cycle in the dispatcher/hotkey action?
There was a problem hiding this comment.
But then, when we do have actions to cycle through, only one notification is fired. This is really wierd.
There was a problem hiding this comment.
with presets we only run once, only one widget address show widget: table: 0x011f2c90e8
DEBUG key event => code: 1073742048 (Ctrl), value: 1, time: 1520575.126891
DEBUG key event => code: 113 (Q), value: 1, time: 1520575.897105
DEBUG AutoSuspend: onInputEvent
DEBUG AutoTurn: onInputEvent
DEBUG Executing actions for hotkey: alt_plus_q in Reader with events: {
cycle_dictionary_preset = true
} --[[table: 0x011d8c6ce8]]
DEBUG show widget: table: 0x011f2c90e8
DEBUG setDirty nil from widget table: 0x011f2c90e8 w/ NO region; dithering: nil
DEBUG setDirty via a func from widget table: 0x011f2c90e8
DEBUG painting widget: table: 0x011f2c90e8
DEBUG Found font: NotoSansCJKsc-Regular.otf in ./fonts/noto/NotoSansCJKsc-Regular.otf
DEBUG _refresh: Enqueued ui update for region 151 5 238 53 dithering: false
DEBUG refresh on logical rectangle 238x53+151+5
DEBUG key event => code: 113 (Q), value: 0, time: 1520575.901003
DEBUG AutoSuspend: onInputEvent
DEBUG AutoTurn: onInputEvent
DEBUG key event => code: 1073742048 (Ctrl), value: 0, time: 1520575.994968
DEBUG close widget: table: 0x011f2c90e8
There was a problem hiding this comment.
DEBUG cyclePresets called from:
DEBUG stack traceback:
frontend/ui/presets.lua:143: in function 'handleEvent'
frontend/ui/widget/container/widgetcontainer.lua:83: in function 'propagateEvent'
frontend/ui/widget/container/widgetcontainer.lua:101: in function 'handleEvent'
frontend/ui/uimanager.lua:896: in function 'sendEvent'
frontend/dispatcher.lua:1217: in function 'execute'
plugins/hotkeys.koplugin/main.lua:99: in function <plugins/hotkeys.koplugin/main.lua:89>
[C]: in function 'pcall'
frontend/pluginloader.lua:62: in function 'handleEvent'
frontend/ui/widget/container/widgetcontainer.lua:83: in function 'propagateEvent'
frontend/ui/widget/container/widgetcontainer.lua:101: in function 'handleEvent'
frontend/ui/uimanager.lua:896: in function 'sendEvent'
frontend/ui/uimanager.lua:53: in function '__default__'
frontend/ui/uimanager.lua:1423: in function 'handleInputEvent'
frontend/ui/uimanager.lua:1523: in function 'handleInput'
frontend/ui/uimanager.lua:1567: in function 'run'
reader.lua:265: in main chunk
[C]: at 0x010c1a4890
There was a problem hiding this comment.
so it all comes down to return false, the return false propagates the event and runs it another time... mmm return true solves that problem. sigh..
| text = _("Download dictionaries"), | ||
| sub_item_table_func = function() return self:_genDownloadDictionariesMenu() end, | ||
| text = _("Dictionary presets"), | ||
| help_text = _("This feature enables you to group (e.g., per language) dictionaries as presets. You can then quickly switch between these presets to change the dictionaries used for lookups. \n\n Note: No other settings are stored in the presets, neither are the actual dictionaries, only a reference to the dictionaries."), |
There was a problem hiding this comment.
No spaces around the \n\n please. Also an editorial suggestion.
| help_text = _("This feature enables you to group (e.g., per language) dictionaries as presets. You can then quickly switch between these presets to change the dictionaries used for lookups. \n\n Note: No other settings are stored in the presets, neither are the actual dictionaries, only a reference to the dictionaries."), | |
| help_text = _("This feature allows you to organize dictionaries into presets (for example, by language). You can quickly switch between these presets to change which dictionaries are used for lookups.\n\nNote: presets only store a reference to the dictionaries, no other settings."), |
| end | ||
| end | ||
| UIManager:show(InfoMessage:new{ | ||
| text = T(_("Some dictionaries from this preset have been deleted or are no longer available (%1 missing): \n\n• %2"), |
There was a problem hiding this comment.
Perhaps we can avoid sticking that • %2 in this string at all? (I.e., simple concatenation at the end.) If not, no space before the \n\n.
There was a problem hiding this comment.
don't you think it would be more useful (as a user) to be told straight up which dictionaries are missing instead of having to guess? The idea is also to encourage the user to either "update" the preset or to find out what went wrong with dict X.
There was a problem hiding this comment.
Meaning:
T(_("Some dictionaries from this preset have been deleted or are no longer available (%1 missing):"), whatever) .. "\n\n• %2"There was a problem hiding this comment.
Also if that %1 missing is a number it should be ngettext.
There was a problem hiding this comment.
but I specifically wrote it so it wouldn't need ngettext, it works for one or many...
edit: or just remove the number
There was a problem hiding this comment.
it works for one or many...
In English. But yes, the less you bother translators with the better.
16bddce to
3049b9c
Compare
re-arrange menu order @Frenzie's string suggestions
f8bcb11 to
5b50c62
Compare
| if module.ui.hotkeys:updatePresetReference(update_key_for_module, preset_name, new_name) then -- luacheck: ignore 542 | ||
| -- Hotkeys plugin manages its own 'updated' flag. | ||
| end | ||
| end |
There was a problem hiding this comment.
Shouldn't you just run that without the if? This doesn't really look like a valid ignore use case.
There was a problem hiding this comment.
Fair enough, will change it
4efaae6 to
6322c0d
Compare
| return | ||
| end | ||
| if Presets.validateAndSavePreset(new_name, preset_obj, presets[preset_name]) then | ||
| presets[preset_name] = nil |
There was a problem hiding this comment.
How we usually rename a key in a table?
t[new_key] = t[old_key]
t[old_key] = nilYou break this two connected lines, hiding the saving into other function.
More calls, broken logic. Unnecessary passing of arguments (like dialog_instance).
(Name duplication check should be separated from the caller callback)
There was a problem hiding this comment.
It's so simple:
koreader/frontend/apps/filemanager/filemanagercollection.lua
Lines 1229 to 1241 in 4dc4339
koreader/frontend/apps/filemanager/filemanagercollection.lua
Lines 1271 to 1280 in 4dc4339
(I just rename the menu items. But the preset list is sorted so the menu should be rebuilt, yes)
| local sub_item_table = item.sub_item_table | ||
| if item.sub_item_table_func then | ||
| sub_item_table = item.sub_item_table_func() | ||
| sub_item_table = item.sub_item_table_func(self) |
|
|
||
| function ReaderDictionary:onShowDictionaryLookup() | ||
| local buttons = {} | ||
| local preset_names = Presets:getPresets(self.preset_obj) |
| }, | ||
| }, | ||
| } | ||
| local preset_names = Presets:getPresets(self.ui.dictionary.preset_config) |
| align = "left", | ||
| text = preset_name, | ||
| callback = function() | ||
| self.ui.dictionary:loadPreset(self.ui.dictionary.preset_config.presets[preset_name], true) |
| end | ||
| -- then enable only those from the preset that still exist | ||
| for _, ifo in ipairs(available_ifos) do | ||
| for _, enabled_name in ipairs(preset.enabled_dict_names) do |
There was a problem hiding this comment.
To avoid loops doubling and nested loops it's better to use hash table instead of array:
function ReaderDictionary:buildPreset()
local preset = { enabled_dict_names = {} }
for _, name in ipairs(self.enabled_dict_names) do
preset.enabled_dict_names[name] = true
end
return preset
endIt's not more expensive, tableDeepCopy iterates over the table in the same way.
And then:
local dicts_disabled, valid_enabled_names = {}, {}
for _, ifo in ipairs(available_ifos) do
if preset.enabled_dict_names[ifo.name] then
table.insert(valid_enabled_names, ifo.name)
else
dicts_disabled[ifo.file] = true
end
end|
Code duplication: koreader/frontend/ui/presets.lua Line 182 in 225d6c7 koreader/frontend/ui/presets.lua Line 264 in 225d6c7 You can avoid arg passing and close the input dialog once right near koreader/frontend/ui/presets.lua Line 160 in 225d6c7 |
| -- Show a message if any dictionaries from the preset are missing. | ||
| if not skip_notification and #preset.enabled_dict_names > #valid_enabled_names then | ||
| local missing_dicts = {} | ||
| for _, preset_name in ipairs(preset.enabled_dict_names) do |
There was a problem hiding this comment.
preset.enabled_dict_names is now a hash table.
util.tableSize instead of #
pairs instead of ipairs
| toggle_status_bar = {category="none", event="ToggleFooterMode", title=_("Toggle status bar"), reader=true}, | ||
| toggle_chapter_progress_bar = {category="none", event="ToggleChapterProgressBar", title=_("Toggle chapter progress bar"), reader=true}, | ||
| load_dictionary_preset = {category="string", event="LoadDictionaryPreset", title=_("Load dictionary preset"), args_func=ReaderDictionary.getPresets, reader=true}, | ||
| cycle_dictionary_preset = {category="none", event="CycleDictionaryPresets", title=_("Cycle through dictionary presets"), reader=true,}, |
There was a problem hiding this comment.
We can look up dictionaries from the file browser. Maybe allow these actions as well?
|
No more comments, thanks. |
| end | ||
|
|
||
| function ReaderDictionary:loadPreset(preset, skip_notification) | ||
| if not preset.enabled_dict_names then return end |
There was a problem hiding this comment.
under normal usage, probably not. it's more a safeguard if a preset gets corrupted or a user accidentally deletes the content.
| UIManager:show(InfoMessage:new{ | ||
| text = _("Some dictionaries from this preset have been deleted or are no longer available:") .. "\n\n• " .. table.concat(missing_dicts, "\n• "), | ||
| }) |
There was a problem hiding this comment.
Just curious: what's the procedure/steps for a user getting this message to get rid of it?
There was a problem hiding this comment.
one off, pressing back or tapping outside of the widget.
longer term, (which I assume is what you a getting at here), you would need to add back the same dict or "update" the profile. preset.
There was a problem hiding this comment.
longer term, (which I assume is what you a getting at here)
Right.
you would need to add back the same dict or "update" the profile. preset.
That is this "update" I was wondering about. Ok, I go there, I just "update" and "ok" it ? It will clean it up from the old dict all by itself ?
There was a problem hiding this comment.
What?
It will run the build function again and not add that missing dict. Effectively removing it from the preset.
There was a problem hiding this comment.
Ok, that's what I was asking and the answer I was hoping for.
(It will not crash, or it will not bug because the dict does not exist, or keep it because it doesn't exist, etc...)
| self.preset_obj, -- preset object | ||
| _("Create new preset from current settings"), -- optional: custom text for UI menu | ||
| function() return self:hasValidSettings() end, -- optional: function to enable/disable creating presets |
There was a problem hiding this comment.
(you might want to align the comments)
also probably: optional: function to enable/disable creating presets presets submenu
There was a problem hiding this comment.
What do you mean? The function stops you from creating them not from accessing them. At least in the case of reader dict that’s the case
that function specifically enables/disables only the create button
|
Actually hold off on merging for a second, I still need to fix the docs. |



what's new
This pull request introduces a new unified preset management system for KOReader modules, centralising the creation, loading, updating, and deletion of presets. The changes refactor existing preset handling in
ReaderFooterso that any module can use the newPresetsmodule, simplifying code and improving maintainability. Additionally, we add preset functionality toReaderDictionaryand new dispatcher actions are added for loading dictionary and footer presets.Presetsmodule (frontend/ui/presets.lua) that provides a standardised interface for managing presets across modules, including methods for creating, loading, updating, and deleting presets. It also supports generating menu items and integrating with Dispatcher.ReaderDictionaryto use the newPresetsmodule for managing dictionary presets, replacing custom logic with standardised methods (createModulePreset,genModulePresetMenuTable, etc.).ReaderFooterto leverage thePresetsmodule for footer presets, significantly reducing the complexity of preset-related methods.load_dictionary_presetandload_footer_preset), enabling gesture or hotkey-based preset switching.screenshots
expand for more screenshots
related issues
This change is