8167637: jshell tool: /edit should use EDITOR setting
authorrfield
Mon, 24 Oct 2016 17:06:10 -0700
changeset 41641 a628785b9dd9
parent 41640 0c5bdda9ac56
child 41642 86bcd45aad5b
8167637: jshell tool: /edit should use EDITOR setting 8167640: jshell tool: external editor temp file should be *.java Reviewed-by: jlahoda
langtools/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/ExternalEditor.java
langtools/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/JShellTool.java
langtools/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/resources/l10n.properties
langtools/test/jdk/jshell/ReplToolTesting.java
langtools/test/jdk/jshell/StartOptionTest.java
langtools/test/jdk/jshell/ToolCommandOptionTest.java
--- a/langtools/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/ExternalEditor.java	Mon Oct 24 14:47:48 2016 +0100
+++ b/langtools/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/ExternalEditor.java	Mon Oct 24 17:06:10 2016 -0700
@@ -80,7 +80,7 @@
     private void setupWatch(String initialText) throws IOException {
         this.watcher = FileSystems.getDefault().newWatchService();
         this.dir = Files.createTempDirectory("jshelltemp");
-        this.tmpfile = Files.createTempFile(dir, null, ".edit");
+        this.tmpfile = Files.createTempFile(dir, null, ".java");
         Files.write(tmpfile, initialText.getBytes(Charset.forName("UTF-8")));
         dir.register(watcher,
                 ENTRY_CREATE,
--- a/langtools/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/JShellTool.java	Mon Oct 24 14:47:48 2016 +0100
+++ b/langtools/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/JShellTool.java	Mon Oct 24 17:06:10 2016 -0700
@@ -133,32 +133,26 @@
     final PrintStream userout;
     final PrintStream usererr;
     final Preferences prefs;
+    final Map<String, String> envvars;
     final Locale locale;
 
     final Feedback feedback = new Feedback();
 
     /**
-     * The constructor for the tool (used by tool launch via main and by test
-     * harnesses to capture ins and outs.
-     * @param in command line input -- snippets, commands and user input
-     * @param cmdout command line output, feedback including errors
-     * @param cmderr start-up errors and debugging info
-     * @param console console control interaction
-     * @param userout code execution output  -- System.out.printf("hi")
-     * @param usererr code execution error stream  -- System.err.printf("Oops")
-     * @param prefs preferences to use
-     * @param locale locale to use
+     * Simple constructor for the tool used by main.
+     * @param in command line input
+     * @param out command line output, feedback including errors, user System.out
+     * @param err start-up errors and debugging info, user System.err
      */
-    public JShellTool(InputStream in, PrintStream cmdout, PrintStream cmderr,
-            PrintStream console,
-            PrintStream userout, PrintStream usererr,
-            Preferences prefs, Locale locale) {
-        this(in, cmdout, cmderr, console, null, userout, usererr, prefs, locale);
+    public JShellTool(InputStream in, PrintStream out, PrintStream err) {
+        this(in, out, err, out, null, out, err,
+                Preferences.userRoot().node("tool/JShell"),
+                System.getenv(),
+                Locale.getDefault());
     }
 
     /**
-     * The constructor for the tool (used by tool launch via main and by test
-     * harnesses to capture ins and outs.
+     * The complete constructor for the tool (used by test harnesses).
      * @param cmdin command line input -- snippets and commands
      * @param cmdout command line output, feedback including errors
      * @param cmderr start-up errors and debugging info
@@ -167,12 +161,13 @@
      * @param userout code execution output  -- System.out.printf("hi")
      * @param usererr code execution error stream  -- System.err.printf("Oops")
      * @param prefs preferences to use
+     * @param envvars environment variable mapping to use
      * @param locale locale to use
      */
     public JShellTool(InputStream cmdin, PrintStream cmdout, PrintStream cmderr,
             PrintStream console,
             InputStream userin, PrintStream userout, PrintStream usererr,
-            Preferences prefs, Locale locale) {
+            Preferences prefs, Map<String, String> envvars, Locale locale) {
         this.cmdin = cmdin;
         this.cmdout = cmdout;
         this.cmderr = cmderr;
@@ -186,6 +181,7 @@
         this.userout = userout;
         this.usererr = usererr;
         this.prefs = prefs;
+        this.envvars = envvars;
         this.locale = locale;
     }
 
@@ -212,6 +208,9 @@
     private String startup = null;
     private EditorSetting editor = BUILT_IN_EDITOR;
 
+    private static final String[] EDITOR_ENV_VARS = new String[] {
+        "JSHELLEDITOR", "VISUAL", "EDITOR"};
+
     // Commands and snippets which should be replayed
     private List<String> replayableHistory;
     private List<String> replayableHistoryPrevious;
@@ -486,10 +485,7 @@
      * @throws Exception
      */
     public static void main(String[] args) throws Exception {
-        new JShellTool(System.in, System.out, System.err, System.out,
-                 System.out, System.err,
-                 Preferences.userRoot().node("tool/JShell"),
-                 Locale.getDefault())
+        new JShellTool(System.in, System.out, System.err)
                 .start(args);
     }
 
@@ -513,11 +509,7 @@
             }
         }
 
-        // Read retained editor setting (if any)
-        editor = EditorSetting.fromPrefs(prefs);
-        if (editor == null) {
-            editor = BUILT_IN_EDITOR;
-        }
+        configEditor();
 
         resetState(); // Initialize
 
@@ -547,6 +539,23 @@
         }
     }
 
+    private EditorSetting configEditor() {
+        // Read retained editor setting (if any)
+        editor = EditorSetting.fromPrefs(prefs);
+        if (editor != null) {
+            return editor;
+        }
+        // Try getting editor setting from OS environment variables
+        for (String envvar : EDITOR_ENV_VARS) {
+            String v = envvars.get(envvar);
+            if (v != null) {
+                return editor = new EditorSetting(v.split("\\s+"), false);
+            }
+        }
+        // Default to the built-in editor
+        return editor = BUILT_IN_EDITOR;
+    }
+
     /**
      * Process the command line arguments.
      * Set options.
@@ -1418,6 +1427,10 @@
             }
         }
 
+        static void removePrefs(Preferences prefs) {
+            prefs.remove(EDITOR_KEY);
+        }
+
         void toPrefs(Preferences prefs) {
             prefs.put(EDITOR_KEY, (this == BUILT_IN_EDITOR)
                     ? BUILT_IN_REP
@@ -1449,11 +1462,13 @@
         private final String[] command;
         private final boolean hasCommand;
         private final boolean defaultOption;
+        private final boolean deleteOption;
         private final boolean waitOption;
         private final boolean retainOption;
+        private final int primaryOptionCount;
 
         SetEditor(ArgTokenizer at) {
-            at.allowedOptions("-default", "-wait", "-retain");
+            at.allowedOptions("-default", "-wait", "-retain", "-delete");
             String prog = at.next();
             List<String> ed = new ArrayList<>();
             while (at.val() != null) {
@@ -1464,8 +1479,10 @@
             this.command = ed.toArray(new String[ed.size()]);
             this.hasCommand = command.length > 0;
             this.defaultOption = at.hasOption("-default");
+            this.deleteOption = at.hasOption("-delete");
             this.waitOption = at.hasOption("-wait");
             this.retainOption = at.hasOption("-retain");
+            this.primaryOptionCount = (hasCommand? 1 : 0) + (defaultOption? 1 : 0) + (deleteOption? 1 : 0);
         }
 
         SetEditor() {
@@ -1476,7 +1493,7 @@
             if (!check()) {
                 return false;
             }
-            if (!hasCommand && !defaultOption && !retainOption) {
+            if (primaryOptionCount == 0 && !retainOption) {
                 // No settings or -retain, so this is a query
                 EditorSetting retained = EditorSetting.fromPrefs(prefs);
                 if (retained != null) {
@@ -1489,8 +1506,11 @@
                 }
                 return true;
             }
+            if (retainOption && deleteOption) {
+                EditorSetting.removePrefs(prefs);
+            }
             install();
-            if (retainOption) {
+            if (retainOption && !deleteOption) {
                 editor.toPrefs(prefs);
                 fluffmsg("jshell.msg.set.editor.retain", format(editor));
             }
@@ -1501,7 +1521,7 @@
             if (!checkOptionsAndRemainingInput(at)) {
                 return false;
             }
-            if (hasCommand && defaultOption) {
+            if (primaryOptionCount > 1) {
                 errormsg("jshell.err.default.option.or.program", at.whole());
                 return false;
             }
@@ -1517,6 +1537,8 @@
                 editor = new EditorSetting(command, waitOption);
             } else if (defaultOption) {
                 editor = BUILT_IN_EDITOR;
+            } else if (deleteOption) {
+                configEditor();
             } else {
                 return;
             }
--- a/langtools/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/resources/l10n.properties	Mon Oct 24 14:47:48 2016 +0100
+++ b/langtools/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/resources/l10n.properties	Mon Oct 24 17:06:10 2016 -0700
@@ -133,7 +133,7 @@
 ''/set feedback -retain <mode>'' requires that <mode> is predefined or has been retained with ''/set mode -retain'' -- {0}
 
 jshell.err.unknown.option = Unknown option: {0} -- {1}
-jshell.err.default.option.or.program = Specify -default option or program, not both -- {0}
+jshell.err.default.option.or.program = Specify -default option, -delete option, or program -- {0}
 jshell.err.option.or.filename = Specify no more than one of -default, -none, or a startup file name -- {0}
 jshell.err.unexpected.at.end = Unexpected arguments at end of command: {0} -- {1}
 jshell.err.conflicting.options = Conflicting options -- {0}
@@ -678,7 +678,9 @@
 \n\t\
 /set editor [-retain] [-wait] <command>\n\
 \n\t\
-/set editor [-retain] [-retain] -default\n\
+/set editor [-retain] -default\n\
+\n\t\
+/set editor [-retain] -delete\n\
 \n\
 Retain the current editor setting for future sessions:\n\
 \n\t\
@@ -691,7 +693,14 @@
 The <command> is an operating system dependent string.\n\
 The <command> may include space-separated arguments (such as flags)\n\n\
 If the -default option is specified, the built-in default editor will be used.\n\n\
-Otherwise an external editor should be specified in <command>. When <command>\n\
+If the -delete option is specified, previous settings are ignored -- the editor\n\
+settings are initialized as when starting the jshell tool.  Specifically, if there\n\
+is a retained setting it is used (unless both -retain and -delete are specified --\n\
+which deletes the retained setting), if one of these environment variables is set\n\
+it will be used: JSHELLEDITOR, VISUAL, or EDITOR (in that order).  Otherwise the\n\
+built-in default editor will be used.\n\n\
+If <command> is specified, it will be used as the external editor. The <command>\n\
+consists of the program and zero or more program arguments.  When <command>\n\
 is used, the temporary file to edit will be appended as the last argument.\n\
 Normally, edit mode will last until the external editor exits. Some external editors\n\
 will exit immediately (for example, if the edit window exists) either external editor\n\
--- a/langtools/test/jdk/jshell/ReplToolTesting.java	Mon Oct 24 14:47:48 2016 +0100
+++ b/langtools/test/jdk/jshell/ReplToolTesting.java	Mon Oct 24 17:06:10 2016 -0700
@@ -93,6 +93,7 @@
     private Map<String, ImportInfo> imports;
     private boolean isDefaultStartUp = true;
     private Preferences prefs;
+    private Map<String, String> envvars;
 
     public JShellTool repl = null;
 
@@ -232,6 +233,11 @@
     @BeforeMethod
     public void setUp() {
         prefs = new MemoryPreferences();
+        envvars = new HashMap<>();
+    }
+
+    protected void setEnvVar(String name, String value) {
+        envvars.put(name, value);
     }
 
     public void testRaw(Locale locale, String[] args, ReplTest... tests) {
@@ -247,9 +253,11 @@
                 new PrintStream(cmdout),
                 new PrintStream(cmderr),
                 new PrintStream(console),
+                userin,
                 new PrintStream(userout),
                 new PrintStream(usererr),
                 prefs,
+                envvars,
                 locale);
         repl.testPrompt = true;
         try {
@@ -462,7 +470,8 @@
 
     private List<String> computeCompletions(String code, boolean isSmart) {
         JShellTool js = this.repl != null ? this.repl
-                                      : new JShellTool(null, null, null, null, null, null, prefs, Locale.ROOT);
+                                      : new JShellTool(null, null, null, null, null, null, null,
+                                              prefs, envvars, Locale.ROOT);
         int cursor =  code.indexOf('|');
         code = code.replace("|", "");
         assertTrue(cursor > -1, "'|' not found: " + code);
--- a/langtools/test/jdk/jshell/StartOptionTest.java	Mon Oct 24 14:47:48 2016 +0100
+++ b/langtools/test/jdk/jshell/StartOptionTest.java	Mon Oct 24 17:06:10 2016 -0700
@@ -37,6 +37,7 @@
 import java.io.PrintStream;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Path;
+import java.util.HashMap;
 import java.util.Locale;
 import java.util.function.Consumer;
 
@@ -63,9 +64,11 @@
                 new PrintStream(cmdout),
                 new PrintStream(cmderr),
                 new PrintStream(console),
+                null,
                 new PrintStream(userout),
                 new PrintStream(usererr),
                 new ReplToolTesting.MemoryPreferences(),
+                new HashMap<>(),
                 Locale.ROOT);
     }
 
--- a/langtools/test/jdk/jshell/ToolCommandOptionTest.java	Mon Oct 24 14:47:48 2016 +0100
+++ b/langtools/test/jdk/jshell/ToolCommandOptionTest.java	Mon Oct 24 17:06:10 2016 -0700
@@ -23,7 +23,7 @@
 
  /*
  * @test
- * @bug 8157395 8157393 8157517 8158738 8167128 8163840
+ * @bug 8157395 8157393 8157517 8158738 8167128 8163840 8167637
  * @summary Tests of jshell comand options, and undoing operations
  * @modules jdk.jshell/jdk.internal.jshell.tool
  * @build ToolCommandOptionTest ReplToolTesting
@@ -124,7 +124,7 @@
                 (a) -> assertCommand(a, "/set editor -furball -mattress",
                         "|  Unknown option: -furball -mattress -- /set editor -furball -mattress"),
                 (a) -> assertCommand(a, "/set editor -default prog",
-                        "|  Specify -default option or program, not both -- /set editor -default prog"),
+                        "|  Specify -default option, -delete option, or program -- /set editor -default prog"),
                 (a) -> assertCommand(a, "/set editor prog",
                         "|  Editor set to: prog"),
                 (a) -> assertCommand(a, "/set editor prog -default",
@@ -135,6 +135,10 @@
                         "|  Editor set to: prog -furball"),
                 (a) -> assertCommand(a, "/set editor",
                         "|  /set editor prog -furball"),
+                (a) -> assertCommand(a, "/se ed -delete",
+                        "|  Editor set to: -default"),
+                (a) -> assertCommand(a, "/set editor",
+                        "|  /set editor -default"),
                 (a) -> assertCommand(a, "/set editor prog arg1 -furball arg3 -default arg4",
                         "|  Editor set to: prog arg1 -furball arg3 -default arg4"),
                 (a) -> assertCommand(a, "/set editor",
@@ -157,7 +161,7 @@
                 (a) -> assertCommand(a, "/set editor -retain -furball -mattress",
                         "|  Unknown option: -furball -mattress -- /set editor -retain -furball -mattress"),
                 (a) -> assertCommand(a, "/set editor -retain -default prog",
-                        "|  Specify -default option or program, not both -- /set editor -retain -default prog"),
+                        "|  Specify -default option, -delete option, or program -- /set editor -retain -default prog"),
                 (a) -> assertCommand(a, "/set editor -retain -wait",
                         "|  -wait applies to external editors"),
                 (a) -> assertCommand(a, "/set editor -retain -default -wait",
@@ -172,6 +176,10 @@
                 (a) -> assertCommand(a, "/set editor",
                         "|  /set editor -retain prog\n" +
                         "|  /set editor other"),
+                (a) -> assertCommand(a, "/se ed -delete",
+                        "|  Editor set to: prog"),
+                (a) -> assertCommand(a, "/set editor",
+                        "|  /set editor -retain prog"),
                 (a) -> assertCommand(a, "/set editor -retain prog -default",
                         "|  Editor set to: prog -default\n" +
                         "|  Editor setting retained: prog -default"),
@@ -198,6 +206,39 @@
         );
     }
 
+    public void setEditorEnvTest() {
+        setEnvVar("EDITOR", "best one");
+        setEditorEnvSubtest();
+        setEnvVar("EDITOR", "not this");
+        setEnvVar("VISUAL", "best one");
+        setEditorEnvSubtest();
+        setEnvVar("VISUAL", "not this");
+        setEnvVar("JSHELLEDITOR", "best one");
+        setEditorEnvSubtest();
+    }
+
+    private void setEditorEnvSubtest() {
+        test(
+                (a) -> assertCommand(a, "/set editor",
+                        "|  /set editor best one"),
+                (a) -> assertCommand(a, "/set editor prog",
+                        "|  Editor set to: prog"),
+                (a) -> assertCommand(a, "/set editor",
+                        "|  /set editor prog"),
+                (a) -> assertCommand(a, "/set editor -delete",
+                        "|  Editor set to: best one"),
+                (a) -> assertCommand(a, "/set editor -retain stored editor",
+                        "|  Editor set to: stored editor\n" +
+                        "|  Editor setting retained: stored editor")
+        );
+        test(
+                (a) -> assertCommand(a, "/set editor",
+                        "|  /set editor -retain stored editor"),
+                (a) -> assertCommand(a, "/set editor -delete -retain",
+                        "|  Editor set to: best one")
+        );
+    }
+
     public void setStartTest() {
         test(
                 (a) -> assertCommand(a, "/set start -furball",