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

Add inapplicable condition on menu activation and navigation #608

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Abdillah
Copy link
Contributor

@Abdillah Abdillah commented Jul 20, 2023

Allow until keybinding with send: menu* (activation and navigation) stacked with other action by defining some Inapplicable conditions.

Fixes #603.
Fixes #604.

@sholderbach
Copy link
Member

sholderbach commented Jul 20, 2023

Thanks for trying to address this! With the failed tests I am not sure if this breaks any assumptions. We also should test this behavior with different menus not just the basic completions. e.g. the help menu in nushell you can load with F1 (e.g. if there is only one example and one command)

EDIT: The CI failure seems to come from cargo clippy, so it just seems a change to .is_empty() instead of a verbose comparison.

@Abdillah
Copy link
Contributor Author

Abdillah commented Jul 21, 2023

I am not sure if this breaks any assumptions. We also should test this behavior with different menus not just the basic completions

Sure, I have this branch set on my daily driver, just let me know if something need to be tested.

It seems help menu indeed not called, I will investigate this case.

@Abdillah
Copy link
Contributor Author

Abdillah commented Jul 21, 2023

I can confirm that help menu is empty on the conditional I added.

This is because on DescriptionMenu.menu_event(Activated), unlike completion menu, DescriptionMenu just toggles active state and doesn't call its update_values (linked). My changes on other events likely fine because the menu's value must has been updated.

The subsequent updates of the menu happened after repaint -> menu.update_working_details -> menu.update_value.

I think, I will make the auto-close only works on quick menu only since it includes auto-completion menu (which is Tab key main target). Moreover, the update_value already called in this case.

@Abdillah
Copy link
Contributor Author

So far, everything works smoothly in my config: help menu, completion, completion hint. I think this branch is ready for review.

@Abdillah Abdillah force-pushed the feature/addmenuinappl branch from e372084 to 0c27b0a Compare October 21, 2023 05:44
@codecov
Copy link

codecov bot commented Oct 21, 2023

Codecov Report

Merging #608 (0c27b0a) into main (adc20cb) will decrease coverage by 0.75%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #608      +/-   ##
==========================================
- Coverage   49.76%   49.01%   -0.75%     
==========================================
  Files          41       42       +1     
  Lines        7827     7946     +119     
==========================================
  Hits         3895     3895              
- Misses       3932     4051     +119     
Files Coverage Δ
src/engine.rs 5.02% <0.00%> (-0.12%) ⬇️

... and 3 files with indirect coverage changes

@Abdillah
Copy link
Contributor Author

Do I need to do something on the @codecov, @sholderbach? I don't see any test in engine.rs file.

Also, if there is something I can verify or do regarding the merge of this PR, I would gladly do that.

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