Copilot commented on code in PR #2521:
URL: https://github.com/apache/groovy/pull/2521#discussion_r3213166174


##########
subprojects/groovy-groovysh/src/main/groovy/org/apache/groovy/groovysh/jline/GroovyCommands.groovy:
##########
@@ -214,6 +236,200 @@ class GroovyCommands extends JlineCommandRegistry 
implements CommandRegistry {
         return null
     }
 
+    /**
+     * Displays an image inline using JLine's terminal-graphics support
+     * (Sixel, Kitty, iTerm2). Falls back to a summary line when the
+     * terminal doesn't speak any supported protocol; the {@code --gui}
+     * flag opens a Swing window instead.
+     *
+     * @param input parsed command input
+     * @return always {@code null}
+     */
+    def img(CommandInput input) {
+        checkArgCount(input, [0, 1, 2, 3, 4, 5])
+        if (maybePrintHelp(input, '/img')) return
+        try {
+            Integer width = null
+            Integer height = null
+            boolean preserveAspect = true
+            boolean gui = false
+            Object positional = null
+            String positionalLabel = null
+            for (int i = 0; i < input.args().length; i++) {
+                String a = input.args()[i]
+                if (a == null) {
+                    // JLine puts null into args() when a $var reference 
resolves
+                    // to null — usually because the variable isn't defined 
yet.
+                    throw new IllegalArgumentException(
+                            '/img: variable reference resolved to null ' +
+                                    '(undefined or not yet assigned) — define 
it first, e.g. ' +
+                                    "'panel = 
ScatterPlot.of(...).canvas().panel()'")
+                }
+                if (a == '-w' || a == '--width') {
+                    width = Integer.parseInt(input.args()[++i])
+                } else if (a.startsWith('--width=')) {
+                    width = Integer.parseInt(a.substring('--width='.length()))

Review Comment:
   The `-w/--width` branch unconditionally consumes the next token via 
`input.args()[++i]`. If the user passes `--width` without a value (or as the 
last argument), this will throw an 
`ArrayIndexOutOfBoundsException`/`NullPointerException` and end up as a less 
helpful saved exception. Please add an explicit bounds/value check and throw a 
targeted `IllegalArgumentException` explaining the missing width value.
   



##########
subprojects/groovy-groovysh/src/main/groovy/org/apache/groovy/groovysh/jline/GroovyCommands.groovy:
##########
@@ -214,6 +236,200 @@ class GroovyCommands extends JlineCommandRegistry 
implements CommandRegistry {
         return null
     }
 
+    /**
+     * Displays an image inline using JLine's terminal-graphics support
+     * (Sixel, Kitty, iTerm2). Falls back to a summary line when the
+     * terminal doesn't speak any supported protocol; the {@code --gui}
+     * flag opens a Swing window instead.
+     *
+     * @param input parsed command input
+     * @return always {@code null}
+     */
+    def img(CommandInput input) {
+        checkArgCount(input, [0, 1, 2, 3, 4, 5])

Review Comment:
   `/img` currently restricts the argument count to 0..5 via `checkArgCount`, 
but valid invocations can exceed this when using the space-separated forms of 
`--width`/`--height` together with flags (e.g. `--width 64 --height 32 --gui 
$img` => 6+ args). Consider removing the fixed arg-count check for this 
command, or increasing it and/or validating based on parsed options + exactly 
one positional argument instead of raw token count.
   



##########
subprojects/groovy-groovysh/src/main/groovy/org/apache/groovy/groovysh/jline/GroovyCommands.groovy:
##########
@@ -214,6 +236,200 @@ class GroovyCommands extends JlineCommandRegistry 
implements CommandRegistry {
         return null
     }
 
+    /**
+     * Displays an image inline using JLine's terminal-graphics support
+     * (Sixel, Kitty, iTerm2). Falls back to a summary line when the
+     * terminal doesn't speak any supported protocol; the {@code --gui}
+     * flag opens a Swing window instead.
+     *
+     * @param input parsed command input
+     * @return always {@code null}
+     */
+    def img(CommandInput input) {
+        checkArgCount(input, [0, 1, 2, 3, 4, 5])
+        if (maybePrintHelp(input, '/img')) return
+        try {
+            Integer width = null
+            Integer height = null
+            boolean preserveAspect = true
+            boolean gui = false
+            Object positional = null
+            String positionalLabel = null
+            for (int i = 0; i < input.args().length; i++) {
+                String a = input.args()[i]
+                if (a == null) {
+                    // JLine puts null into args() when a $var reference 
resolves
+                    // to null — usually because the variable isn't defined 
yet.
+                    throw new IllegalArgumentException(
+                            '/img: variable reference resolved to null ' +
+                                    '(undefined or not yet assigned) — define 
it first, e.g. ' +
+                                    "'panel = 
ScatterPlot.of(...).canvas().panel()'")
+                }
+                if (a == '-w' || a == '--width') {
+                    width = Integer.parseInt(input.args()[++i])
+                } else if (a.startsWith('--width=')) {
+                    width = Integer.parseInt(a.substring('--width='.length()))
+                } else if (a == '--height') {
+                    height = Integer.parseInt(input.args()[++i])
+                } else if (a.startsWith('--height=')) {
+                    height = 
Integer.parseInt(a.substring('--height='.length()))

Review Comment:
   The `--height` branch unconditionally consumes the next token via 
`input.args()[++i]`. If the user passes `--height` without a value (or as the 
last argument), this will throw and surface as an internal exception. Please 
add an explicit bounds/value check and throw a clear `IllegalArgumentException` 
for the missing height value.
   



##########
subprojects/groovy-groovysh/src/test/groovy/org/apache/groovy/groovysh/commands/ImgTest.groovy:
##########
@@ -0,0 +1,151 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ */
+package org.apache.groovy.groovysh.commands
+
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.io.TempDir
+
+import javax.imageio.ImageIO
+import javax.swing.JLabel
+import java.awt.image.BufferedImage
+import java.nio.file.Files

Review Comment:
   Unused import: `java.nio.file.Files` is imported but not referenced anywhere 
in this test file.
   



##########
subprojects/groovy-groovysh/src/spec/doc/groovysh.adoc:
##########
@@ -720,6 +720,75 @@ groovy> /imports
 import java.util.concurrent.BlockingQueue
 --------------
 
+[[GroovyShell-img]]
+==== `/img`
+
+The `/img` command displays an image inline using JLine's terminal-graphics
+support (Sixel, Kitty, or iTerm2 protocols, auto-detected). The argument is
+either a local file path, an `http(s)://` URL, or a Groovy variable
+reference using the standard `$` syntax.
+
+[source,jshell]
+--------------
+groovy> /img chart.png
+groovy> /img --width=80 https://example.com/diagram.png
+groovy> img = new java.awt.image.BufferedImage(200, 100, 1)  // some 
BufferedImage
+groovy> /img $img
+--------------
+
+When the argument resolves to a Groovy value (rather than a path), `/img`
+will accept any of:
+
+* a `java.awt.image.BufferedImage` — used as-is
+* a `java.awt.image.RenderedImage` — drawn into a `BufferedImage`
+* anything with a `createBufferedImage(int, int)` method (e.g.
+  `org.jfree.chart.JFreeChart`) — duck-typed, no compile-time dependency
+* anything with a `toBufferedImage(int, int)` method (e.g. Smile's
+  `smile.plot.swing.Figure`) — duck-typed sibling for libraries that
+  follow the `to…` rather than `create…` naming convention
+* a `javax.swing.JComponent` (e.g. Smile's `Canvas` and `MultiFigurePane`) —
+  laid out and painted to a `BufferedImage` at the requested size
+
+So once you've grabbed JFreeChart you can render charts in the REPL
+without saving to a file:
+
+image:assets/img/repl_img_jfreechart.png[Using /img with JFreeChart]
+
+Smile is similar — `Figure.toBufferedImage(int, int)` matches the second
+duck-type, so a fresh `Figure` can be handed straight to `/img`. The
+following draws the classic Iris scatter plot:
+
+image:assets/img/repl_img_smile.png[Using /img with Smile]

Review Comment:
   These new images use `image:assets/img/...` while the rest of this document 
uses `image:{reldir_groovysh}/assets/img/...`. Using the consistent 
`{reldir_groovysh}` prefix here will avoid broken image links when the docs are 
built from different base directories.
   



##########
subprojects/groovy-groovysh/src/spec/doc/groovysh.adoc:
##########
@@ -720,6 +720,75 @@ groovy> /imports
 import java.util.concurrent.BlockingQueue
 --------------
 
+[[GroovyShell-img]]
+==== `/img`
+
+The `/img` command displays an image inline using JLine's terminal-graphics
+support (Sixel, Kitty, or iTerm2 protocols, auto-detected). The argument is
+either a local file path, an `http(s)://` URL, or a Groovy variable
+reference using the standard `$` syntax.
+
+[source,jshell]
+--------------
+groovy> /img chart.png
+groovy> /img --width=80 https://example.com/diagram.png
+groovy> img = new java.awt.image.BufferedImage(200, 100, 1)  // some 
BufferedImage
+groovy> /img $img
+--------------
+
+When the argument resolves to a Groovy value (rather than a path), `/img`
+will accept any of:
+
+* a `java.awt.image.BufferedImage` — used as-is
+* a `java.awt.image.RenderedImage` — drawn into a `BufferedImage`
+* anything with a `createBufferedImage(int, int)` method (e.g.
+  `org.jfree.chart.JFreeChart`) — duck-typed, no compile-time dependency
+* anything with a `toBufferedImage(int, int)` method (e.g. Smile's
+  `smile.plot.swing.Figure`) — duck-typed sibling for libraries that
+  follow the `to…` rather than `create…` naming convention
+* a `javax.swing.JComponent` (e.g. Smile's `Canvas` and `MultiFigurePane`) —
+  laid out and painted to a `BufferedImage` at the requested size
+
+So once you've grabbed JFreeChart you can render charts in the REPL
+without saving to a file:
+
+image:assets/img/repl_img_jfreechart.png[Using /img with JFreeChart]
+
+Smile is similar — `Figure.toBufferedImage(int, int)` matches the second
+duck-type, so a fresh `Figure` can be handed straight to `/img`. The
+following draws the classic Iris scatter plot:
+
+image:assets/img/repl_img_smile.png[Using /img with Smile]

Review Comment:
   These new images use `image:assets/img/...` while the rest of this document 
uses `image:{reldir_groovysh}/assets/img/...`. Using the consistent 
`{reldir_groovysh}` prefix here will avoid broken image links when the docs are 
built from different base directories.
   



##########
subprojects/groovy-groovysh/src/main/groovy/org/apache/groovy/groovysh/jline/GroovyCommands.groovy:
##########
@@ -871,6 +1087,18 @@ class GroovyCommands extends JlineCommandRegistry 
implements CommandRegistry {
         ])
     }
 
+    private CmdDesc imgCmdDesc(String name) {
+        new CmdDesc([
+            new AttributedString("$name [OPTIONS] FILE_OR_URL"),

Review Comment:
   The `/img` help/usage line says `FILE_OR_URL`, but the implementation and 
docs also accept a Groovy variable reference (e.g. `/img $imgVar`) and other 
non-string values resolved via `xargs`. Updating the usage synopsis to reflect 
the supported inputs would make `--help` less misleading.
   



##########
subprojects/groovy-groovysh/src/main/groovy/org/apache/groovy/groovysh/jline/GroovyCommands.groovy:
##########
@@ -871,6 +1087,18 @@ class GroovyCommands extends JlineCommandRegistry 
implements CommandRegistry {
         ])
     }
 
+    private CmdDesc imgCmdDesc(String name) {
+        new CmdDesc([
+            new AttributedString("$name [OPTIONS] FILE_OR_URL"),
+        ], [], [
+            '-? --help'                       : doDescription('Displays 
command help'),
+            '-w --width=COLS'                 : doDescription('Image width in 
character columns'),
+            '   --height=ROWS'                : doDescription('Image height in 
character rows'),
+            '-p --no-preserve-aspect-ratio'   : doDescription("Don't preserve 
aspect ratio"),

Review Comment:
   `--width`/`--height` are described here as character columns/rows, but for 
generated-image inputs (`createBufferedImage`/`toBufferedImage`/`JComponent`) 
the same options are interpreted as source-image pixels (as described later in 
the user docs and implied by `dimsConsumedByGeneration`). Consider clarifying 
the option descriptions (or adding a note) so `--help` matches the actual 
behavior.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to