jsakamoto / Toolbelt.Blazor.HotKeys2

This is a class library that provides configuration-centric keyboard shortcuts for your Blazor apps.
https://jsakamoto.github.io/Toolbelt.Blazor.HotKeys2/
Mozilla Public License 2.0
87 stars 6 forks source link

Ability to enable/disable hot keys from C# during runtime (WASM only) #19

Closed uhfath closed 2 months ago

uhfath commented 3 months ago

This is a slightly different version than #15. The code is more cleaner and easier to use. But it has a drawback - WASM only. The problem is this line. JS code used inside document.addEventListener can't be truly async since event itself will be already finished by the time async method returns and so there is no way to preventDefault after it. That is the only reason why this PR is WASM only, since this mode allows sync code to be called from JS to .NET and Server mode doesn't.

So, @jsakamoto if you could review the code and perhaps come out with an idea how to overcome this limitation it would be great. Currently I haven't found a way. Tried numerous hacks to preventDefault first and then re-dispatch the event but that doesn't work with real input - browser protection.

This is a description from previous PR: This PR adds and ability to control each hot key's state during runtime from C# code. Currently I've added a "disabled" state but a user can use this to add it's own custom state. Something that might look like this:

public class HotKeyState : HotKeyEntryState
{
    public bool IsHiddenFromCheatSheet { get; set; }
    public string CheatSheetCategory { get; set; }
}

Then when rendering a cheat sheet it becomes possible to customize it in any way it is required. In this case - show/hide entries or group them by categories.

Also an IsDisabled state is introduced which can be changed during runtime from C# code. When a property is changed it's appropriate JS code is updated to accommodate for it's new value. Useful when one has to control specific keys state during modal dialogs. For instance, when a CRUD dialog is opened there is no need for hot keys of it's parent page to be active. This can be statically controlled using ExcludeSelector but sometimes it is desired to have more granular dynamic control from C# without mangling HTML selectors.

Appropriate tests are also inplace. And BTW, the SampleSite.Components project is also fixed to use an appropriate ProjectReference to Toolbelt.Blazor.HotKeys2 during normal builds or tests and not Nuget's version which might be not available at test time.

PR was built against v.4.0.0 tag.

jsakamoto commented 2 months ago

@uhfath Thank you for your contributions! I'll investigate this pull request later.

jsakamoto commented 2 months ago

@uhfath

So, @jsakamoto if you could review the code and perhaps come out with an idea how to overcome this limitation it would be great.

I came up with an idea that inverted the handling of the state.

That means instead of asking the state from JavaScript to Blazor every time, I made it notify the state from Blazor to JavaScript whenever the state has changed. Please see the following commit.

https://github.com/jsakamoto/Toolbelt.Blazor.HotKeys2/commit/60d265c1fd55577da5591c665e824409300b814c

Does that make sense? If you think so, I'll merge this feature branch to the main branch and publish it as a new NuGet package version.

jsakamoto commented 2 months ago

@uhfath One more thing. What do you think about the idea that changing the property name IsDisabled to just Disabled ? I prefer Disabled rather than IsDisabled.

uhfath commented 2 months ago

@jsakamoto

I came up with an idea that inverted the handling of the state.

That is how the first version of this PR was made. But there are cases where this is inconvenient.

For instance, imagine you need to disable a hot key based on multiple factors, like properties of other classes. When state is being polled from JS it would look like this:

HotkeyState.IsDisabled => someClass.IsDialogVisible && (someOtherClass.IsPanelActive || anotherClass.GetEnabled());

When JS is being notified instead we are forced to track these changes externally which is not always possible. Another workaround could be overriding OnParametersSet and setting HotKeyState.IsDisabled. But as noted that's not always the case.

jsakamoto commented 2 months ago

@uhfath Oh, now I understand the scenario you considered. Thank you so much for explaining that.

But anyway, as long as the interoperability from the JavaScript side to the Blazor side is limited to async operations, the "every time ask the latest state from JS to Blazor on-demand" architecture won't work on Blazor Server apps. And I won't accept to drop the Blazor Server support for this library.

Therefore, we must stick to the "push the latest state from Blazor to JS" architecture. I think there are no other options.

Fortunately, the "Blazor to JS" architecture will be able to cover the scenario you mentioned without much hustle. For instance, if I have to cover the scenario you mentioned, I will implement a Blazor app like the one below.

@inject HotKeys HotKeys
...
@code {

    private HotKeysContext? HotKeysContext;

    private readonly HotKeyEntryState HotKeyEntryState = new();

    protected override void OnInitialized()
    {
        this.HotKeysContext = this.HotKeys.CreateContext()
            .Add(..., new() { State = HotKeyEntryState });
    }

    protected override void OnAfterRender(bool firstRender)
    {
        // Update the state of the hotkey entry every time the component is rendered.
        // Because the causing of rendering means that some of the states of the components have been changed.
        this.HotKeyEntryState.IsDisabled = someClass.IsDialogVisible && (someOtherClass.IsPanelActive || anotherClass.GetEnabled());
    }
}

In that implementation, we don't have to define a new class derived from the "HotKeyEntryState" class. I feel like the above implementation is simpler than defining a new derived class.

I believe the code above will work as expected. What do you think about the idea above?

uhfath commented 2 months ago

@jsakamoto, that is a good idea! Seems to be a reasonable workaround. Should work for most cases. Perhaps it's also worth to mention this method in readme too. Thanks for taking the time.

What do you think about the idea that changing the property name IsDisabled to just Disabled ? I prefer Disabled rather than IsDisabled

Agreed. This was just my quirk from our codebase.

jsakamoto commented 2 months ago

@uhfath Thanks for the reply.

it's also worth to mention this method in readme too.

Yes, I agree with you.

So, I'll do things, such as changing the property name from IsDisabled to Disabled, updating the README doc to mention how to effectively use the Disabled property, merging the feature branch to the main branch, and publishing it as a new NuGet package.

jsakamoto commented 2 months ago

@uhfath I published the new version of the "Blazor HotKeys 2".

Thank you for your contributions!

uhfath commented 2 months ago

@jsakamoto thank you for considering the changes! I'm sure it will be useful for others as well.