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

Find in the hotkeys popup #3970

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

kosorin
Copy link
Contributor

@kosorin kosorin commented Oct 18, 2024

Implements #3963

  • find in description only
  • no fuzzy or other sophisticated searching
  • uses awful.prompt.run
  • show_help() does not return awful.keygrabber (breaking change?)

Theme variables

Theme Default value Description
highlight_bg "#ffff00" The highlighted text background color.
highlight_fg "#000000" The highlighted text foreground color.
find_fg_cursor - The find prompt cursor foreground color.
find_bg_cursor - The find prompt cursor background color.
find_ul_cursor - The find prompt cursor underline style.
find_prompt "<b>Find: </b>" The find prompt text.
find_font font The find prompt text font.
find_margin group_margin Margin around the find prompt.

hotkeys_popup_find

@actionless
Copy link
Member

actionless commented Oct 18, 2024

thanks for submitting, i found small issue right from the description (and left a separate comment), i'll start reviewing the code itself later on in upcoming days or mb even hours

@actionless
Copy link
Member

and also the test is failing:

  • /home/runner/work/awesome/awesome/tests/test-awesomerc.lua: Error: running function for step 13/18 (@3): /home/runner/work/awesome/awesome/tests/test-awesomerc.lua:289: assertion failed!

@actionless
Copy link
Member

regarding keygrabber question, i'll need first to understand:

  1. what was the initial intention of returning it there
  2. why it's not possible to do it anymore

@actionless
Copy link
Member

also, mb add a bool option to args for closing hotkeys on a any keypress (the current behavior), so the users who used to that, could restore that old default behavior

@actionless
Copy link
Member

i think find_prompt should be moved from theme option to args

@kosorin
Copy link
Contributor Author

kosorin commented Oct 19, 2024

regarding keygrabber question, i'll need first to understand:

1. what was the initial intention of returning it there

2. why it's not possible to do it anymore

Because I use awful.prompt I no longer have access to the underlying keygrabber. Why was it returning keygrabber? I don't know, it doesn't make much sense to me.

Actually, I need to check if the prompt is stopped gracefully. Right now I just call awful.keygrabber.stop().

also, mb add a bool option to args for closing hotkeys on a any keypress (the current behavior), so the users who used to that, could restore that old default behavior

I added show_args.enable_find (enabled by default). If false, the prompt is not visible and the popup is hidden on any key.

i think find_prompt should be moved from theme option to args

I don't know. What are the odds that you want multiple prompts for different hotkey popups? It's just a simple text "find: ", nothing more so I thought was easier for user to set it once in the theme.

@actionless
Copy link
Member

actionless commented Oct 19, 2024

What are the odds that you want multiple prompts for different hotkey popups

it wasn't a point about having multiple different prompts, but about untying translation and style - and as you can see from our existing themes (and defined theme variables in the awful/wibox/etc builtin libraries), it's indeed not a used practice storing such sort of info there

I added show_args.enable_find (enabled by default). If false, the prompt is not visible and the popup is hidden on any key.

thanks 👍

Because I use awful.prompt I no longer have access to the underlying keygrabber. Why was it returning keygrabber? I don't know, it doesn't make much sense to me.

ok i'll try to dive into git blame to figure that out 😺

@actionless
Copy link
Member

style checking is failing in the CI btw

Copy link
Member

@actionless actionless left a comment

Choose a reason for hiding this comment

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

⬆️

Copy link

codecov bot commented Nov 30, 2024

Codecov Report

Attention: Patch coverage is 6.75676% with 138 lines in your changes missing coverage. Please review.

Project coverage is 48.39%. Comparing base (206e6e1) to head (38b3f8a).
Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
lib/awful/hotkeys_popup/widget.lua 6.75% 138 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #3970       +/-   ##
===========================================
- Coverage   93.90%   48.39%   -45.51%     
===========================================
  Files         869      195      -674     
  Lines       51353    22523    -28830     
===========================================
- Hits        48221    10900    -37321     
- Misses       3132    11623     +8491     
Files with missing lines Coverage Δ
lib/awful/hotkeys_popup/init.lua 100.00% <ø> (ø)
lib/awful/hotkeys_popup/widget.lua 34.43% <6.75%> (-63.82%) ⬇️

... and 816 files with indirect coverage changes

@actionless
Copy link
Member

style checking is failing in the CI btw

this is still actual

@actionless
Copy link
Member

also, this suggestion may be out of scope of this PR, but would be nice to have an option instead of just highlighting search results - to show only the matching items (and their respective group headers)

@kosorin
Copy link
Contributor Author

kosorin commented Dec 1, 2024

style checking is failing in the CI btw

this is still actual

I see, I fixed it and now there is another issue. How do you setup your local dev environment so I can see these warnings/errors before pushing?

also, this suggestion may be out of scope of this PR, but would be nice to have an option instead of just highlighting search results - to show only the matching items (and their respective group headers)

Widgets' positions are calculated only once when they are created. Then they are cached. Highlighting the text will not change their position. However, hiding the text requires recalculating all positions. Which is a non-trivial task given the code. So maybe some other time...

@actionless
Copy link
Member

on arch linux it's normally enough to have the same libraries as in makedeps of the PKGBUILD

for ubuntu see:

https://github.com/awesomeWM/awesome/blob/master/.github/workflows/main.yml

common instructions for other distros are here:

https://awesomewm.org/apidoc/documentation/10-building-and-testing.md.html

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