Toggle panels and no-Distraction mode #11732

Merged
merged 12 commits into from Nov 18, 2015

Projects

None yet

6 participants

@abose
Contributor
abose commented Sep 23, 2015

Full code view with all other panels/toolbars hidden.
Also minor refactoring of variable names for toggle/hide for sidebar.
shortcut : ctrl+shift+` for toggle panels
shortcut : ctrl+shift+2 for no distraction mode
Also added panel management API's to workspace manager.

@nethip

  • Add unit tests
@abose abose added this to the Release 1.6 milestone Sep 23, 2015
@abose abose added the F General UI label Sep 23, 2015
@abose abose changed the title from no-Distraction mode to Toggle panels and no-Distraction mode Oct 7, 2015
@abose
Contributor
abose commented Oct 8, 2015

Should it be hide panels or toggle panels? toggle seems to be troublesome at times.

@abose
Contributor
abose commented Oct 12, 2015
@petetnt petetnt commented on an outdated diff Oct 12, 2015
src/extensions/default/NoDistractions/main.js
+ * Updates the command checked status based on the preference name given.
+ *
+ * @param {string} name Name of preference that has changed
+ */
+ function _updateCheckedState() {
+ CommandManager.get(CMD_TOGGLE_PURE_CODE).setChecked(PreferencesManager.get(PREFS_PURE_CODE));
+ }
+
+ function _togglePureCode() {
+ PreferencesManager.set(PREFS_PURE_CODE, !PreferencesManager.get(PREFS_PURE_CODE));
+ }
+
+ /**
+ * hide all open panels
+ */
+ function _hidePanlesIfRequired() {
@petetnt
petetnt Oct 12, 2015 Member

Small typo: Panles -> `Panels"

@petetnt petetnt commented on an outdated diff Oct 12, 2015
src/extensions/default/NoDistractions/main.js
+ */
+ function _hidePanlesIfRequired() {
+ var panelIDs = WorkspaceManager.getAllPanelIDs(), i = 0;
+ _previouslyOpenPanelIDs = [];
+ for (i = 0; i < panelIDs.length; i++) {
+ if (WorkspaceManager.getPanelForID(panelIDs[i]).isVisible()) {
+ WorkspaceManager.getPanelForID(panelIDs[i]).hide();
+ _previouslyOpenPanelIDs.push(panelIDs[i]);
+ }
+ }
+ }
+
+ /**
+ * show all open panels that was previously hidden by _hidePanlesIfRequired()
+ */
+ function _showPanlesIfRequired() {
@petetnt
petetnt Oct 12, 2015 Member

Small typo: Panles -> `Panels"

@petetnt petetnt commented on an outdated diff Oct 12, 2015
src/extensions/default/NoDistractions/main.js
+ /**
+ * hide all open panels
+ */
+ function _hidePanlesIfRequired() {
+ var panelIDs = WorkspaceManager.getAllPanelIDs(), i = 0;
+ _previouslyOpenPanelIDs = [];
+ for (i = 0; i < panelIDs.length; i++) {
+ if (WorkspaceManager.getPanelForID(panelIDs[i]).isVisible()) {
+ WorkspaceManager.getPanelForID(panelIDs[i]).hide();
+ _previouslyOpenPanelIDs.push(panelIDs[i]);
+ }
+ }
+ }
+
+ /**
+ * show all open panels that was previously hidden by _hidePanlesIfRequired()
@petetnt
petetnt Oct 12, 2015 Member

Same here: Small typo: Panles -> `Panels"

@petetnt
Member
petetnt commented Oct 12, 2015

I think that hidePanels should be sufficient, at least until the panels are grouped together with something like #11586. Panels get currently opened and closed all the time so it would be tons of trouble to keep up with their states at this point IMO.

Made a few comments on the code, just some small typos.

The strings use "No distractions" and "Code view only" as the names, should those be unified to one of them?

@petetnt petetnt commented on an outdated diff Oct 12, 2015
src/extensions/default/NoDistractions/main.js
+
+ var Menus = brackets.getModule("command/Menus"),
+ CommandManager = brackets.getModule("command/CommandManager"),
+ Commands = brackets.getModule("command/Commands"),
+ Strings = brackets.getModule("strings"),
+ PreferencesManager = brackets.getModule("preferences/PreferencesManager"),
+ ViewUtils = brackets.getModule("utils/ViewUtils"),
+ KeyBindingManager = brackets.getModule("command/KeyBindingManager"),
+ WorkspaceManager = brackets.getModule("view/WorkspaceManager");
+
+ // Constants
+ var PREFS_PURE_CODE = "noDistractions",
+ CMD_TOGGLE_PURE_CODE = "view.togglePureCode",
+ CMD_TOGGLE_PANELS = "view.togglePanels";
+
+ //key biding keys
@petetnt
petetnt Oct 12, 2015 Member

binding

@petetnt petetnt commented on the diff Oct 12, 2015
src/extensions/default/NoDistractions/main.js
+ }
+
+ function _togglePanels() {
+ panelsToggled = !panelsToggled;
+ if (panelsToggled) {
+ _hidePanlesIfRequired();
+ } else {
+ _showPanlesIfRequired();
+ }
+ }
+
+ PreferencesManager.definePreference(PREFS_PURE_CODE, "boolean", false, {
+ description: Strings.DESCRIPTION_PURE_CODING_SURFACE
+ });
+
+ PreferencesManager.on("change", PREFS_PURE_CODE, function () {
@petetnt
petetnt Oct 12, 2015 Member

_showPanelsIfRequired, _hidePanelsIfRequired

@abose
Contributor
abose commented Oct 13, 2015

Thanks @petetnt for reviewing.
Made the change to toggle panel only if workspace has not changed after a hide panels command is issued. This looked intuitive to me.
The documentation for the preference was intentionally worded as Code view only. The string describes what no distractions mode in simple terms.

@ficristo ficristo commented on an outdated diff Oct 14, 2015
src/utils/Resizer.js
@@ -61,12 +62,13 @@ define(function (require, exports, module) {
// Load dependent modules
var AppInit = require("utils/AppInit"),
EventDispatcher = require("utils/EventDispatcher"),
+ viewUtils = require("utils/ViewUtils"),
@ficristo
ficristo Oct 14, 2015 Member

nit: ViewUtils, in ProperCase

@ficristo
Member

In the View menu, Toggle Panels menu doesn't show the shortcut.

@abose I briefly tested the patch and seems fine.
I agree with @petetnt for the panels.

@abose
Contributor
abose commented Oct 26, 2015

@zaggino There seems to be a bug. If you hide the git panel, the blue active icon for the git extension still remains in the main-toolbar. So added events when panels are shown or hidden so that extensions can listen to panel close events.

@zaggino
Member
zaggino commented Oct 26, 2015

thanks for the info @abose

@petetnt
Member
petetnt commented Oct 27, 2015

Tested this out again against 51c0232 , love the No distractions mode.

There's a small issue with toggle panels though (other than the shortcut not showing up/working)

  1. Open foo.js
  2. Create a validation error (I am using brackets-eslint)
  3. Click Toggle Panels -> nothing happens (expected: panel closes)
  4. Click Toggle Panels -> panel closes.

That issue that affects brackets-git affects other Extensions too, such as brackets-todo. Is there a way to (at least temporarily / during 1.6.) to throw a warning if panel was closed but the button state remained + link to say Brackets-Wiki where those events would be explained). That would provide a easy way for extension makers that use panels to know that they should update their extensions).

@ryanstewart
Member

Looks good to me. And agree, that the no distraction mode is a nice touch.

Do we have an idea of how often people will want to jump into this? Main question is wether we can set an easier shortcut.

I also like that it doesn't go full screen the way Sublime Text does, but any thought about whether we need that?

@petetnt
Member
petetnt commented Nov 10, 2015

@ryanstewart there was an issue about "true" fullscreen some while ago (#11594) and when checking out that one I saw that CEF got updated to support fullscreen quite recently so it could be implemented relatively easily (needs a brackets-shell update though) separate from this feature. 👍

@ryanstewart
Member

@petetnt ahh, awesome. And I'm definitely not advocating for that, just wanted to throw it out there. Sounds like it's been covered though!

@abose
Contributor
abose commented Nov 17, 2015

ctrl+shift+~ falls in the same vertical last line in the keyboard, so i thought it would be easily accesible to dismiss panels with one hand.

@abose
Contributor
abose commented Nov 17, 2015

@petetnt i have created #11924
This is an issue in extract extension too.

@nethip nethip commented on an outdated diff Nov 18, 2015
src/extensions/default/NoDistractions/main.js
@@ -0,0 +1,155 @@
+/*
+ * Copyright (c) 2013 Adobe Systems Incorporated. All rights reserved.
@nethip
nethip Nov 18, 2015 Contributor

Change copyright from 2013 to 2015

@nethip nethip commented on an outdated diff Nov 18, 2015
src/extensions/default/NoDistractions/main.js
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ */
+
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, regexp: true, indent: 4, maxerr: 50 */
@nethip
nethip Nov 18, 2015 Contributor

Please review if all of these params to jslint are valid for this file.

@nethip nethip commented on an outdated diff Nov 18, 2015
src/extensions/default/NoDistractions/main.js
+ var togglePureCodeKey = "Ctrl-Shift-2",
+ togglePureCodeKeyMac = "Cmd-Shift-2",
+ togglePanelsKey = "Ctrl-Shift-`",
+ togglePanelsKeyMac = "Cmd-Shift-`";
+
+ //locals
+ var _previouslyOpenPanelIDs = [],
+ panelsToggled = false,
+ layoutUpdated = false;
+
+ /**
+ * @private
+ *
+ * Updates the command checked status based on the preference name given.
+ *
+ * @param {string} name Name of preference that has changed
@nethip
nethip Nov 18, 2015 Contributor

Should we update the documentation as I don't see any {string} parameter.

@nethip nethip commented on an outdated diff Nov 18, 2015
src/extensions/default/NoDistractions/main.js
+ togglePanelsKeyMac = "Cmd-Shift-`";
+
+ //locals
+ var _previouslyOpenPanelIDs = [],
+ panelsToggled = false,
+ layoutUpdated = false;
+
+ /**
+ * @private
+ *
+ * Updates the command checked status based on the preference name given.
+ *
+ * @param {string} name Name of preference that has changed
+ */
+ function _updateCheckedState() {
+ CommandManager.get(CMD_TOGGLE_PURE_CODE).setChecked(PreferencesManager.get(PREFS_PURE_CODE));
@nethip
nethip Nov 18, 2015 Contributor

A null check would be better here.

@nethip nethip commented on an outdated diff Nov 18, 2015
src/extensions/default/NoDistractions/main.js
+ * @private
+ *
+ * Updates the command checked status based on the preference name given.
+ *
+ * @param {string} name Name of preference that has changed
+ */
+ function _updateCheckedState() {
+ CommandManager.get(CMD_TOGGLE_PURE_CODE).setChecked(PreferencesManager.get(PREFS_PURE_CODE));
+ }
+
+ function _togglePureCode() {
+ PreferencesManager.set(PREFS_PURE_CODE, !PreferencesManager.get(PREFS_PURE_CODE));
+ }
+
+ /**
+ * hide all open panels
@nethip
nethip Nov 18, 2015 Contributor

Since you are following adding "@private" to the function documentation, let us be consistent with labeling all the private functions with "@private"

@nethip nethip and 1 other commented on an outdated diff Nov 18, 2015
src/extensions/default/NoDistractions/main.js
+ function _updateCheckedState() {
+ CommandManager.get(CMD_TOGGLE_PURE_CODE).setChecked(PreferencesManager.get(PREFS_PURE_CODE));
+ }
+
+ function _togglePureCode() {
+ PreferencesManager.set(PREFS_PURE_CODE, !PreferencesManager.get(PREFS_PURE_CODE));
+ }
+
+ /**
+ * hide all open panels
+ */
+ function _hidePanelsIfRequired() {
+ var panelIDs = WorkspaceManager.getAllPanelIDs(), i = 0;
+ _previouslyOpenPanelIDs = [];
+ for (i = 0; i < panelIDs.length; i++) {
+ if (WorkspaceManager.getPanelForID(panelIDs[i]).isVisible()) {
@nethip
nethip Nov 18, 2015 Contributor

It is better to have a null check here.

@abose
abose Nov 18, 2015 Contributor

As we just got the panel ID's from the above statement, this should never be null at this line. It would be better that a NULL access exception be raised here in that case, else will be hard to debug if things fails- which it should not 🔮 .

@nethip
nethip Nov 18, 2015 Contributor

We will never know whether we are given a null panel or an actual panel as that entirely depends on the API that is getting called. A check is surely of no harm here.

@nethip nethip commented on an outdated diff Nov 18, 2015
src/extensions/default/NoDistractions/main.js
+ */
+ function _updateCheckedState() {
+ CommandManager.get(CMD_TOGGLE_PURE_CODE).setChecked(PreferencesManager.get(PREFS_PURE_CODE));
+ }
+
+ function _togglePureCode() {
+ PreferencesManager.set(PREFS_PURE_CODE, !PreferencesManager.get(PREFS_PURE_CODE));
+ }
+
+ /**
+ * hide all open panels
+ */
+ function _hidePanelsIfRequired() {
+ var panelIDs = WorkspaceManager.getAllPanelIDs(), i = 0;
+ _previouslyOpenPanelIDs = [];
+ for (i = 0; i < panelIDs.length; i++) {
@nethip
nethip Nov 18, 2015 Contributor

forEach instead of for()?

@nethip nethip commented on an outdated diff Nov 18, 2015
src/extensions/default/NoDistractions/main.js
+ function _hidePanelsIfRequired() {
+ var panelIDs = WorkspaceManager.getAllPanelIDs(), i = 0;
+ _previouslyOpenPanelIDs = [];
+ for (i = 0; i < panelIDs.length; i++) {
+ if (WorkspaceManager.getPanelForID(panelIDs[i]).isVisible()) {
+ WorkspaceManager.getPanelForID(panelIDs[i]).hide();
+ _previouslyOpenPanelIDs.push(panelIDs[i]);
+ }
+ }
+ }
+
+ /**
+ * show all open panels that was previously hidden by _hidePanelsIfRequired()
+ */
+ function _showPanelsIfRequired() {
+ var panelIDs = _previouslyOpenPanelIDs, i = 0;
@nethip
nethip Nov 18, 2015 Contributor

No need of an extra variable paneIDs here. You could further simplify this with forEach

@nethip nethip commented on an outdated diff Nov 18, 2015
src/extensions/default/NoDistractions/main.js
+ _previouslyOpenPanelIDs = [];
+ for (i = 0; i < panelIDs.length; i++) {
+ if (WorkspaceManager.getPanelForID(panelIDs[i]).isVisible()) {
+ WorkspaceManager.getPanelForID(panelIDs[i]).hide();
+ _previouslyOpenPanelIDs.push(panelIDs[i]);
+ }
+ }
+ }
+
+ /**
+ * show all open panels that was previously hidden by _hidePanelsIfRequired()
+ */
+ function _showPanelsIfRequired() {
+ var panelIDs = _previouslyOpenPanelIDs, i = 0;
+ for (i = 0; i < panelIDs.length; i++) {
+ if (WorkspaceManager.getPanelForID(panelIDs[i])) {
@nethip
nethip Nov 18, 2015 Contributor

Store the result from getPanelForID instead of calling the same function below.

@nethip nethip commented on an outdated diff Nov 18, 2015
src/nls/root/strings.js
@@ -768,5 +771,6 @@ define({
"DESCRIPTION_OPEN_PREFS_IN_SPLIT_VIEW" : "false to disable opening preferences file in split view",
"DESCRIPTION_OPEN_USER_PREFS_IN_SECOND_PANE" : "false to open user preferences file in left/top pane",
"DEFAULT_PREFERENCES_JSON_HEADER_COMMENT" : "/*\n * This is a read-only file with the preferences supported\n * by {APP_NAME}.\n * Use this file as a reference to modify your preferences\n * file \"brackets.json\" opened in the other pane.\n * For more information on how to use preferences inside\n * {APP_NAME}, refer to the web page at https://github.com/adobe/brackets/wiki/How-to-Use-Brackets#preferences\n */",
- "DEFAULT_PREFERENCES_JSON_DEFAULT" : "Default"
+ "DEFAULT_PREFERENCES_JSON_DEFAULT" : "Default",
+ "DESCRIPTION_PURE_CODING_SURFACE" : "true to enable Code view only mode, Every other UI element in brackets will be hidden"
@nethip
nethip Nov 18, 2015 Contributor

can we rephrase this to "true to enable code only mode and hide all other UI elements in {APP_NAME}"

@nethip nethip commented on the diff Nov 18, 2015
src/view/WorkspaceManager.js
@@ -191,6 +201,7 @@ define(function (require, exports, module) {
*/
Panel.prototype.hide = function () {
Resizer.hide(this.$panel[0]);
+ exports.trigger(EVENT_WORKSPACE_PANEL_HIDDEN, this.panelID);
@nethip
nethip Nov 18, 2015 Contributor

Instead of having two events EVENT_WORKSPACE_PANEL_HIDDEN and EVENT_WORKSPACE_PANEL_SHOWN, can we have just one event and pass in the param whether the panel is hidden or shown?

@abose
abose Nov 18, 2015 Contributor

I generally prefer named stateless events. That is usually much easier to deal with.

@nethip
nethip Nov 18, 2015 Contributor

I am not sure if having a new event is easier to deal with. With the param passing approach we have one event less. I don't see a compelling reason why we would want two events when one can suffice the requirement.

@abose
abose Nov 18, 2015 Contributor

In the case, say if we raise a panel visibilitychanged event, then we would need to write an intermediate function to route that event to PanelShown and panelHidden functions. That can be avoided if we can directly tag a specific trigger to an action. Maybe just a choice of writing style :)

@nethip
Contributor
nethip commented Nov 18, 2015

Couple of quick comments.

  • We need to add unit tests for the new No Distraction feature. I saw some changes in the unit tests but I am not sure if they are going to cover the new workflows
  • We need to add health logs for the new functionality. Also make sure to update our Health Data wiki with this new data
  • It is better to have null checks at places where we calling APIs, as we would never know if the API returns an actual value or null. If you want to catch errors here, use a null check along with console.warn/console.error. That should raise errors in the console, in case of failures
  • Please update the documentation, wherever required
  • Make sure we update our API docs with the changes/additions

Overall it looks good 👍 It is good to merge, provided the review comments are addressed.

@abose
Contributor
abose commented Nov 18, 2015

@nethip thanks for reviewing. Made the changes.
Will add unit tests soon.

@nethip
Contributor
nethip commented Nov 18, 2015

@abose No problem :) I took the liberty to add unit tests checkbox to your first comment. Also I created a waffle card for tracking the unit tests for this feature I am going ahead merging this.

Thanks for this feature @abose !

@nethip nethip merged commit 3cec328 into master Nov 18, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MarcelGerber MarcelGerber deleted the abose/noDistractions branch May 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment