fix: changing Shortcuts.kts logic to use HashableKeys. #1616
Conversation
|
|
czp13
changed the title
czp13
changed the title
czp13
changed the title
czp13
changed the title
| @Suppress("UNCHECKED_CAST") | ||
| val data = MockFilterJsonObject(key, modifiers.toSet() as Set<KeyModifier>) | ||
| val data = MockFilterJsonObject(key, hashableKeys.toSet() as Set<KeyModifier>) |
There was a problem hiding this comment.
Isn't this emulating data coming from the browser? Why would that data have hashableKeys?
There was a problem hiding this comment.
Thanks.
Super TLDR:
- we still generate the filterText on the server side, which will be passed down to the client, and will be part of the event on frontend/browser when key pushed. If filter text generation is not the same/proper, then the event will be not listened (Shortcutlistener)
Longer:
It is not exactly doing this in my understanding, it is trying to generate or emaluate a filterText using flow-server private methods.
MockFilterJsonObject will go to ->
// compute the filter
filter = mgenerateEventKeyFilter.invoke(null, key).toString() + " && " +
mgenerateEventModifierFilter.invoke(null, modifiers).toString()
which will call:
mgenerateEventModifierFilter = ShortcutRegistration::class.java.getDeclaredMethod("generateEventModifierFilter", Collection::class.java)
So eventually all comes down to generateEventModifierFilter private method, which was never intended to call in the server with just key-s, those keys/keymodifiers will be always wrapped into HashableKey-s.
This is my understanding of this problem very high level:
we generate the filterTexts on the serverSide -> pass it to client -> client send back the filters when it is having a listener to it (flow client code) and matching it (matchesFilter() if I am correct), and then on the serverSide we handle the event... if the filterText generation is not the same, because the inputs are not the same then it will not work we will not match the filter...
The problem is the duplicated logics here and calling private methods, filterText generation in not considering HashableKeys.
I tested this locally :), and the failed test cases were generating events on my server side for example (shift, alt, w).
So for example this is working in a sample app with newest Flow, although the tests are failing for the same case:
Shortcuts.addShortcutListener(textField, () -> {
System.out.println("I am clicked from addShortCutListener textField alt + shift + w, MAC/Chrome/Firefox");
}, Key.KEY_W, KeyModifier.ALT, KeyModifier.SHIFT);
There was a problem hiding this comment.
I just took a quick look, but if the failure is because of the following check in Flow code
final boolean altAdded = modifiers
.contains(new HashableKey(KeyModifier.ALT));
If possible, I would change this one, instead of applying changes in testbench, as the generateEventModifierFilter method expects a collection of generic Key.
Can the above statement be rewritten in terms of Key.isModifier() and Key.matches() instead of using an equality check?
There was a problem hiding this comment.
Yes, Marco, I shared on Slack that the mentioned line changed, so my Monday/yd changes caused the difference between the testbench and flow, how they call this private method, which has triggered the test failures. I can definitely rewrite the flow part to help address the issue.
However, I would emphasize that generateEventModifierFilter() is always called with HashableKeys right now in my understandings:
(and some other code parts making sure it)
private void addKey(Key key) {
assert key != null;
HashableKey hashableKey = new HashableKey(key);
if (Key.isModifier(key)) {
if (!modifiers.contains(hashableKey)) {
modifiers.add(hashableKey);
prepareForClientResponse();
}
} else {
if (primaryKey == null || !primaryKey.equals(hashableKey)) {
primaryKey = hashableKey;
prepareForClientResponse();
}
}
}
Since generateEventModifierFilter() is a private method that is called through other methods and in a state when HashableKeys have already been added to the ShortcutRegistration object, it would make sense to call it properly with HashableKeys, just like in any other use case, it is not an open API it shall not be called with Key objects, and potentially method signature could have been changed as well to be less flexible and refer to this.
So another solution would be to make the flow side (the private method) more rigid (so use HashableKeys in the method contract, and parameters), to catch errors even before runtime, as this is a specific, exact use case for this private method.
I feel like we want to force the Liskov principle to private methods :D.
But as the consensus here as I feel is to make the methods more flexible, with less constraint, I can do that, let me make the PR for it.
Worth mentioning that by doing this we can have more unexpected behavior in runtime as we are more flexible.
But I am doing this just to share my opinion and ideas about the optimal solution.
I need a bit of time again to build the flow artifact, include it in a customer project, and manually as well trying out many combinations to make sure it is working properly (I always do manual tests too).
There was a problem hiding this comment.
I get your point.
If ShortcutRegistration only uses HashableKeys, it may be worth to change the method (and the Map) to take this type instead of the interface.
The, for this PR I would maybe move the HashableKey handling inside MockFilterJsonObject, since it is needed only there.
There was a problem hiding this comment.
I thought I replied to this, sorry.
Sure we can do this, I already did a commit containing the changes to do the HashAbleKey transformation within the MockFilterJsonObject! 🙇
czp13
force-pushed
the
fix-shortcuts-to-use-hashkeys-in-reflection
branch
3 times, most recently
from
a5edab3
to
c7c80db
Compare
| public fun getHashableKey(keyModifier: Key): Any? { | ||
| val nestedClass = Class.forName( | ||
| "com.vaadin.flow.component.ShortcutRegistration\$HashableKey" | ||
| ) | ||
| val ctor = nestedClass.declaredConstructors[0] | ||
| ctor.isAccessible = true | ||
| return ctor.newInstance(keyModifier) | ||
| } |
There was a problem hiding this comment.
Does this method need to be public? Or can we move it into the MockFilterJsonObject class or in the companion object and make it private or internal?
It seems to me it is not used elsewhere
There was a problem hiding this comment.
Yes, we can do this as well, makes sense as we only use it from that class.
Also I got these errors:
java.lang.ClassCastException: class com.vaadin.flow.component.Key$1 cannot be cast to class com.vaadin.flow.component.KeyModifier (com.vaadin.flow.component.Key$1 and com.vaadin.flow.component.KeyModifier are in unnamed module of loader 'app')
Hencewhy, I changed some casting and the class variable types. Anytime I tried to get back the Key from the Keymodifier set, I got this....
Some kind of anonymous class/Kotlin magic is happening here that Key$1 is generated, but I would spare my nerves and would not go further down the road and I just fix this with the proper typing/class names in the methods... (I just wanted initially to have this strict as well, but we do enough magic here (reflection for private method, inner class constructor, emulating filterText, etc)...
I will commit your suggestion and just leaving as it is this change:
- private class MockFilterJsonObject(val key: Key, val modifiers: Set<KeyModifier>) : JreJsonObject(JreJsonFactory()) {
+ private class MockFilterJsonObject(val key: Key, val modifiers: Set<Key>) : JreJsonObject(JreJsonFactory()) {This part was needed because of the class cast exception.
There was a problem hiding this comment.
The method replacement is Done ✅.
Please approve if you feel, have time and do not have any further requests, comments. :)
| @@ -560,8 +560,7 @@ private String getValueProviderString(int row, Grid.Column targetColumn) | |||
| ColumnPathRenderer renderer = (ColumnPathRenderer) targetColumn | |||
| .getRenderer(); | |||
|
|
|||
| Field f = ColumnPathRenderer.class | |||
| .getDeclaredField("provider"); | |||
| Field f = ColumnPathRenderer.class.getDeclaredField("provider"); | |||
There was a problem hiding this comment.
I would revert this formatting change, so that the pr only focus on the shortcut issue
There was a problem hiding this comment.
I just pushed mvn formatter:format. I can revert it.
| @@ -85,9 +81,13 @@ private class MockFilterJsonObject(val key: Key, val modifiers: Set<Key>) : JreJ | |||
| companion object { | |||
| private val mgenerateEventKeyFilter = ShortcutRegistration::class.java.getDeclaredMethod("generateEventKeyFilter", Key::class.java) | |||
| private val mgenerateEventModifierFilter = ShortcutRegistration::class.java.getDeclaredMethod("generateEventModifierFilter", Collection::class.java) | |||
| private val chashableKey = Class.forName( | |||
| "com.vaadin.flow.component.ShortcutRegistration\$HashableKey" | |||
| ).declaredConstructors[0] | |||
There was a problem hiding this comment.
very last thing is we could have this:
ShortcutRegistration::class.java
.classLoader
.loadClass("com.vaadin.flow.component.ShortcutRegistration\$HashableKey")
.declaredConstructors[0]
Not sure if it is better or not, maybe a bit more Kotlin syntax.
czp13
force-pushed
the
fix-shortcuts-to-use-hashkeys-in-reflection
branch
4 times, most recently
from
3c01309
to
b5e564d
Compare
… need to use it in our tests as well to do not have any differences within our tests and real life application. (now we used simple Key objects, but we need to use HashableKeys)
… MockFilterJsonObject class. (Based on @mcollovati review, thank you)
…we use it (suggestion by @mcollovati, thanks!).
…shableKey() method, fix imports.
… constructor), and being more strict on that we are using ShortcutRegistration class. (discussed with @mcollovati that this could be a better way)
czp13
force-pushed
the
fix-shortcuts-to-use-hashkeys-in-reflection
branch
from
7665c8a
to
3fbfd8b
Compare
…FIX pr to merge it to master)
* As the ShortcutRegistration will always use HashableKeys, hence why we need to use it in our tests as well to not have any differences between our tests and real-life application. (now we used simple Key objects, but we need to use HashableKeys) * Cleaned up the code a bit to use the HashableKey creation only in the MockFilterJsonObject class. (Based on @mcollovati review, thank you) * Put the getHashableKey() method into the class (mock JSON), so where we use it (suggestion by @mcollovati, thanks!). * One more time, even nicer this way, like this one :) (Thanks again @mcollovati) * Revert GridTester changes, and fix java doc for the newly added getHashableKey() method, fix imports. * Using a bit more Kotlinish syntax for reflection (private inner class constructor), and being more strict on that we are using ShortcutRegistration class. (discussed with @mcollovati that this could be a better way) * Enabling Shortcut related tests (were disabled by another dependency FIX pr to merge it to master) (cherry picked from commit baf3886)
* As the ShortcutRegistration will always use HashableKeys, hence why we need to use it in our tests as well to not have any differences between our tests and real-life application. (now we used simple Key objects, but we need to use HashableKeys) * Cleaned up the code a bit to use the HashableKey creation only in the MockFilterJsonObject class. (Based on @mcollovati review, thank you) * Put the getHashableKey() method into the class (mock JSON), so where we use it (suggestion by @mcollovati, thanks!). * One more time, even nicer this way, like this one :) (Thanks again @mcollovati) * Revert GridTester changes, and fix java doc for the newly added getHashableKey() method, fix imports. * Using a bit more Kotlinish syntax for reflection (private inner class constructor), and being more strict on that we are using ShortcutRegistration class. (discussed with @mcollovati that this could be a better way) * Enabling Shortcut related tests (were disabled by another dependency FIX pr to merge it to master) (cherry picked from commit baf3886) Co-authored-by: czp13 <61667986+czp13@users.noreply.github.com>
* As the ShortcutRegistration will always use HashableKeys, hence why we need to use it in our tests as well to not have any differences between our tests and real-life application. (now we used simple Key objects, but we need to use HashableKeys) * Cleaned up the code a bit to use the HashableKey creation only in the MockFilterJsonObject class. (Based on @mcollovati review, thank you) * Put the getHashableKey() method into the class (mock JSON), so where we use it (suggestion by @mcollovati, thanks!). * One more time, even nicer this way, like this one :) (Thanks again @mcollovati) * Revert GridTester changes, and fix java doc for the newly added getHashableKey() method, fix imports. * Using a bit more Kotlinish syntax for reflection (private inner class constructor), and being more strict on that we are using ShortcutRegistration class. (discussed with @mcollovati that this could be a better way) * Enabling Shortcut related tests (were disabled by another dependency FIX pr to merge it to master) (cherry picked from commit baf3886) Co-authored-by: czp13 <61667986+czp13@users.noreply.github.com>
#1620) * As the ShortcutRegistration will always use HashableKeys, hence why we need to use it in our tests as well to not have any differences between our tests and real-life application. (now we used simple Key objects, but we need to use HashableKeys) * Cleaned up the code a bit to use the HashableKey creation only in the MockFilterJsonObject class. (Based on @mcollovati review, thank you) * Put the getHashableKey() method into the class (mock JSON), so where we use it (suggestion by @mcollovati, thanks!). * One more time, even nicer this way, like this one :) (Thanks again @mcollovati) * Revert GridTester changes, and fix java doc for the newly added getHashableKey() method, fix imports. * Using a bit more Kotlinish syntax for reflection (private inner class constructor), and being more strict on that we are using ShortcutRegistration class. (discussed with @mcollovati that this could be a better way) * Enabling Shortcut related tests (were disabled by another dependency FIX pr to merge it to master) (cherry picked from commit baf3886) Co-authored-by: czp13 <61667986+czp13@users.noreply.github.com>
Description
The testbench tests failed in some cases, as some part of the filter text generation logic was changed, and now considering the browser differences (Firefox and chrome having different implementations for Alt/Option event). To tackle these browser differences I made a fix in the flow server here:
The new logic is considering now if the key is Alt or not, using contains(). But because of this change, some tests were running failed in testbench project.
In this PR:
Key-s toHashableKeyas we do in real-life scenarios always (check ShortCutRegistration 485 linef or more information).So now with this fix, when we call the private methods it will be equal to how we do in a real-life apps.
More info:
testbench/vaadin-testbench-unit-shared/src/main/kotlin/com/vaadin/testbench/unit/internal/Shortcuts.kt
Line 19 in d110378
Type of change
Checklist
Additional for
Featuretype of change