reedline icon indicating copy to clipboard operation
reedline copied to clipboard

Add inapplicable condition on menu activation and navigation

Open Abdillah opened this issue 2 years ago • 6 comments

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

Fixes #603. Fixes #604.

Abdillah avatar Jul 20 '23 07:07 Abdillah

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.

sholderbach avatar Jul 20 '23 20:07 sholderbach

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 avatar Jul 21 '23 03:07 Abdillah

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 avatar Jul 21 '23 23:07 Abdillah

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

Abdillah avatar Jul 28 '23 12:07 Abdillah

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%) :arrow_down:

... and 3 files with indirect coverage changes

codecov[bot] avatar Oct 21 '23 05:10 codecov[bot]

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.

Abdillah avatar Oct 22 '23 22:10 Abdillah