Add some additional keybindings, improve re-opening an already open bot #1203

cwhitten · 2018-12-29T04:54:25Z

(CMD/CTRL) + W now closes the active tab if there is one, currently the binding closes the application entirely.
(CMD/CTRL) + O now prompts the user to select a .bot file to open. currently there is no behavior with this binding.
(CMD/CTRL) + N now opens the new bot configuration modal. currently there is no behavior with this binding.
Bugfixes/Improvements:

When a conversation tab is closed and opened again via an open dialog, the user receives an error about the bot already being open. This is nonsensical. To achieve this behavior successfully it requires a click on the correct Endpoint item to re-open the conversation tab. This change makes it so that the tab is automatically re-opened without the error when the user opens an already open bot with an open dialog.

coveralls · 2018-12-29T05:00:55Z

Pull Request Test Coverage Report for Build 1715

14 of 19 (73.68%) changed or added relevant lines in 3 files are covered.
568 unchanged lines in 26 files lost coverage.
Overall coverage decreased (-2.3%) to 56.782%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/app/client/src/ui/shell/mdi/tabBar/tabBar.tsx	6	7	85.71%
packages/app/client/src/ui/helpers/activeBotHelper.ts	0	4	0.0%

Files with Coverage Reduction	New Missed Lines	%
packages/app/client/src/data/action/presentationActions.ts	1	50.0%
testSetup.js	1	66.67%
packages/app/main/src/settingsData/reducers/frameworkReducer.ts	1	66.67%
packages/sdk/shared/built/ipc/channel.js	1	85.71%
packages/app/main/src/settingsData/actions/frameworkActions.ts	1	50.0%
packages/app/client/src/ipc.ts	1	41.67%
packages/sdk/shared/built/utils/bot.js	1	94.12%
packages/app/client/src/data/action/endpointServiceActions.ts	1	71.43%
packages/emulator/core/lib/types/log/util.js	2	46.67%
packages/app/client/src/data/reducer/resourcesReducer.ts	2	73.33%

Totals
Change from base Build 1703:	-2.3%
Covered Lines:	3570
Relevant Lines:	5603

💛 - Coveralls

justinwilaby · 2018-12-30T18:25:15Z

packages/app/client/src/ui/shell/mdi/tabBar/tabBar.tsx

@@ -82,6 +82,14 @@ export class TabBar extends React.Component<TabBarProps, TabBarState> {
    };
  }

+  public componentDidMount() {
+    window.addEventListener('keydown', this.keyboardListener);


Event naming should follow JS best practices. in this case onKeyDown or onWindowKeyDown is recommended.

justinwilaby · 2018-12-30T18:30:17Z

packages/app/client/src/utils/eventHandlers.ts

+  const key = event.key.toLowerCase();
+
+  if (ctrlOrCmdPressed && key ===  'o') {
+    const { Commands: { Bot: { OpenBrowse }} } = SharedConstants;


This const declaration is redundant since it is only ever used once. Consider just using SharedConstants.commands.Bot.OpenBrowse

I'm going to hoist this declaration as the following (and leave the destructuring in place)

const { Commands: { Bot: { OpenBrowse }, UI: { ShowBotCreationDialog }} } = SharedConstants;

justinwilaby · 2018-12-30T18:30:41Z

packages/app/client/src/utils/eventHandlers.ts

+  }
+
+  if (ctrlOrCmdPressed && key ===  'n') {
+    const { Commands: { UI: { ShowBotCreationDialog }} } = SharedConstants;


same here: redundant const declaration

justinwilaby · 2018-12-30T18:35:43Z

packages/app/client/src/utils/eventHandlers.spec.ts

+  }
+}));
+
+describe('#globalHandlers', () => {


Consider making this test suite read like a sentence. Instead of #globalHandlers handles CMD+O, use The globalHandlers should call OpenBrowse when CMD+O is pressed

justinwilaby · 2018-12-31T15:08:27Z

@cwhitten -

There are a few things about this pull request that gives me pause. I strongly believe this PR should not be accepted in its current state for the following reasons:

It changes the functionality of how a bot it opened - this PR eliminates the message to the user that a bot is already open and to click on an endpoint to open a new conversation. Instead, the user is presented with a dialog to switch bots (even though the bot they are switching to is already open). This has the side effect of irreversibly wiping out all previous conversations from that bot. The current implementation is intentional and was put in place to avoid this. This PR removes that intentional design and introduces a defect.
The keyboard shortcuts in this PR deviate from, or provides an incomplete solution to the accepted and approved design listed in Menu items need shortcut keys #560.

My suggestion is to revert the changes to how a bot is opened and implement the shortcuts as described in #560.

cwhitten · 2018-12-31T15:22:47Z

It changes the functionality of how a bot it opened - this PR eliminates the message to the user that a bot is already open and to click on an endpoint to open a new conversation. Instead, the user is presented with a dialog to switch bots (even though the bot they are switching to is already open). This has the side effect of irreversibly wiping out all previous conversations from that bot. The current implementation is intentional and was put in place to avoid this. This PR removes that intentional design and introduces a defect.

Ah, I didn't notice that the Switch Bots message shows up when a bot's tab is already opened, I can look into removing that message in that scenario.

When I use the Endpoint to open a bot conversation, I also lose all conversation history. You alluded to this not being the case in some scenarios. Can you explain this a bit more?

This has the side effect of irreversibly wiping out all previous conversations from that bot. The current implementation is intentional and was put in place to avoid this.

This implementation is weak, simply put. We should improve it.

cwhitten · 2018-12-31T15:52:51Z

Additionally, we should re-evaluate the accepted keybindings for Windows. I'd like to introduce the following that we can reference https://code.visualstudio.com/shortcuts/keyboard-shortcuts-windows.pdf

The intention of these few bindings in this change was considered really low-hanging and not meant to be comprehensive.

cwhitten · 2019-01-02T20:34:47Z

I've updated the diff to include a candidate change to the confirmAndSwitchBots implementation in activeBotHelper.ts, essentially removing the confirmation step and just moving on with the "switch" portion of the implementation. There are two call sites for this function in the live code, both in botCommands.ts. Can someone explain to me why we can't do something like this this?

tonyanziano

Looks good to me! Had one minor nitpick about formatting.

👍

tonyanziano · 2019-01-07T20:37:30Z

packages/app/client/src/ui/shell/mdi/tabBar/tabBar.spec.tsx

+    const closeTabSpy = jest.fn();
+
+    mount(
+      <TabBar tabOrder={ [] } documents={ {} } activeDocumentId={ '1234' } closeTab={closeTabSpy}/>


padding between curly braces:

closeTab={ closeTabSpy }

tonyanziano · 2019-01-07T21:07:11Z

packages/app/client/src/ui/helpers/activeBotHelper.ts

-      const result = await this.confirmSwitchBot();
-
-      if (result) {
-        store.dispatch(EditorActions.closeNonGlobalTabs());


On second glance, I noticed that this closeNonGlobalTabs() is never being called in the updated version of switchBots().

This means that if you previously had a livechat tab open with BotFile1, and were now switching to BotFile2 , the livechat tab corresponding to BotFile1 would not be closed, meaning that we would have a hanging livechat with a bot file that is not currently active. This doesn't really fit within the Emulator's concept of having one "active" bot file.

… botfile is open

Stale

tonyanziano

Looks good now!

justinwilaby · 2019-01-08T01:10:55Z

Looks like this PR causes a small defect. Unless this is WOD....

Repro

Open Bot 1 and start a conversation
Open Bot 1 again

Notice 2 Chat windows are now open

Expected behavior

The existing chat window should become focused and a second window should not appear.

notes

If I have a bot with multiple endpoints open and am working with an endpoint at index=1, then open the same bot, I would not expect another chat window to open with endpoint at index=0 selected. It seems more appropriate that if I already have a chat window open and try to open the same bot again, the existing chat window will move into focus.

justinwilaby

Looks like there still could be an issue here.

cwhitten · 2019-01-08T02:06:22Z

@justinwilaby I saw https://github.com/Microsoft/BotFramework-Emulator/blob/85b220825625068b79245f7df2a6ed811ba7be10/packages/app/client/src/ui/helpers/activeBotHelper.ts#L270-L280 and opted to go with selecting the first endpoint in the list of services as it seemed to be a bit of a convention.

I actually like the idea of opening multiple tabs. To me, it's still VSCode-like and relatively intuitive (though redundant in many cases).

The point of the change was not to do the ultimate and best experience possible, to be honest. It's improving a considerably poor existing experience. I'm happy to revisit down the road with some proper scope and prioritization, but something needs to land in the interim.

justinwilaby · 2019-01-08T15:39:19Z

The point of the change was not to do the ultimate and best experience possible, to be honest. It's improving a considerably poor existing experience. I'm happy to revisit down the road with some proper scope and prioritization, but something needs to land in the interim.

I can certainly relate to that!

justinwilaby

cwhitten requested review from tonyanziano, a-b-r-o-w-n and justinwilaby December 29, 2018 05:00

justinwilaby previously requested changes Dec 30, 2018

View reviewed changes

tonyanziano approved these changes Jan 7, 2019

View reviewed changes

tonyanziano suggested changes Jan 7, 2019

View reviewed changes

cwhitten added 5 commits January 7, 2019 15:58

Add tests to tabBar.spec

bae3f94

More contentional test descriptions

7ad8cc0

Temporary logic change in confirmAndSwitchBots

326499b

Convert confirmAndSwitchBots -> switchBots

78f0da4

Revert switchBots rename, gracefully opening a new live chat when its…

720c81d

… botfile is open

cwhitten force-pushed the keyboard-bindings branch from e5aed83 to 720c81d Compare January 7, 2019 23:59

tonyanziano previously approved these changes Jan 8, 2019

View reviewed changes

Merge branch 'master' into keyboard-bindings

c8f3615

justinwilaby suggested changes Jan 8, 2019

View reviewed changes

justinwilaby previously approved these changes Jan 8, 2019

View reviewed changes

cwhitten added 2 commits January 8, 2019 08:35

Merge branch 'master' into keyboard-bindings

bae2cbe

Add padding around prop value

bd05f7f

cwhitten dismissed stale reviews from justinwilaby and tonyanziano via bd05f7f January 8, 2019 18:06

justinwilaby approved these changes Jan 8, 2019

View reviewed changes

cwhitten merged commit f74cb46 into master Jan 8, 2019

cwhitten deleted the keyboard-bindings branch January 8, 2019 18:30

cwhitten mentioned this pull request Jan 8, 2019

Gracefully reopen an already open botfile #1214

Closed

justinwilaby mentioned this pull request Jan 31, 2019

Menu items need shortcut keys #560

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some additional keybindings, improve re-opening an already open bot #1203

Add some additional keybindings, improve re-opening an already open bot #1203

cwhitten commented Dec 29, 2018 •

edited

Loading

coveralls commented Dec 29, 2018 •

edited

Loading

justinwilaby Dec 30, 2018 •

edited

Loading

cwhitten Dec 30, 2018

justinwilaby Dec 30, 2018

cwhitten Dec 30, 2018 •

edited

Loading

justinwilaby Dec 30, 2018

justinwilaby Dec 30, 2018

justinwilaby commented Dec 31, 2018 •

edited

Loading

cwhitten commented Dec 31, 2018

cwhitten commented Dec 31, 2018

cwhitten commented Jan 2, 2019

tonyanziano left a comment

tonyanziano Jan 7, 2019

tonyanziano Jan 7, 2019

tonyanziano left a comment

justinwilaby commented Jan 8, 2019 •

edited

Loading

justinwilaby left a comment

cwhitten commented Jan 8, 2019 •

edited

Loading

justinwilaby commented Jan 8, 2019

justinwilaby left a comment

Add some additional keybindings, improve re-opening an already open bot #1203

Add some additional keybindings, improve re-opening an already open bot #1203

Conversation

cwhitten commented Dec 29, 2018 • edited Loading

coveralls commented Dec 29, 2018 • edited Loading

Pull Request Test Coverage Report for Build 1715

💛 - Coveralls

justinwilaby Dec 30, 2018 • edited Loading

Choose a reason for hiding this comment

cwhitten Dec 30, 2018

Choose a reason for hiding this comment

justinwilaby Dec 30, 2018

Choose a reason for hiding this comment

cwhitten Dec 30, 2018 • edited Loading

Choose a reason for hiding this comment

justinwilaby Dec 30, 2018

Choose a reason for hiding this comment

justinwilaby Dec 30, 2018

Choose a reason for hiding this comment

justinwilaby commented Dec 31, 2018 • edited Loading

cwhitten commented Dec 31, 2018

cwhitten commented Dec 31, 2018

cwhitten commented Jan 2, 2019

tonyanziano left a comment

Choose a reason for hiding this comment

tonyanziano Jan 7, 2019

Choose a reason for hiding this comment

tonyanziano Jan 7, 2019

Choose a reason for hiding this comment

tonyanziano left a comment

Choose a reason for hiding this comment

justinwilaby commented Jan 8, 2019 • edited Loading

Repro

Expected behavior

justinwilaby left a comment

Choose a reason for hiding this comment

cwhitten commented Jan 8, 2019 • edited Loading

justinwilaby commented Jan 8, 2019

justinwilaby left a comment

Choose a reason for hiding this comment

cwhitten commented Dec 29, 2018 •

edited

Loading

coveralls commented Dec 29, 2018 •

edited

Loading

justinwilaby Dec 30, 2018 •

edited

Loading

cwhitten Dec 30, 2018 •

edited

Loading

justinwilaby commented Dec 31, 2018 •

edited

Loading

justinwilaby commented Jan 8, 2019 •

edited

Loading

cwhitten commented Jan 8, 2019 •

edited

Loading