Add a feature to toggle between panes in the core #10555 #12853

arthur801031 · 2016-10-26T01:46:13Z

I'm not sure if I need to add a menu item for this feature inside "Navigate".
I'm also not sure if I placed the code inside the correct files since I'm new to open source project contributions.

Thank you!

…issues adobe#10555

swmitra · 2016-10-26T07:48:08Z

@arthur801031 Can you please add/update some tests in MainViewManager test file?

swmitra · 2016-10-26T07:50:42Z

src/nls/root/strings.js

@@ -418,6 +418,7 @@ define({
    "CMD_SHOW_IN_EXPLORER"                : "Show in Explorer",
    "CMD_SHOW_IN_FINDER"                  : "Show in Finder",
    "CMD_SHOW_IN_OS"                      : "Show in OS",
+    "CMD_SPLITVIEW_TOGGLE"                : "Toggle Panes",


As this command is for shuffling focus across panes, can we name it accordingly?

Can we write it like this?
"CMD_SHUFFLE_FOCUS_ACROSS_PANES" : "Shuffle Focus Across Panes",

@petetnt What could be a suitable var name here ?

Something like SWITCH_PANE_FOCUS? 🚲 🏠

edit: changed from TOGGLE as it sounds like on/off

swmitra · 2016-10-26T07:54:18Z

src/view/Pane.js

@@ -209,6 +210,19 @@ define(function (require, exports, module) {
    }

    /**
+     * Toggle between panes
+     */
+    function _handleSwitchPane() {


This entire logic of shuffling focus across panes could be in MainViewManager. Pane should contain operations specific to creation, state mutation and handling other things inside a Pane. As this operation is a user requested focus preemption, it could be at a higher level above Panes.

I will try moving this function to MainViewManager. Thank you!

swmitra · 2016-10-26T07:55:24Z

src/view/MainViewManager.js

@@ -1658,8 +1658,8 @@ define(function (require, exports, module) {

        return result;
    }
-
-
+


Please revert this change as it's only unintentional whitespace modification.

swmitra · 2016-10-26T07:56:14Z

src/view/Pane.js

@@ -167,6 +167,7 @@ define(function (require, exports, module) {
        ViewUtils           = require("utils/ViewUtils"),
        ProjectManager      = require("project/ProjectManager"),
        paneTemplate        = require("text!htmlContent/pane.html");
+    var KeyBindingManager   = brackets.getModule("command/KeyBindingManager");


This var name declaration can be clubbed with the previous one.

Bumping version Number to 1.9

arthur801031 · 2016-10-27T02:02:19Z

Hi @swmitra @petetnt , I'm having trouble using the Jasmine Spec Runner for unit tests. I go to Debug -> Run Tests and the content inside the Bracket Tests window is blank. I also can't close that window. It seems like it freezes.

I was also having this problem before I started editing any files.

Here is my Brackets version:

Here is the Developer Tools' Console output:

And I'm using a 15 inch Macbook Pro and macOS Sierra Version 10.12.1. Please help. Thank you very much.

Edit: @petetnt I saw your post #11773. It has the exact same behavior as the one your described. I also tried resetting cache & preferences. I just tried deleting all the files in the brackets folder and I cloned brackets.git from my account and followed the steps in "How to Hack on Brackets" to re-install the code. I'm now getting different error messages in the console:

petetnt · 2016-10-27T07:26:26Z

@arthur801031 Did you run git submodule update --init too?

It might be related to the spec runner window running out of memory and choking too and usually just restarting until it works works too. There's an issue tracking that too somewhere IIRC.

If you have the tests at hand I can try running them too.

…by petetnt and swmitra

arthur801031 · 2016-10-27T08:11:11Z

@petetnt I tried restarting the program many times, but Jasmine Spec Runner is still blank. I just submitted the files with the unit test for this feature, but I've never written a unit test before, so I don't know if my code would work. Thank you!

arthur801031 · 2016-10-27T09:28:40Z

@petetnt @swmitra
I figured out how to get Jasmine Spec Runner to run. Here is what I did:

I used npm install chokidar --save to install chokidar since I was missing the code from chokidar.
I used npm install acorn to install acorn since I was missing this as well.
There were error messages saying that it's looking for 'acorn' in '/Users/i-chun/Documents/brackets/src/extensions/default/JavaScriptCodeHints', so I created a folder named 'node_modules' and put 'acorn' folder inside this directory.
There was also an error message saying that it's looking for 'acorn' in '/Users/i-chun/Documents/brackets/test', so I created a folder named 'node_modules' and put 'acorn' folder inside this directory.

Then, all the error messages in Jasmine Spec Runner's DevTools went away, and the Jasmine Spec Runner is now working correctly.

Note: the unit test that I created for this feature does not work. I will try to modify it. Thank you!

zaggino · 2016-10-27T09:36:32Z

That's weird @arthur801031 , the dependencies you mentioned are already in package.json file. Didn't just npm install install them for you?

arthur801031 · 2016-10-27T09:44:14Z

@zaggino I think I ran npm install before, but I'm not 100% sure. But, in my previous comment's bullet point 3, it seemed that 'HintUtils.js', 'main.js', 'Session.js' were looking for 'node_modules/acorn' in their currernt folder, since that folder was not there the Jasmine Spec Runner crash.

…wManager-test.js and the test is now passing

arthur801031 · 2016-10-27T10:07:11Z

@petetnt @swmitra

The feature and the test function for this feature are now working correctly. I'm not sure why the check has failed for this commit. Please help. Thank you!

petetnt

Made some comments about the tests. Feel free to ask extra instructions if needed 👍

petetnt · 2016-10-27T12:56:00Z

package.json

-    "version": "1.8.0-0",
-    "apiVersion": "1.8.0",
+    "version": "1.9.0-0",
+    "apiVersion": "1.9.0",


Needs rebase against master so we don't have these duplicate files, git fetch upstream master && git rebase upstream/master.

petetnt · 2016-10-27T12:57:13Z

src/view/MainViewManager.js

+    }
+
+    /**
+     * Unit test for switching from first pane to second pane


We can use SpecRunnerUtils.simulateKeyEvent instead; for examples see https://github.com/adobe/brackets/blob/f028b3ecdf2017498196f39ddbbe4a2364366c92/test/spec/InstallExtensionDialog-test.js and search for simulateKeyEvent

Hi @petetnt I'm having trouble simulating 'Alt-w' key press using SpecRunnerUtils.simulateKeyEvent. Here is my attempt:

SpecRunnerUtils.simulateKeyEvent(KeyEvent.DOM_VK_ALT, "keydown", `$('#first-pane')[0]);
SpecRunnerUtils.simulateKeyEvent(KeyEvent.DOM_VK_W, "keydown", $('#first-pane')[0]);
SpecRunnerUtils.simulateKeyEvent(KeyEvent.DOM_VK_W, "keypress", $('#first-pane')[0]);
SpecRunnerUtils.simulateKeyEvent(KeyEvent.DOM_VK_W, "keyup", $('#first-pane')[0]);

Could you please let me know what I'm doing wrong? Thank you so much!

@arthur801031 Dang, I remembered that SimulateKeyEvent has modifier keys support but it seems not: https://github.com/adobe/brackets/blob/master/test/spec/SpecRunnerUtils.js#L998

Let's modify the tests so that you use CommandManager.executeCommand to run cmd.switchPaneFocus and check that panes have switches after running that command.

Sorry for the extra trouble. 👍

@petetnt So we don't check whether or not 'Alt-W' is pressed? We only check if cmd.switchPaneFocus can actually switch pane between first pane and second pane.

@arthur801031 exactly. I created an issue for adding the modifier key option #12859

petetnt · 2016-10-27T12:57:36Z

src/view/MainViewManager.js

@@ -1729,6 +1763,9 @@ define(function (require, exports, module) {

    exports.getAllOpenFiles               = getAllOpenFiles;
    exports.focusActivePane               = focusActivePane;
+    exports.switchPaneFocus               = switchPaneFocus;
+    exports.switchPaneUnitTest1To2        = switchPaneUnitTest1To2;


Not needed, see above

petetnt · 2016-10-27T12:57:43Z

src/view/MainViewManager.js

@@ -1729,6 +1763,9 @@ define(function (require, exports, module) {

    exports.getAllOpenFiles               = getAllOpenFiles;
    exports.focusActivePane               = focusActivePane;
+    exports.switchPaneFocus               = switchPaneFocus;
+    exports.switchPaneUnitTest1To2        = switchPaneUnitTest1To2;
+    exports.switchPaneUnitTest2To1        = switchPaneUnitTest2To1;


Not needed, see above

petetnt · 2016-10-27T12:58:45Z

test/spec/MainViewManager-test.js

+                    MainViewManager.setLayoutScheme(1, 2);
+                });
+                runs(function () {
+                    MainViewManager.switchPaneUnitTest1To2();


See above for my comment on how to handle these; the tests are valid but they test if switchPaneUnitTest1To2 work, instead of testing if the keypress itself work ^^

…issues adobe#10555

…by petetnt and swmitra

…wManager-test.js and the test is now passing

arthur801031 · 2016-10-28T08:24:53Z

@petetnt I finished/uploaded the test and rebased against master branch. Please review my code again. Thank you so much! :)

petetnt · 2016-10-28T08:33:27Z

This LGTM, all tests passing. Merging this, thanks a lot @arthur801031, hopefully you'll contribute to Brackets in the future too!

swmitra · 2016-10-28T09:04:30Z

Great work @arthur801031 and @petetnt in closing this PR 👍

schroef · 2017-03-24T13:59:37Z

SOrry for asking but what button is associated with this, tried google for the icon but cant seem to find it. On mac i need to use cmd+fn+ arrow up/down, my guess its page up/down.

EDIT
found them, http://apple.stackexchange.com/questions/55727/where-can-i-find-the-unicode-symbols-for-mac-functional-keys-command-shift-e

swmitra · 2017-03-24T14:53:01Z

If I remember correctly it's Ctrl+w. @Petent Can you please confirm?

petetnt · 2017-03-24T14:55:29Z

It's Alt+W 👍

schroef · 2017-03-24T16:44:57Z

PS perhaps you should add a menu item for this. I noticed the new feature but it only tells us its possible now, doesnt say anything about the shortcut. I was actually mixing it up with switching documents, just know i understood that its for switching when 2 panes are active.

arthur801031 added 2 commits October 26, 2016 09:33

Added the feature to toggle between panes as requested by petetnt in …

08a46b3

…issues adobe#10555

Added the feature to toggle between panes as requested by petetnt in …

2f793e7

…issues adobe#10555

swmitra reviewed Oct 26, 2016

View reviewed changes

swmitra added the changes requested label Oct 26, 2016

shubhsnov and others added 2 commits October 26, 2016 15:21

Bumping version Number to 1.9

fc5a114

Merge pull request adobe#12855 from adobe/adobe/shubham/VersionUpdate

b092dea

Bumping version Number to 1.9

swmitra added this to the Release 1.9 milestone Oct 27, 2016

arthur801031 added 2 commits October 27, 2016 16:00

Edited the files and added a unit test for this feature as requested …

0b63bc7

…by petetnt and swmitra

Edited the files and added a unit test for this feature as requested …

ce4350b

…by petetnt and swmitra

arthur801031 added 2 commits October 27, 2016 16:15

Removed duplicate/unnecessary code in Pane.js

e099010

Removed additional duplicate/unnecessary code in Pane.js

77a8f4b

arthur801031 added 2 commits October 27, 2016 17:51

Edited 'should switch pane when alt-w is pressed' function in MainVie…

4289bd5

…wManager-test.js and the test is now passing

Revert package.json and src/config.json to previous commit

8211931

petetnt suggested changes Oct 27, 2016

View reviewed changes

arthur801031 added 5 commits October 28, 2016 12:47

Added the feature to toggle between panes as requested by petetnt in …

40586ae

…issues adobe#10555

Added the feature to toggle between panes as requested by petetnt in …

569698f

…issues adobe#10555

Edited the files and added a unit test for this feature as requested …

8823a85

…by petetnt and swmitra

Removed duplicate/unnecessary code in Pane.js

567668a

Removed additional duplicate/unnecessary code in Pane.js

77ee794

arthur801031 added 2 commits October 28, 2016 12:50

Edited 'should switch pane when alt-w is pressed' function in MainVie…

88b333e

…wManager-test.js and the test is now passing

Revert package.json and src/config.json to previous commit

717d13e

petetnt changed the title ~~Added a feature to toggle between panes in the core as requested by @petetnt in issues #10555~~ Added a feature to toggle between panes in the core #10555 Oct 28, 2016

petetnt changed the title ~~Added a feature to toggle between panes in the core #10555~~ Add a feature to toggle between panes in the core #10555 Oct 28, 2016

arthur801031 added 3 commits October 28, 2016 16:06

Modified test code to test CMD_SWITCH_PANE_FOCUS

0848104

Modified test code to test CMD_SWITCH_PANE_FOCUS

954f39c

Removed unnecessary code in MainViewManager-test.js

8e5d581

petetnt approved these changes Oct 28, 2016

View reviewed changes

petetnt removed the changes requested label Oct 28, 2016

petetnt merged commit e78688a into adobe:master Oct 28, 2016

petetnt mentioned this pull request Oct 28, 2016

Keyboard shortcut for switching focus between panes #10555

Closed

spiritson mentioned this pull request Jan 27, 2017

Suggestion: Add a keybind command to swap the files currently open in upper and lower pane. #13061

Open

zaggino mentioned this pull request Mar 6, 2017

command to swap visible files between panes per issue #13061 #13150

Open

haslam22 mentioned this pull request Mar 29, 2017

[1.9] Warning beep on alt+w pane swap. #13218

Open

This was referenced Aug 30, 2021

[CLOSED] Add a feature to toggle between panes in the core #10555 brackets-archive/bracketsIssues#10946

Open

command to swap visible files between panes per issue #13061 brackets-archive/bracketsIssues#13798

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a feature to toggle between panes in the core #10555 #12853

Add a feature to toggle between panes in the core #10555 #12853

arthur801031 commented Oct 26, 2016

swmitra commented Oct 26, 2016

swmitra Oct 26, 2016

arthur801031 Oct 26, 2016

swmitra Oct 26, 2016

petetnt Oct 26, 2016 •

edited

Loading

swmitra Oct 26, 2016

arthur801031 Oct 26, 2016

swmitra Oct 26, 2016

swmitra Oct 26, 2016

arthur801031 commented Oct 27, 2016 •

edited

Loading

petetnt commented Oct 27, 2016

arthur801031 commented Oct 27, 2016

arthur801031 commented Oct 27, 2016

zaggino commented Oct 27, 2016

arthur801031 commented Oct 27, 2016

arthur801031 commented Oct 27, 2016

petetnt left a comment

petetnt Oct 27, 2016

petetnt Oct 27, 2016

arthur801031 Oct 28, 2016 •

edited

Loading

petetnt Oct 28, 2016

arthur801031 Oct 28, 2016

petetnt Oct 28, 2016

petetnt Oct 27, 2016

petetnt Oct 27, 2016

petetnt Oct 27, 2016

arthur801031 commented Oct 28, 2016

petetnt commented Oct 28, 2016

swmitra commented Oct 28, 2016

schroef commented Mar 24, 2017 •

edited

Loading

swmitra commented Mar 24, 2017

petetnt commented Mar 24, 2017

schroef commented Mar 24, 2017

		@@ -1658,8 +1658,8 @@ define(function (require, exports, module) {

		return result;
		}

Add a feature to toggle between panes in the core #10555 #12853

Add a feature to toggle between panes in the core #10555 #12853

Conversation

arthur801031 commented Oct 26, 2016

swmitra commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petetnt Oct 26, 2016 • 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

arthur801031 commented Oct 27, 2016 • edited Loading

petetnt commented Oct 27, 2016

arthur801031 commented Oct 27, 2016

arthur801031 commented Oct 27, 2016

zaggino commented Oct 27, 2016

arthur801031 commented Oct 27, 2016

arthur801031 commented Oct 27, 2016

petetnt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthur801031 Oct 28, 2016 • 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

arthur801031 commented Oct 28, 2016

petetnt commented Oct 28, 2016

swmitra commented Oct 28, 2016

schroef commented Mar 24, 2017 • edited Loading

swmitra commented Mar 24, 2017

petetnt commented Mar 24, 2017

schroef commented Mar 24, 2017

petetnt Oct 26, 2016 •

edited

Loading

arthur801031 commented Oct 27, 2016 •

edited

Loading

arthur801031 Oct 28, 2016 •

edited

Loading

schroef commented Mar 24, 2017 •

edited

Loading