Adding support for hotkeys in Emulator (#304) #308

rinormaloku · 2017-10-15T20:57:05Z

Support added for following hotkeys:

F5 / Ctrl+R - Refresh the conversation.
F10 / Alt+F - Open the menu.
F6 / Ctrl+L - Focus the address bar.

The same applies for Mac using ⌘ instead of Ctrl.

msftclas · 2017-10-15T20:57:14Z

All CLA requirements met.

eanders-ms · 2017-10-16T22:51:23Z

Thank you for this, but I am sorry to say that it needs to be done another way. These hotkeys should be implemented as accelerators on menu items. It can be done using Electron's accelerator support. No need for mousetrap. Search the code for 'show-about'. It is an accelerator to show the About Dialog. Follow its implementation. Let me know if you need further clarification. Also, let me know if you are out of time to work on this :)

rinormaloku · 2017-10-17T12:57:11Z

@eanders-ms I updated the commit by using the pattern that 'show-about' uses.
Suggestions are very welcome (New to React and Redux)

eanders-ms · 2017-10-20T16:22:37Z

src/client/addressBar/addressBarTextBox.tsx

@@ -95,6 +95,10 @@ export class AddressBarTextBox extends React.Component<{}, {}> {

    componentWillMount() {
        this.settingsUnsubscribe = addSettingsListener((settings) => {
+            if (settings.hotkey.focusAddressBar) {
+                settings.hotkey.focusAddressBar = false;


settings is immutable. It can only be set in a reducer. A total pain, I know. Let me think about how to do this another way.

We can set the other values to false e.g. when triggering openMenu: Object.assign({}, state, { openMenu: true, focusAddressBar: false, refreshConversation: false });

Though the current approach was working.

It's a guiding principle of Redux that the state object is immutable. Redux works by returning a new state object each time a change is affected in a reducer. To change the state locally creates an inconsistency between the locally changed state and the actual application state. The flag you're resetting isn't getting reset in the authoritative version of the state.

In fact, you can see it happen:

in src/server/main.ts, comment out the last line: require('electron-debug')();, so that you can test the F5 shortcut without reloading the app, then recompile and run the emulator.

connect to a bot

press F5 to refresh the conversation

say something in the conversation

notice the conversation refreshes again!

I think it can be fixed by clearing the flag via another reducer action, like HotKey_FocusAddressBar_Clear.

Thank you very much for taking the time to share the knowledge. :)
Implemented your suggestion in the latest commit

It's getting closer! I noticed two bugs:

Menu Position vs. Zoom Level: When the zoom level is not the default level, the menu doesn't appear in the correct location when opened using the hotkey. Could you position it so that the top-left corner is in the center of the hamburger icon?

Double Conversation Refresh: After I send a bot a message, pressing F5 once doesn't refresh the conversation. If pressed a second time, it will refresh. After the refresh, if I say something to the bot, my bot will reply but then the conversation will unexpectedly refresh again.

Getting into more fine-grained feedback on the behavior:

Some hotkeys should toggle the state, e.g.: When the address bar is focused, pressing F6 should unfocus it. This means some hotkeys are "tri-state", having Unset, Focus, Blur states. I notice the menu hotkey already toggles, but am unsure why. It might be the behavior of Menu.popup to hide the menu if it is already showing. The Menu object does have a closePopup method.

Some tips for debugging client code

Shift+Ctrl+I will open up the Chrome debugger, which can be very useful to set breakpoints and stepping through client code, to see when the render calls happen, that the state looks like in reducers, etc.

If you have to comment out the inclusion of electron-debug, then you can uncomment the call to mainWindow.webContents.openDevTools(); that appears earlier in main.ts. This will just always open the Chrome debugger.

Thank you again for taking this on!

Er, I guess you wouldn't need a tri-state field actually, just toggle existing focus state. Sorry for the bad advice :)

@eanders-ms I was checking to create a solution for Menu Position vs Zoom Level but when zoomed in/out the electron API reports the wrong ratio. It is a known issue.
Possible Solution: different ratio per zoom level?
(Simulating click on the menu won't work as by default the menu appears wherever the mouse cursor is.)

@eanders-ms

Latest commit solves all issues:

Toggling: Affected other components visibility as well so I moved changes up to the parent component AddressBar.

Double Conversation Refresh: Clicking the refresh button the method newConversation is called. I updated hotkeys to call the newConversation method directly.

Menu Position: This solution doesn't make me happy. But couldn't come up or find any better solutions.

This is looking good! Agreed the menu positioning is pretty rough with the fixed zoom ratios. I might rework the zoom system later to pull all the logic into some kind of zoom handler.

A few minor comments in the code, then we can merge it.

Thanks for all the help along the way :)

eanders-ms · 2017-10-20T16:36:00Z

src/server/main.ts

@@ -201,6 +201,19 @@ const createMainWindow = () => {
        windowManager.zoomTo(0);
    });

+    let registerHotkeys = (hotkeys, callback) => hotkeys.forEach(hotkey =>
+        Electron.globalShortcut.register(hotkey, callback));


I recently became aware of this Electron issue, where globally registered shortcuts trigger regardless of whether the app is in focus. I think we need to avoid that, otherwise, when you refreshed a browser window, it would also refresh your bot conversation in the background. A viable workaround would be to take a dependency on 'electron-localshortcut', which circumvents this issue. We'll have to do this for the Zoom shortcuts as well (we'll take care of that in a separate PR. @mgbennet for visibility)

Implemented electron-localshortcut. And tested it. Works like a charm.

eanders-ms · 2017-10-30T16:26:34Z

src/client/addressBar/addressBarTextBox.tsx

+    onBlur() {
+        this.hasFocus = false;
+        AddressBarActions.loseFocus();
+        //const settings = getSettings();


If this is dead code, please remove.

eanders-ms · 2017-10-30T16:27:16Z

src/client/addressBar/addressBarMenu.tsx

+    }
+
+    getZoomRatio(zoomLevel: number) {
+        let zoomInRatios = [1, 1.2, 1.437, 1.73, 2.07, 2.49, 2.99, 3.58, 4.3];


Please describe how you calculated these values, or where they came from.

Added comment to it. The zoom in ratios are found very scientifically.
Binary Search and trial&error :P . Tested it in two resolutions 1080p and 1600x900.

eanders-ms · 2017-10-30T22:45:46Z

src/client/addressBar/addressBarMenu.tsx

+    getZoomRatio(zoomLevel: number) {
+        // Below zoom in and out ratios are found by trial and error.
+        // They fix the wrong coordinates reported on different zoom levels by Element.getBoundingClientRect().
+        let zoomInRatios = [1, 1.2, 1.437, 1.73, 2.07, 2.49, 2.99, 3.58, 4.3];


OK, this was really bugging me so I came up with a mathematical approximation based on your data set:

getZoomRatio(zoomLevel: number): number { return 0.4819 * Math.exp(0.1824 * (zoomLevel + 4)); }

Could you please make this change? One thing left to do is to make a constant for the 4 literal, but I can do that separately after your changes are in (since I'll want to thread it into the zoom in/out code as well).

* F5 / Ctrl+R - Refresh the conversation. * F10 / Alt+F - Open the menu. * F6 / Ctrl+L - Focus the address bar. The same apply for Mac using ⌘ instead of Ctrl.

eanders-ms · 2017-10-31T15:33:57Z

src/client/addressBar/addressBarMenu.tsx

+    }
+
+    getZoomRatio(zoomLevel: number): number {
+        return 0.4819 * Math.exp(0.1824 * (zoomLevel + 4));


When this is merged in I will comment it :)

That was a piece of math 👍
Btw. I wanted to make the 4 a constant but left it better to you as I was coming up with this name REBALANCE_NEGATIVE_ZOOM_LEVEL :p

eanders-ms · 2017-10-31T15:35:05Z

@rinormaloku thank you for your contribution! It's been a pleasure working with you.

eanders-ms reviewed Oct 20, 2017

View reviewed changes

eanders-ms reviewed Oct 30, 2017

View reviewed changes

Adding support for hotkeys in Emulator (#304)

ce389f1

* F5 / Ctrl+R - Refresh the conversation. * F10 / Alt+F - Open the menu. * F6 / Ctrl+L - Focus the address bar. The same apply for Mac using ⌘ instead of Ctrl.

eanders-ms reviewed Oct 31, 2017

View reviewed changes

eanders-ms approved these changes Oct 31, 2017

View reviewed changes

eanders-ms merged commit bed3dff into microsoft:master Oct 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for hotkeys in Emulator (#304) #308

Adding support for hotkeys in Emulator (#304) #308

rinormaloku commented Oct 15, 2017

msftclas commented Oct 15, 2017 •

edited

Loading

eanders-ms commented Oct 16, 2017

rinormaloku commented Oct 17, 2017

eanders-ms Oct 20, 2017

rinormaloku Oct 20, 2017 •

edited

Loading

eanders-ms Oct 20, 2017

rinormaloku Oct 20, 2017 •

edited

Loading

eanders-ms Oct 20, 2017

eanders-ms Oct 23, 2017

rinormaloku Oct 23, 2017 •

edited

Loading

rinormaloku Oct 28, 2017 •

edited

Loading

eanders-ms Oct 30, 2017

rinormaloku Oct 30, 2017

eanders-ms Oct 20, 2017

rinormaloku Oct 20, 2017

eanders-ms Oct 30, 2017

rinormaloku Oct 30, 2017

eanders-ms Oct 30, 2017

rinormaloku Oct 30, 2017

eanders-ms Oct 30, 2017

eanders-ms Oct 31, 2017

rinormaloku Oct 31, 2017

eanders-ms commented Oct 31, 2017

Adding support for hotkeys in Emulator (#304) #308

Adding support for hotkeys in Emulator (#304) #308

Conversation

rinormaloku commented Oct 15, 2017

msftclas commented Oct 15, 2017 • edited Loading

eanders-ms commented Oct 16, 2017

rinormaloku commented Oct 17, 2017

Choose a reason for hiding this comment

rinormaloku Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rinormaloku Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Some tips for debugging client code

Choose a reason for hiding this comment

rinormaloku Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

rinormaloku Oct 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eanders-ms commented Oct 31, 2017

msftclas commented Oct 15, 2017 •

edited

Loading

rinormaloku Oct 20, 2017 •

edited

Loading

rinormaloku Oct 20, 2017 •

edited

Loading

rinormaloku Oct 23, 2017 •

edited

Loading

rinormaloku Oct 28, 2017 •

edited

Loading