Implement item shortcuts

[AydinHassan]

I've implemented shortcuts based on the code from #171 with support for getting the selected item. I've done it a bit different, in the builder. I think that's the better place for something like this. Eg added convenience using the raw methods on CliMenu.

What do you think?

@Lynesth @vesper8 @mikeymike

Will write some docs if everyone likes :)

[codecov-io]

Codecov Report

Merging #176 into master will decrease coverage by 3.27%.
The diff coverage is 29.5%.

@@             Coverage Diff              @@
##             master     #176      +/-   ##
============================================
- Coverage     97.26%   93.99%   -3.28%     
- Complexity      423      466      +43     
============================================
  Files            25       26       +1     
  Lines          1317     1465     +148     
============================================
+ Hits           1281     1377      +96     
- Misses           36       88      +52

Impacted Files	Coverage Δ	Complexity Δ
src/Exception/InvalidShortcutException.php	0% <0%> (ø)	1 <1> (?)
src/CliMenu.php	93.25% <0%> (-3.63%)	101 <3> (+3)
src/Builder/SplitItemBuilder.php	74.28% <14.28%> (-15%)	10 <2> (+3)
src/Builder/CliMenuBuilder.php	82.88% <40.47%> (-12.39%)	62 <14> (+16)
src/Input/Text.php	88.37% <0%> (-11.63%)	17% <0%> (+6%)
src/Input/Number.php	88.46% <0%> (-11.54%)	23% <0%> (+10%)
src/Input/InputIO.php	98.9% <0%> (+1.95%)	29% <0%> (+4%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83fbce5...2426a48. Read the comment docs.

[vesper8]

looks good to me! I like the fact that you don't need to run anything extra to apply the shortcuts.

One minor thing I would add to the shortcuts example is change First option to [F]irst option to show/test that the same letter can be used twice (once on top menu and once on submenu) and that shortcuts also get applied to all submenus automatically. I haven't actually tested this but I assume it works since it is working on the code from #171

On a side note, did you notice what I said about the newly added method setDefaultControlMappings needing to be re-applied on submenus? I guess that makes sense but it's a bit undesired.. not sure how we could allow the user to set them so they are always applied. If this were a Laravel package project then you could achieve this through a configuration file (ideal) or by using the Cache helper (hacky) but not sure how it could be done otherwise.

[AydinHassan]

@vesper8 done :)

Regarding setDefaultControlMappings we discussed in #175 but I mentioned I'd like a different solution to that - maybe @Lynesth will have a crack, I'm not sure ! For me it would just be an option in CliMenuBuilder and it would apply it to all new submenus created via it.

[Lynesth]

I'll write a better review later but for now I would just like to say that it should be optionnal.
Ideally when setting the option we should be able to provide the regexp (defaulting to what is currently set).

Note: we will need to make sure that the custom regexp only returns one character.
Just checking that it contains "(.)" should be enough for our purpose (ie. preg_match('[^\\]\(\.\)', $regexp))
Just throw an error (or return null) in extractShortcut if the match is more that one character.

[AydinHassan]

@Lynesth that regex only matches a single character anyway.

I think setting the regex is a going a bit far. If that level of customisation is really needed then you could just use the custom control mapping on CliMenu directly.

I'll add a method to disable the shortcuts now.

[AydinHassan]

Docs and feature to disable added.

[Lynesth]

@AydinHassan I'm still convinced it should be the other way around (auto-shortcuts disabled by default) to maximize BC. And if adding a function to enable it, why not have it set the regexp on the fly ?

public function enableAutoShortcuts($regexp = '/\[(.)\]/') : self
{
    $this->autoShortcutsRegexp = $regexp;
    $this->autoShortcuts = true;
    return $this;
}

That way we can easily add items to the menu like those:

A. Turn it off.
B. Try to smash it.
C. Leave it like this.

without having to manually add custom controls (since automation seems to be the point of this PR) simply by using ->enableAutoShortcuts('#^(.)#')

[AydinHassan]

I guess a couple of reasons:

1. I don't believe complete customisation is better. I think leaner API's with less configuration that just work are better, rather than having to opt in.

2. Although technically there could be a BC break, realistically there won't be any breakages. How would there be? Someone using square brackets and already registered a custom key map for that single char that will be between the square brackets? Highly unlikely imo.

3. I think it's better to add features when they are needed/actually requested rather than trying to imagine all possible use-cases, it just leads to code bloat.

[vesper8]

Considering your own example for adding keyboard shortcuts makes use of a square bracket:

$menu = (new CliMenuBuilder)
    ->addItem('List of [C]lients', $myCallback)
    ->build();

// Now, pressing Uppercase C (it's case sensitive) will call $myCallback
$menu->addCustomControlMapping('C', $myCallback);

It would be bad if this push led to a fatal error for anyone that's implemented their key shortcuts in this manner, as I would imagine it's highly likely that people are using shortcuts in exactly this manner. If this doesn't lead to an error though then I think it's good the way it is. I am generally however always in favour of defaults that can be easily overridden.

[AydinHassan]

@vesper8 that will still work correctly!

[Lynesth]

In response to each of your points:

1. I don't like things imposed having to be removed. I also think that it feels much more natural to ask for things we want when we want them (calling the function to activate auto-shortcuts on purpose).

2. It will break for everyone who has already registered the key, even on the line it's meant to be since it will try to bind the same key twice.
   Furthermore, I added this for a client as the last line of a menu, it would also break because the executeAction is different than the action when using the shortcut.

// Added this item
$builder->addItem('Click here to go back or press [X] to quit the application', myFunction() {
    echo 'You went back';
});
// Then made this shortcut
$menu->addCustomControlMapping('x', function() {
    echo 'You exited';
});

3. I think me saying "If we are adding this feature, I would like this" is actually me making a request. And in this case, I'm not even changing the amount of code by more than a couple of lines.

[AydinHassan]

Right okay. Feel free to take over the PR.

[Lynesth]

Hmmm, I'm sorry if I made you mad or anything, conveying emotions with written words isn't easy (especially in another language).

I wasn't angry (and surely didn't want to make you angry), just trying to make my point but i'm really not sure I manage to convince you.

That's your project first and foremost so unless you're okay with my proposal I think you should stay true to your beliefs. Or maybe we could simply ask more people their opinion on this so that it isn't just our two point of views "clashing".

What do you say ?

[AydinHassan]

It's fine, I just don't have any more time to spare on this. I thought it would be easier 🙂

[vesper8]

I'm all for attaching an extra command to the flow in order to activate this automapping.. it makes sense for the user to need to ask for it.

Hope this can be re-opened quickly :) Can't wait to have this awesome functionality merged in!

Thank you both for your awesome work on this project!

[vesper8]

@Lynesth @AydinHassan Hey guys.. you both worked hard on this awesome PR.. I'm sure everyone that uses this package would love to have this functionality in. If you're worried about backwards compatibility then maybe just push a new major version out to include it?

This PR is just about done.. seems like a big waste to let it rot away at this point..

[AydinHassan]

@vesper8 maybe I can find some time in the next few days but a lot has happened recently, my partner has been in hospital and life is a bit complicated 😓

[vesper8]

@AydinHassan so sorry to hear about your partner, i hope they're ok and things are stable. my step-dad has been in and out of the hospital as of late and is on the list for a heart transplant so i know what it's like. of course this is a priority over any code project.. i'm sure you understand how i feel, about a few hours of work having already been spent on this PR and it just needs another little bump to get to the finish line, would be a shame if it never does.. maybe at least re-open the PR for now.. and when you have time, even if that's weeks from now, then you can make those last changes for backwards compatibility and merge it in. I think it will be a fine addition! I've been using my own implementation of automap shortcuts that started off this PR and I've been finding it immensely useful. I'm sure others would to

Cheers and good health to you and yours!

[AydinHassan]

@vesper8 @Lynesth I've switched the default around and added in the ability to swap the regex. It required a bit more code to propagate to submenus but it seems okay. Will merge later tonight if no comments.

[Lynesth]

Hey there, sorry I disappeared, I'm having some health issues and will not be able to contribute for some time.

This PR looks fine to me. Thank you for taking my opinion in account on this matter I really appreciate it.

Kind regards

[Lynesth]

I guess the only thing missing the the explanation about the regex in the docs.
Appart from that it sounds good :)

[AydinHassan]

Great thanks. The regex docs can come later.

Sorry about your health. I hope it's not too serious and wish you all the best. It seems like it's been a shitty time for all of us 😔

[AydinHassan]

Tagged 3.2.0 :)