Refactoring of KeyTipService and KeyTipAdorner

[batzen]

Currently there is quite a lot duplicated code between those two to handle keyboard input, termination etc..

Responsibilities are also a bit unclear duo to this duplication.

KeyTipAdorner should just do what an adorner should do and show something.
KeyTipService should handle keyboard input etc..

[batzen]

@floele-sp
Started refactoring in https://github.com/fluentribbon/Fluent.Ribbon/tree/KeyTipRefactoring

[floele-ww]

Your changes work quite well so far, great job :)
I found a small issue, doesn't matter a lot though:
Use Alt+K, R, R to go into the sub menu. Press left. Now when you press R again you get a key tip for "resizing and screentips" (and only there). A perfect solution would probably be to show the previous key tips when closing the menu. Alternatively, showing all screen tips of the root menu instead of just one is also a feasible solution. Even without this fix it's a huge improvement though.

[floele-ww]

One more issue: KeyTips in Backstage (Office 2013) are not displayed entirely.

They are displayed correctly if you press Alt twice in backstage to make them reappear.

[batzen]

Will try to find a solution for the navigation problem when i got some spare time.

Yeah, i am aware of that backstage issue. Planned to have a look at it. Seems to be cause by the animation/storyboard and the adorners or adornerlayer not being updated properly. Though they work for the first time opening the backstage by it's keytip.

[batzen]

Found a solution for the wrong keytip placement in backstage.
Will fix it by assigning a fixed position to the keytips. That way they always stay at the left side of the backstage + 5 units.
This also causes them to be aligned in one vertical line instead of being dependent on the control type.

Still have to figure out a solution for the navigation issue.

[batzen]

Implementing navigation the way office does would require complex focus/state tracking etc. so i decided to just terminate keytips and not restore focus.

That way i don't have to get a headache implementing it and keytips don't react weird when navigating by keyboard.

Are you ok with that way?

[floele-ww]

Sure, thanks!

[batzen]

Nice, will release the next preview then and wait if anyone finds any more issues with 4.0.

[floele-ww]

Hi again,

seems like I did find a new issue. In KeyTipService, you have the following code:

                this.currentUserInput = previousInput;
                System.Media.SystemSounds.Beep.Play();
                e.Handled = true;
                return;

The "e.Handled = true" is new here and causes a problem: My application already binds commands to Alt+(1-9) keyboard shortcuts. This interferes with the quick access shortcuts of course, but even if you don't have any, the keyboard shortcut will be swallowed here and cannot be used by my application.

Unfortunately, even removing this line doesn't fix the problem, the ribbon still blocks Alt shortcuts from being used.

[batzen]

So you just added some buttons which have access-keys?

[floele-ww]

No, I have input bindings for Alt+X shortcuts that execute commands. Like this:

target.InputBindings.Add(new KeyBinding(SwitchToTab01Command, new KeyGesture(Key.D1, ModifierKeys.Alt)));

[batzen]

Did your keybindings work before the refactoring?

Do keybindings work if you got a regular WPF app and assign access-keys to controls with the same keys as your keybindings?

[floele-ww]

Actually the issue is far more fundamental as it seems. Consider the following XAML (added to the showcase app).

<Label Target="{Binding ElementName=targetTextBox}">_Label with Access Key L</Label>
<TextBox x:Name="targetTextBox">Focus me!</TextBox>

The "L" access key simply does not work at all, but it should focus the textbox. I was so focused at checking the KeyTip functionality that I lost sight of testing the basic stuff, sorry. This needs to get sorted out though.

[batzen]

Will have a look at the issues.

[batzen]

I think i got a solution and i would like to hear what you think about it.

var shownImmediately = false;

// Should we show the keytips and immediately react to key?
if (this.activeAdornerChain == null
    || this.activeAdornerChain.IsAdornerChainAlive == false
    || this.activeAdornerChain.AreAnyKeyTipsVisible == false)
{
    this.ShowImmediatly();
    shownImmediately = true;
}

if (this.activeAdornerChain == null)
{
    return;
}

var previousInput = this.currentUserInput;
var key = keyConverter.ConvertToString(actualKey);
this.currentUserInput += key;

// If no key tips match the current input, continue with the previously entered and still correct keys.
if (this.activeAdornerChain.ActiveKeyTipAdorner.ContainsKeyTipStartingWith(this.currentUserInput) == false)
{
    if (shownImmediately)
    {
        this.activeAdornerChain?.Terminate();
        return;
    }

    this.currentUserInput = previousInput;
    System.Media.SystemSounds.Beep.Play();
    e.Handled = true;
    return;
}

So i would check if keytips should be shown immediately, because ALT was pressed and a second key was pressed.
If there are no keytips matching this key, i just terminate the just shown adorner and leave the key unhandled.
This should work for access-keys and inputbindings.
What do you think?

[floele-ww]

I tried the code in my application and it solves the access key problem for the most part. I don't see the reason why you introduced an additional "key" variable though, doesn't seem to be used.

The interesting question that remains is what should happen if there is an access key in the application "body" and the ribbon with the same char. I checked the various sidebars in MS Word and Outlook, but it looks like they all have no access key at all so there will never be a conflict. So I think that prioritising the ribbon over the rest of the application is fine and the developer has to take care of not creating duplicate access keys.

I will deploy your code changes now and check if any issues occur in our production environment.

[batzen]

The unused variable is a left over from my experiments.

Yeah, everyone has to take care not setting duplicate shortcuts.
If you do the same just with access-keys and input bindings you also have to be careful.
Input bindings should win in that case and block the access-keys, as far as i remember.

Please let me know about your test results. Will push my changes if you don't find any new issues.

[floele-ww]

Input bindings should win in that case and block the access-keys, as far as i remember.

That's not the case though. The ribbon autogenerates the access keys for the QAT (1-9), which prevents the input bindings Alt+1..9 from working inside my application. I modified my version of Fluent now so that the QAT access keys always start with 0 (01, 02, 03), that also fixes my problem. I believe the Fluent behaviour is correct but in my app the input bindings are more important than the QAT items.

[batzen]

I know that it's not the case for the ribbon.
It should be the case without a ribbon.
One of them has to win. ;-)

So you are using a modified version, and not the nuget package, of the ribbon because the auto generated QAT keytips collide with your bindings? If that's the case i could implement an extension point so you can provide a string format, or something like that.

[floele-ww]

of the ribbon because the auto generated QAT keytips collide with your bindings?

No, rather because of customisations like the search feature. But indeed, it would be great to have a way for some influence on how the keytips are generated. Maybe an event or whatever that gets the index of the QAT item as input and the access key to use as output ("none" also being an option).

[batzen]

Search feature is in the backlog. Just have to get 4.0 out of the door to start with it.

Could you create a new issue for the QAT keytip customization?

How's your test going?

[floele-ww]

Search feature is in the backlog. Just have to get 4.0 out of the door to start with it.

Sure, no pressure. It's in fact not the only modification we have to do. Another is required for being able to save and restore the QAT items. The ribbon doesn't provide any access to these at the moment, so that's another reason for not using the "official" version.

How's your test going?

It's in use and so far no issues have occured.

It may be that there still is a bug left though. We had cases where the adorner key tips didn't disappear and you could see multiple layers at the same time. So far we couldn't reproduce it though so I can't file a report for that (yet).

[batzen]

I always thought about providing a way to modify the way QAT items are saved and loaded.
Maybe create an issue for that one too?
If you decide to create one, maybe you could provide your current solution too, in case it's not too specific to your needs.

There should be no need to use a modified version of the ribbon. ;-)

It's in use and so far no issues have occured.

Nice to hear that.

[floele-ww]

Alright, will do. I always thought that I must be missing something so I didn't open a case for that yet.

[floele-ww]

OK one more issue: We also need to handle ö,ä,ü in regard to the "isKeyRealInput" variable. While this is comparatively simple, what about other non-german keyboard layouts? Simply consider all Oem1-Oem8 keys?

[batzen]

Even the KeyConverter returns Oem1 etc. for ä,ö,ü...
Will try to find a way to get the real text input.

[batzen]

@floele-sp Could you have a look and test the new changes?

[floele-ww]

Looking good so far, "ö" works fine for me. Thanks!

[batzen]

As you don't seem to have found any new issues i will close this issue. Please create a new issue in case you find any new issues.