Explain available copy/cut/paste options better #666

mfainshtein4 · 2017-03-27T04:14:07Z

implemented a check to see if browser supports these commands via queryCommandSupported if not display dialog which prompts users to use the native functionality for cut, copy, paste. The functionality also includes a check to see if the system is a mac and if so display command+c ..etc and if its not a mac, display Ctrl+c..etc.

fixes #451

humphd

I've left some feedback. The entire thing needs to be fixed for indent issues: Brackets uses 4-space indents.

humphd · 2017-03-27T14:55:24Z

src/command/DefaultMenus.js

        editor_cmenu.addMenuItem(Commands.TOGGLE_QUICK_EDIT);
        editor_cmenu.addMenuItem(Commands.TOGGLE_QUICK_DOCS);
-        editor_cmenu.addMenuItem(Commands.EDIT_SELECT_ALL);
+		editor_cmenu.addMenuDivider();


Indent issues here, please match surrounding code.

humphd · 2017-03-27T14:56:19Z

src/base-config/keyboard.json

-    "edit.paste": [
-        "Ctrl-V"
-    ],
    "edit.selectAll":  [


So you're thinking to leave this one in? Probably that's OK, but let's make sure it always works in all browsers.

yup we will need to do some testing, see if it works on all operating systems

humphd · 2017-03-27T14:57:27Z

src/command/DefaultMenus.js

                        //e.pageY += 6;
-                    }
-
+


Is this change from the original Adobe commit? It seems odd to just have this comment block hanging there.

humphd · 2017-03-27T14:57:37Z

src/editor/EditorCommandHandlers.js

        Strings            = require("strings"),
        Editor              = require("editor/Editor").Editor,
        CommandManager     = require("command/CommandManager"),
+		Dialogs        	   = require("widgets/Dialogs"),


More indent issues here.

humphd · 2017-03-27T15:01:58Z

src/editor/EditorCommandHandlers.js

+		} else {
+			Dialogs.showModalDialogUsingTemplate(dialogTemplate);
+			return ignoreCommand;
+		}


This pattern is wrong, here, and also in the code below. You need to always return a function from the anonymous function being run on line 1150. Basically, you are making _execCommandCut a function, but you won't know which one of the two possible options until run time when you check on the value of document.queryCommandSupported("cut").

It should be something like this:

var _execCommandCut = (function() { if(document.queryCommandSupported("cut")) { return function() { _execCommand("cut"); }; } return function() { Dialogs.showModalDialogUsingTemplate(dialogTemplate); return ignoreCommand(); }; }());

humphd · 2017-03-27T15:02:09Z

src/editor/EditorCommandHandlers.js

+		}
+	}());
+
+	var _execCommandCopy = (function() {


Same issue here.

humphd · 2017-03-27T15:02:50Z

src/editor/EditorCommandHandlers.js

+	}());
+
+	function _execCommandPaste(){
+		Dialogs.showModalDialogUsingTemplate(dialogTemplate);


Indent issue.

Also, you need to add return ignoreCommand(); as well.

humphd · 2017-03-27T15:04:56Z

src/htmlContent/copy-paste-dialog.html

+      These actions are unavailable via the Edit menu, but you can still use:
+    </p>
+    <table class= "guide">
+    <script>


You don't want to run a script in this dialog. Instead, you'll want to follow the pattern here:

https://github.com/adobe/brackets/blob/f028b3ecdf2017498196f39ddbbe4a2364366c92/src/help/HelpCommandHandlers.js#L81

Render variables into your template with Mustache.

I made the changes to implement mustache to pass the variables in, but when I pass in the unicode for the Command key it displays the code rather than the character. Is there a word around for this or do I change it to Command+C for example?

humphd · 2017-03-27T15:05:39Z

src/htmlContent/copy-paste-dialog.html

@@ -0,0 +1,51 @@
+<div id="myModal" class="modal">


Can you put a screenshot of this in your PR so @flukeout can review the UI as well?

@flukeout it currently looks like this.

@flukeout we need a UI review from you on this change.

Pomax · 2017-03-27T23:47:52Z

src/editor/EditorCommandHandlers.js

        Editor              = require("editor/Editor").Editor,
        CommandManager     = require("command/CommandManager"),
-		Dialogs        	   = require("widgets/Dialogs"),
+	Dialogs        	   = require("widgets/Dialogs"),


this still uses a tab. Please set your code editor to use spaces for tabs in this file. If you do a text selection on any of the lines above/below you'll see that it's indentation that purely uses spaces, unlike your line here, which uses a tab

Pomax · 2017-03-28T15:58:56Z

@mfainshtein4 can you make sure to run npm test before you push up a commit? Right now your code has some linting issues that makes the automated-tester fail, which you can catch by running npm test on your own machine and making sure that your code does not fail that, before pushing it up.

humphd · 2017-03-29T18:53:20Z

Looks like a missing semicolon:

Running "eslint:src" (eslint) task
/home/travis/build/mozilla/brackets/src/editor/EditorCommandHandlers.js

  1173:10  error  Missing semicolon  semi

✖ 1 problem (1 error, 0 warnings)

@Pomax is right, just like with our lab last week, you can do your testing with eslint locally using npm test without having to actually push broken commits. It's good to get things passing locally so you don't waste cycles on travis, or spam people with github notifications for broken builds.

mfainshtein4 · 2017-03-28T03:00:45Z

src/base-config/keyboard.json

-    "edit.paste": [
-        "Ctrl-V"
-    ],
    "edit.selectAll":  [


yup we will need to do some testing, see if it works on all operating systems

mfainshtein4 · 2017-04-02T18:00:59Z

src/htmlContent/copy-paste-dialog.html

+      These actions are unavailable via the Edit menu, but you can still use:
+    </p>
+    <table class= "guide">
+    <script>


I made the changes to implement mustache to pass the variables in, but when I pass in the unicode for the Command key it displays the code rather than the character. Is there a word around for this or do I change it to Command+C for example?

mfainshtein4 · 2017-04-03T17:13:10Z

src/editor/EditorCommandHandlers.js

-    if(true){
+    if(navigator.platform.indexOf("Mac") !== -1;){
        commandKey = {
            COMMAND_KEY: "&#8984"


@humphd This was the code I mentioned in the email. Where the Unicode doesn't display the Command symbol.

ignore the semicolon I will remove that..

I see in the Mustache docs the following: "All variables are HTML-escaped by default. If you want to render unescaped HTML, use the triple mustache: {{{name}}}. You can also use & to unescape a variable."

humphd · 2017-04-03T17:29:30Z

src/htmlContent/copy-paste-dialog.html

+    <table class= "guide">
+      <tr>
+        <th>
+          {{COMMAND_KEY}}+X


Try making these {{{COMMAND_KEY}}} (3 mustaches).

humphd · 2017-04-03T17:29:58Z

src/styles/bramble_overrides.css

 }

+table.guide {
+	width: 100%;


Your indents are all too far in this file. 4-space indent please, match to surrounding code.

Pomax · 2017-04-03T23:20:15Z

src/command/DefaultMenus.js

-        editor_cmenu.addMenuItem(Commands.EDIT_COPY);
-        editor_cmenu.addMenuItem(Commands.EDIT_PASTE);
-        editor_cmenu.addMenuDivider();
+		editor_cmenu.addMenuDivider();


Please use the correct indentation, and please make sure to run npm test before committing code, because npm test will inform you of these easy to catch errors. They should not show up in a pushed commit

Pomax · 2017-04-03T23:21:09Z

src/editor/EditorCommandHandlers.js


        return result.promise();
    }
+<<<<<<< HEAD


this is a file with merge conflict code in. Please fix your commits first, and verify that your npm test and npm run build work. There is no way this file works.

Pomax

and as uncommentable issue: was there a reason to update the path-utils submodule?

Pomax · 2017-04-03T23:22:13Z

src/command/DefaultMenus.js

        // editor_cmenu.addMenuItem(Commands.NAVIGATE_JUMPTO_DEFINITION);
        editor_cmenu.addMenuItem(Commands.TOGGLE_QUICK_EDIT);
        editor_cmenu.addMenuItem(Commands.TOGGLE_QUICK_DOCS);
+		editor_cmenu.addMenuDivider();


please run npm test before you push changes, as that will tell you that your indentation here needs fixing.

when I run npm test it doesn't say anything is wrong? why could this be?

I don't think the Brackets code does much in the way of lint-format checks.

Pomax · 2017-04-03T23:22:27Z

src/editor/EditorCommandHandlers.js


+    function _execCommand(cmd) {
+        window.document.execCommand(cmd);
+	}


indentation

humphd

This is looking really close to me. Let's get the last bits of what I suggested fixed, and we need to deal with any UI reviews from @flukeout. Also, you have some changes in here that are unrelated to your patch (e.g., path-utils), and probably need to git submodule update --init to revert.

humphd · 2017-04-06T12:53:13Z

src/editor/EditorCommandHandlers.js

        _                  = require("thirdparty/lodash");
-
+
+    var commandKey = {


var commandKey; if(brackets.platform === "mac") { commandKey = ...; } else { commandKey = ...; }

humphd · 2017-04-06T12:54:07Z

src/editor/EditorCommandHandlers.js

+        };
+    }());
+
+    function _execCommandPaste(){


Add a comment above this indicating that we ignore the value of document.queryCommandSupported("paste"), because browsers don't honour it due to security concerns.

humphd · 2017-04-06T12:54:40Z

src/htmlContent/copy-paste-dialog.html

@@ -0,0 +1,51 @@
+<div id="myModal" class="modal">


@flukeout we need a UI review from you on this change.

humphd · 2017-04-06T12:55:20Z

src/styles/bramble_overrides.css

  position: relative;
 }

+.CodeMirror-foldgutter:after {


Why this change?

During a merge conflict I just appended it under my stuff. I wasn't sure what its used for. So I kept it there.

flukeout · 2017-04-06T23:50:09Z

Testing this out now.

flukeout · 2017-04-07T00:18:36Z

@mfainshtein4 how do I get this UI to show up in my local environment? I guess my browsers support the operation, but is there something I can change to preview the popup? Let me know, thanks!

flukeout · 2017-04-07T00:22:57Z

Nevermind @mfainshtein4 - I got it to show up when I use the "Paste" action. However, on my mac, it looks like the key type (Command) is missing from the instructions...

humphd · 2017-04-08T19:31:45Z

@mfainshtein4 is this ready for review again? When it is, use @ and our names to make sure we see this.

mfainshtein4 · 2017-04-08T19:43:41Z

@Pomax , @humphd , @flukeout I am ready for another review.

humphd · 2017-04-10T16:56:19Z

Code looks good. @flukeout this needs your UI review/suggestions/testing, then I think it's done.

flukeout · 2017-04-10T17:03:42Z

Checking in on the latest changes now.

flukeout · 2017-04-10T17:09:28Z

@humphd Please merge it! I have some minor suggestions but I'll make those as a separate PR.

@mfainshtein4 - thank you so much for this work! 👍

humphd · 2017-04-10T17:39:58Z

A long road, but we finally made it. I've tested locally, and this is working great. Thanks for sticking it out, @mfainshtein4, this is a good fix.

mfainshtein4 · 2017-04-10T19:57:28Z

@humphd Thanks. I appreciate the constant support and pokes in the right direction!

humphd suggested changes Mar 27, 2017

View reviewed changes

Pomax changed the title ~~Issue 451~~ Explain available copy/cut/paste options better Mar 27, 2017

Pomax suggested changes Mar 27, 2017

View reviewed changes

mfainshtein4 commented Apr 3, 2017

View reviewed changes

humphd reviewed Apr 3, 2017

View reviewed changes

Pomax suggested changes Apr 3, 2017

View reviewed changes

humphd suggested changes Apr 6, 2017

View reviewed changes

Pomax requested a review from flukeout April 6, 2017 20:43

fix mozilla#451: improve cut-copy-paste

9effbf6

mfainshtein4 force-pushed the issue-451 branch from ad4d806 to 9effbf6 Compare April 7, 2017 18:14

humphd merged commit 1dcac9a into mozilla:master Apr 10, 2017

humphd mentioned this pull request Apr 10, 2017

CSS typo fix #706

Merged

timmoy pushed a commit to timmoy/brackets that referenced this pull request Apr 19, 2017

fix mozilla#451: improve cut-copy-paste (mozilla#666)

66e1c9c

timmoy pushed a commit to timmoy/brackets that referenced this pull request Apr 20, 2017

fix mozilla#451: improve cut-copy-paste (mozilla#666)

d210b23

Rajat-dhyani pushed a commit to Rajat-dhyani/brackets that referenced this pull request Apr 20, 2017

fix mozilla#451: improve cut-copy-paste (mozilla#666)

ab6e418

Explain available copy/cut/paste options better #666

Explain available copy/cut/paste options better #666

Uh oh!

Conversation

mfainshtein4 commented Mar 27, 2017 • edited by Pomax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

humphd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pomax commented Mar 28, 2017

Uh oh!

humphd commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pomax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

humphd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfainshtein4 commented Mar 27, 2017 •

edited by Pomax

Loading

mfainshtein4 commented Apr 8, 2017 •

edited

Loading