Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

* layers/+web/eww/funcs.el: Fix eww transient state key bindings #16730

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

sunlin7
Copy link
Contributor

@sunlin7 sunlin7 commented Dec 17, 2024

Warning message happened when enable the eww layer:

Warning (emacs): Unrecognized key: _+_ 
Warning (emacs): Unrecognized key: _w_ 
Warning (emacs): Unrecognized key: _-_ 
Warning (emacs): Unrecognized key: _=_ 

It caused by the transient state for eww has post-defined keys, but the keys not available during transient hints initilizating.

The is PR will fix the issue by define the keys with identify function first, then replay them with the real keys.

@sunlin7 sunlin7 marked this pull request as ready for review December 17, 2024 06:09
Copy link
Collaborator

@bcc32 bcc32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide some more details? I'm not able to reproduce the issue you described. I added the eww layer, started a new Emacs, and didn't see any warnings at startup, or when I started eww, or when I started using the transient state.

layers/+web/eww/funcs.el Outdated Show resolved Hide resolved
layers/+web/eww/funcs.el Outdated Show resolved Hide resolved
@sunlin7 sunlin7 force-pushed the eww-fix-trans-state branch from 5bf28bc to ba9f7b4 Compare December 18, 2024 06:09
@sunlin7
Copy link
Contributor Author

sunlin7 commented Dec 20, 2024

@bcc32 Could you try the eww layer with emacs-28 branch? It constantly reproducible on Emacs 28.

@sunlin7
Copy link
Contributor Author

sunlin7 commented Dec 20, 2024

I tried emacs-29, this issue is also reproducible.

@bcc32
Copy link
Collaborator

bcc32 commented Dec 20, 2024

Hmm, I think I know what this is. I can reproduce the issue you describe if I have a running Spacemacs, and I add the eww layer, and then I press SPC f e R to install the layer after startup is complete. The issue does not occur if I add the eww layer and then start a new Spacemacs. Is that the behavior you're describing? It's not clear from your previous comments.

If that's what we're talking about, this is not really a bug in the eww layer, I would argue. (Otherwise we would have to fix every single callsite of spacemacs/transient-state-register-add-bindings and not just eww).

spacemacs|define-transient-state is supposed to already deal with this by deferring the actual defhydra using spacemacs/defer-until-after-user-config, so it normally works fine (defhydra always runs after the eww/post-init-zoom-frm function, for example).

If we're talking about the same thing, then I think this more a bug in SPC f e R. We should fix SPC f e R to run things in the correct order to avoid the spurious warning.

@sunlin7
Copy link
Contributor Author

sunlin7 commented Dec 20, 2024

@bcc32 It's not same as I described issue.
I found the key point to reproduce the issue:

dotspacemacs-distribution 'spacemacs-base
dotspacemacs-configuration-layers '(eww)

@bcc32
Copy link
Collaborator

bcc32 commented Dec 20, 2024

Can you give a full config to reproduce? I of course already tried adding eww to my layers list (and I'm already using the spacemacs-base distribution).

@sunlin7
Copy link
Contributor Author

sunlin7 commented Dec 20, 2024

@bcc32 The configuration is the default content of core/templates/.spacemacs.template with two lines changes:

dotspacemacs-distribution 'spacemacs-base
dotspacemacs-configuration-layers '(eww)

I tried emacs-28/29/30 on Ubuntu 20.04, the warning messages will be displayed on startup,

Warning (emacs): Unrecognized key: _+_ Disable showing Disable logging
Warning (emacs): Unrecognized key: _w_ Disable showing Disable logging
Warning (emacs): Unrecognized key: _-_ Disable showing Disable logging
Warning (emacs): Unrecognized key: _=_ Disable showing Disable logging

Please keep the warning-suppress-log-types to be nil.

@sunlin7
Copy link
Contributor Author

sunlin7 commented Dec 20, 2024

I'm not sure any other additional layers/package may hide this issue.

@bcc32
Copy link
Collaborator

bcc32 commented Dec 20, 2024

Ohhhh. You're saying to make eww the only layer. That wasn't clear at all from your previous messages. (In general, for the avoidance of doubt, it is best to just paste a full .spacemacs that can be directly loaded to attempt to reproduce, otherwise, there is always the chance of misinterpretation).

Indeed, if the packages like zoom-frm and writeroom-mode are simply not enabled, the warning is issued. Let me take another look at this, then.

@bcc32
Copy link
Collaborator

bcc32 commented Dec 20, 2024

I pushed a new proposed solution to your branch. It attempts to be more general and reusable across other transient states, while requiring minimal duplicated code (such as regexes to match each key hint) in each transient state definition.

Unfortunately it doesn't work for the evil-mode key bindings here, but if I'm understanding your original comment correctly, the evil mode keys are actually never missing. (In fact, spacemacs-bootstrap requires evil so I don't think basically anything else in Spacemacs supports working without loading evil-mode). So, I did not attempt to handle the formatting in that case.

@sunlin7
Copy link
Contributor Author

sunlin7 commented Dec 20, 2024

@bcc32 Thank you, and the the changes are more elegant.
I had test it on my local, there has warning
Warning (emacs): Unrecognized key: _+_ Disable showing

And the transient status menu has a [_+_] in the menu list, cause a layout issue.

 Navigation                Layout/Appearance            Zoom              List/view          Other
 ------------------------- ---------------------------  ----------------- ------------------ ----------------------------
  [h/j/k/l] scroll l/d/u/r [v] toggle visual-line-mode  [_+_] zoom-in       [W] list buffers   [r] reload page
  [H/L] prev/next eww-buff                                                [S] list histories [x] view in external browser

@sunlin7
Copy link
Contributor Author

sunlin7 commented Dec 20, 2024

How to deal the dynamic loaded feature? I mean when the transient state menu started, a package (eg: zoom-frm) did not been loaded yet, but some how the package was loaded for some reason, then re-run the transient state menu, the menu items should be presented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants