Hi Colin, Thanks for your review. Please see my inline comments.
On Mon, Sep 16, 2013 at 5:26 PM, Colin Watson <cjwat...@ubuntu.com> wrote: > On Mon, Sep 16, 2013 at 04:49:46PM +0800, Yang Bai wrote: >> At now, sleep --interruptible 3 can only be interupted by ESC key. >> With this patch, we can special a key such as sleep --interruptible >> f10 3 and we can type F10 to interrupt the sleep. This can work as a >> hotkey handler. > > This patch still duplicates key aliases from > grub-core/commands/menuentry.c, only it's slightly out of sync and has > its table in a different order for no discernible reason. This is an > excellent illustration of why that table should be in only one place in > the source code. What about moving this table out of the commands/menuentry.c and put it into include/grub/term.h as a map from key's name to GRUB_KEY_CODE? And every code uses this struct just include this header. > > Changing "sleep --interruptible" to require a string argument breaks a > user-visible interface. Please do not do this. What about add an repeatable option maybe called "--key" so this will not break a user-visible interface and handle multi-key? > > Requiring the hotkey to be configured in two locations (once in the > menuentry command, once in "sleep --interruptible") is cumbersome. It > also does not support recognising multiple hotkeys (i.e. any of those > configured for any menuentry command) at the hiddenmenu stage. > > This patch does not pass hotkeys on to the menu. As a result, you will > in practice end up pressing the hotkey twice to actually boot the > hotkeyed menu entry. Maybe we can use this as: if sleep --interruptible --key f10 3; then menuentry blahblahblah... else set normal_boot=true fi if normal_boot; then blahblahblah fi So that we can map a special hotkey to a special menuentry. > > I think you have misunderstood the UI requirement here, and as a result > I don't think this patch is the right approach. Sorry. > > -- > Colin Watson [cjwat...@ubuntu.com] > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
