[KOPlugin] Hotkeys, add custom keyboard shortcuts#12484
Conversation
|
It might help to take some inspiration from https://github.com/koreader/koreader/blob/master/plugins%2Fgestures.koplugin%2Fmain.lua#L390 The trick is to open a dialog and register a handler for every type of key press, like is done for every type of gesture in the snippet below. Not sure if our current key code can handle this. |
|
This would probably need to be extended: You can add a event for Also you need to deal somewhere with priorities (do shortcuts override default keybindings or not)? |
|
See this comment for how to store the key name in settings koreader/frontend/device/key.lua Line 34 in adbf3be Tldr, you can just cast the key object to a string and it will be sane |
as for this bit, great idea but i would prefer not to do that just now. I would be using this on kindle (3 and 4) so the 10 to 50-ish combinations are already plenty enough, for other devices that have keyboards though, it could be something to add in later. Furthermore, I want to keep some key mappings (shift+alt+whatever) open in case of needing to hard code something. I also don't want to support single key shortcuts (besides
now this, i am aware of and i'm currently thinking about this very issue. I wanted to simply do something like (i know it’s not handling all cases there, i.e plugin being removed) function ReaderToc:registerKeyEvents()
local hotkey_plugin = self.ui.hotkeyshortcuts and not self.ui.hotkeyshortcuts.disabled
if Device:hasScreenKB() and hotkey_plugin then
self.key_events.ShowToc = { { "ScreenKB", "Up" } }
elseif Device:hasKeyboard() and hotkey_plugin then
self.key_events.ShowToc = { { "T" }, { "Shift", "Up" } }
end
end |
|
We can either do it easy or do it right :) The right way would be to duplicate a lot of the gesture handling in input esp It will probably be a bunch of effort of reading the |
we? are you somehow suggesting that you are going to help get this to be the best it can possibly be? Because if that is the case, okay, we can try, you have the know-how. But if you are expecting me to figure it out by myself, and implement every single lesson learned by everyone that has ever worked on the gestures plugin here, well... you're setting yourself up for disappointment as that is just not going to happen. If it comes to go big or go home, I am going home. But don't get me wrong, I am not saying that we should release something bad, what I am saying is, that I am not going to do all that work that has happened over many years and by many people (including yourself), either alone, or now (for this PR). So as I said, if anyone really wants to custom-create shortcuts (instead of using pre-defined ones), they can either wait, until I can get there some time in the future (post-this-PR) or grab a keyboard and help out.
this is a very tall order (for me at least, a non developer I must add in case you don't know) for one man and little time, Rome was not build in one day. What I have here in this PR so far, is far more than what I personally need. So if worst comes to worst, I'll just run it locally and we can all go from having some sort of shortcut support to nobody having any, (except for me, because I will be running this) for better or worse. So, making that clear, what are legitimate things that could be reasonably implemented here? |
I don't think anyone (besides me) has in very many years hardcoded any type of key_events besides the bare minimum functionality. I also very much doubt that anyone is going to do so in future, especially with something like this PR. Now all one needs is to add more events to dispatcher. So I am not truly concerned about more modifier events and their potential future priority (although I might be wrong and it might bite me in the ass later). |
Commodore64user
force-pushed
the
hotkeyshortcuts
branch
from
997eabd to
1cb0aac
Compare
|
|
@poire-z sorry to bring you back from hibernation. I was wondering if you had any idea why CI is giving those errors and how to solve them. Also, have a look around. |
|
You'll have more info for the why of these error above the bottom where they are summarized, using Ctrl-F to find the filename:linenumber ie.:
Well, I have not enough brain bandwidth to dig into and appreciate your work. |
|
hoping @NiLuJe joins the dark side and has a look around here. |
|
Extremely unlikely ;). |
|
should I assume that the lack of interest means I should put it to rest? |
|
No. The fact that you managed to make it a standalone plugin (without touching core) is good. |
|
At a quick glance it looks a lot more appealing than the previous version. ^_^ |
| @@ -0,0 +1,117 @@ | |||
| -- typed on my Commodore 64 ;) | |||
There was a problem hiding this comment.
Correct, and we are still talking about it. How’s that for a conversation starter?
| modifier_plus_up = nil, | ||
| modifier_plus_down = nil, -- keep nil. | ||
| modifier_plus_left = nil, | ||
| modifier_plus_right = nil, | ||
| modifier_plus_left_page_back = nil, -- keep nil. |
There was a problem hiding this comment.
What's the difference between lines with = nil, and those with = nil, -- keep nil..
If there is one, write about it so one newcomer like me can get it. If none, remove it so I won't ask the question again :)
There was a problem hiding this comment.
My reading would be something like "already assigned outside the plugin." Which if correct is probably a better comment. ^_^
There was a problem hiding this comment.
that is exactly the case, i was originally going to write it differently but apparently never got around it.
There was a problem hiding this comment.
I still don't get the differences.
modifier_plus_down = nil, -- keep nil, already assigned outside this plugin.
modifier_plus_left = nil,
modifier_plus_right = nil,
modifier_plus_left_page_back = nil, -- keep nil, already assigned outside this plugin.How I am supposed to understand the differences between those with that comment and those without ?
There was a problem hiding this comment.
the difference is, one is nil because I don't want to assign anything to it (let the three people that will use it decide for themselves), the other other one is nil with -- keep nil to indicate that nothing should ever be assigned to it because something is already using them. More importantly though, there is no real reason to touch those default values ever again...
for example:
modifier_plus_press = nil, -- keep nil, already assigned outside this plugin.
modifier_plus_menu = nil, -- keep nil, already assigned outside this plugin.modifier_plus_press is assigned to long-press or hold, so we don't want to mess with that. It happens to be free inside the reader, so we can in fact use it there. modifier_plus_menu is used globally for screenshots on K4, so again, don't mess with it. and so forth...
| local fm_do_not_press = { press = true } | ||
| util.tableMerge(reader_only, fm_do_not_press) |
There was a problem hiding this comment.
Why is it named "do_not" ?
There was a problem hiding this comment.
because you are not suppose to press it... i guess, i can't remember exactly why i named it that. Nevertheless it is never used again.
| return G_reader_settings:isTrue("press_key_does_hotkeyshortcuts") | ||
| end, | ||
| callback = function() | ||
| G_reader_settings:flipNilOrFalse("press_key_does_hotkeyshortcuts") |
There was a problem hiding this comment.
Does this press_key_does_hotkeyshortcuts already exists ? Is it used by core ?
If not, and you're adding it, can't it be stored in your hotkeyshortcuts.lua, so all is in one place?
There was a problem hiding this comment.
it doesn't exist elsewhere, it is crreated here and used here only.
There was a problem hiding this comment.
why can't i have something nice though?
There was a problem hiding this comment.
(Because you just haven't earned it yet, baby.)
Seriously: what ?
There was a problem hiding this comment.
can't we just leave it with the G_settings? they have some too... (points at gesture plugin).
There was a problem hiding this comment.
It's just a matter of storing it as:
self.hotkeyshortcuts.press_key_does_hotkeyshortcuts
-- which ends up stored as:
self.settings_data.data["press_key_does_hotkeyshortcuts"]There was a problem hiding this comment.
You can if you really don't want to bother (but you won't have something "nice").
Mmm… didn’t you say it was fine?
There was a problem hiding this comment.
I guess this PR will wait for after next release, so you still have time to try making something nice :)
There was a problem hiding this comment.
But it is already nice.
There was a problem hiding this comment.
You indeed made it nice by making it a standalone plugin.
The only non-standalone thingy left is the use of G_reader_settings for that single internal setting. So, there's room to make it even nicer.
| function HotKeyShortcuts:overrideConflictingFunctions() | ||
| local ReaderBookmark = require("apps/reader/modules/readerbookmark") | ||
| ReaderBookmark.registerKeyEvents = function(readerbookmark) | ||
| end |
There was a problem hiding this comment.
overrideConflictingFunctions() seems to be called always when the plugin is enabled ?
And when the user is not interested by this plugin, you will just remove the original key registration, and plug back an equivalent/identical mapping, right ?
Feels a bit strange, but probably easier than detecting if needed and restoring when the assignment gets back to be similar to the defaults. So... ok.
Anyway, would read simpler with these on a single line when the func is a no-op:
ReaderBookmark.registerKeyEvents = function(readerbookmark) end
There was a problem hiding this comment.
it is called as part of registerKeyEvents() during init so it is only enabled if the plugin is actually active. If someone decides to have the plugin off, it reverts back to core's keyEvents()
| function HotKeyShortcuts:updateProfiles(action_old_name, action_new_name) | ||
| for _, section in ipairs({ "hotkeyshortcuts_fm", "hotkeyshortcuts_reader" }) do | ||
| local hotkeyshortcuts = self.settings_data.data[section] | ||
| for shortcut_name, shortcut in pairs(hotkeyshortcuts) do |
There was a problem hiding this comment.
Not familiar with profiles, so it probably needs a quick reading by @hius07 (even if probably copy & pasted from the gestures plugin.)
There was a problem hiding this comment.
yes, no changes here.
| local exec_props = { hotkeyshortcuts = hotkey } | ||
| Dispatcher:execute(action_list, exec_props) |
There was a problem hiding this comment.
May be needs a quick reading by @yparitcher for the dispatcher re-using.
| - FileSearcher: Customizes `ShowFileSearchBlank` key event for devices with keyboards. | ||
| - FileManagerMenu: Customizes `ShowMenu` key event for devices with keys. | ||
| ]] | ||
| function HotKeyShortcuts:overrideConflictingFunctions() |
There was a problem hiding this comment.
Since this plugin appears to be actually handling key input, these registerKeyEvents in all the modules should be removed and folded in to the plugin, much less of a maintenance headache. Similar to how many of the module gestures were moves to the plugin. As @poire-z said nilling them to register the same ones in the plugin doesn't make much sense
There was a problem hiding this comment.
I actually don't mind that. I'd rather keep having the default key behaviour in each core module, and not have to care about this plugin (which may be disabled - and NT users could be left with no key working at all :)).
There was a problem hiding this comment.
the other thing is, there is really no reason to change those functions as I don't expect new devices will release with spare keys (out of your usual page-turn keys and perhaps menu etc) that would require adding new key mappings. therefore, there should be no headaches at all.
There was a problem hiding this comment.
Gamepads, BT page turners and the like remain "new" devices from the code's perspective that have always been planned to be combined with any work on keyboard shortcuts how @yparitcher stated. That remark was remarkably successful at introducing worry where there was essentially none. ;-)
|
would anyone like to have a look at the |
|
The event is simply one that comes from Dispatcher? |
Yes, but even if you hardcode it (so it doesn't go through dispatcher), still won't work. I think i might have mentioned that early on the development of this idea (when it was just moving the bottom menu to the top, remember that? Feels like light years away) |
Discrepancies are "intended" to arise from changing the user-facing string while the underlying code and settings are already long established. :-) |
Consistency ? There is currently not a single "irrelevant funny line" (there are a few relevant funny lines by @NiLuJe though). (typed on my Casio toy piano - hoping to bring joy and amusement to your little broken heart) |
Are you seriously going to go full Grinch on me?, You're supposed to be the fun one... ;) besides, my hilarious quip ( |
Well, I was trying to not have to go that far and hurtful :/ but if I must go there: This is not funny. |
I said fun, not funny. Besides the winky, smiley face at the end of the comment begs to disagree... |
Commodore64user
changed the title
|
I went ahead and risked all, renamed everything to just 'hotkeys'. @mergen3107 if you update your code to this (all three files have changed since), you are going to need to manually rename the @Frenzie and how do we arbitrate our defaults.lua line 1? |
|
"I tried to read a book on my C64, but it kept saying press play on tape" |
Line 2? |
|
But I don't quite understand why you're so recalcitrant about one little comment line. Removing it is not an unreasonable request. It's just kind of there otherwise, which is the crux of the argument. |
|
Because I want to have a little eater egg, we all want to leave a little mark somewhere; it is both homage to the machines of yesteryear and a little nod to the plugin's author ;). It's fun and kinda hidden from everybody, that's why I thought it wouldn't be a big deal. @poire-z doesn't like it, I do. I expect you will use your wisdom to settle the matter. I will abide by it. |
|
Is there some alternative phrasing in the same spirit both of you might agree on? "typed on a Commodore 64" as a minimal change for example. |
|
If you think some alternative phrasing of something irrelevant would just be ok, go ahead with merging the original :) |
|
Sometimes you have to take a risk. Hopefully it'll pay off rather than come back to bite. ;-) |
|
As always, thank you all for the help provided along the way (@poire-z as always a star) @mergen3107 thank you as well for testing (even though it made me quite angry at some point 🤣), I am also glad we could come to an agreement. See you on the next one, I guess. Oh, and don’t forget the little PR with the profiles bit… Let it be known that this whole plugin came about because I didn’t want the press key to activate the bottom menu but @Frenzie thought we were crossing the Technical Debt line with my simplistic original idea… it goes to show, inspiration can come from anywhere. |
|
@Commodore64user Here is the attached archive with both. Is there something wrong with the nightly or my config? |
|
Ah, I guess, Shouldn't there be a proper migration? |
|
FWIW when I tried OTA update, it downloaded it, but then failed and offered to download the full archive, so I did. Installed OK. |
|
Ah, I see. Commits show that folder and plugin name got changed before merging. |
|
Thanks! All worked |
|
I am trying to rewrite the opening bit, see here: but I am getting an error |
|
It's the |
7 participants
what's new
This plugin allows users to configure and use hotkeys for various dispatcher actions/events.
It provides a flexible way to map keys to specific actions based on device and user settings.
features:
key functions:
usage:
screenshots:
This change is