More updates for the API. #37
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughThis pull request introduces new modules and functions for image and filter manipulation, drawing operations, and graphic context management. It adds native bindings for image loading, resizing, and filter configuration, while updating context disposal logic. The changes span both high-level module definitions and underlying native implementations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImageModule
participant NativeImageModule
participant FileSystem
User->>ImageModule: fromPath(path)
ImageModule->>NativeImageModule: pic_nat_image_new_from_path(path)
NativeImageModule->>FileSystem: Load image from path
FileSystem-->>NativeImageModule: Image or error
NativeImageModule-->>ImageModule: Image reference
ImageModule-->>User: Image object
sequenceDiagram
participant User
participant PenModule
participant NativePenModule
participant GraphicsContext
User->>PenModule: drawImage(ctx, image, x, y)
PenModule->>NativePenModule: pic_nat_draw_image(ctx, image, x, y)
NativePenModule->>GraphicsContext: Draw image at (x, y)
GraphicsContext-->>NativePenModule: Context updated
NativePenModule-->>PenModule: Updated ctx
PenModule-->>User: Updated ctx
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
src/main/java/org/piccode/piccode/Piccode.java (1)
30-32: Module registration looks fine; consider idempotency.If initializeNativeAppModules() could be invoked more than once (e.g., tests), duplicate registrations may occur. Consider guarding with a static boolean or making Compiler.addNativeFunctions ignore duplicate providers.
piccode/filters/filter.pics (1)
2-4: Clarify API semantics and add minimal docs.Please document what apply returns (mutated image, new image, or Unit) and expected types. If it returns the image, consider mentioning it to encourage chaining.
src/main/java/org/editor/events/AccessEvents.java (1)
32-35: Good UX improvement; ensure output routing is consistent.Clearing msgs before compile and showing the start message is good. To avoid surprises if main hasn’t run, consider setting Compiler.out here too (as done in compile()) so logs always go to AccessFrame.
var code = ed.textArea.getText(); - AccessFrame.msgs.setText(""); + AccessFrame.msgs.setText(""); AccessFrame.writeSuccess("Compilation started: "); + Compiler.out = AccessFrame.AccessFrameOutputStream.out; CanvasFrame.the().compile(() -> Compiler.compile(file, code));piccode/filters/metal/brushMetal.pics (1)
6-7: Parameter naming mismatch (copy/paste).The setters amount and shine use parameter name radius, which is confusing.
- amount :: (filter, radius) = pic_nat_brush_metal_set_amount(filter, radius) - shine :: (filter, radius) = pic_nat_brush_metal_set_shine(filter, radius) + amount :: (filter, amount) = pic_nat_brush_metal_set_amount(filter, amount) + shine :: (filter, shine) = pic_nat_brush_metal_set_shine(filter, shine)piccode/pen/draw.pics (1)
62-64: Add docstrings for new Pen APIs to match existing styledrawImage/drawText lack the parameter/returns docs that other functions in this module have. Add brief blocks for consistency and discoverability.
src/main/java/org/editor/nativemods/PiccodeBrushedMetalFilterModule.java (1)
76-107: Consider validating/clamping filter parametersDepending on JHLabs BrushedMetalFilter expectations, amount/shine typically live in a bounded range (often 0..1). Consider clamping or validating to avoid surprising results.
src/main/java/org/editor/nativemods/PiccodePenModule.java (1)
191-195: Improve error message for image type checkGrammar and spacing issue; also include the actual type for clarity.
- if (!(_image instanceof BufferedImage)) { - throw new PiccodeException(caller.file, caller.line, caller.column, "Image in not a correct object. Expected a BufferedImage but found" + _image); - } + if (!(_image instanceof BufferedImage)) { + throw new PiccodeException( + caller.file, caller.line, caller.column, + "Image is not a correct object. Expected BufferedImage but found " + (_image == null ? "null" : _image.getClass().getName()) + ); + }src/main/java/org/editor/nativemods/PiccodeImageModule.java (1)
93-93: Minor style nit: stray space in List.of arguments- NativeFunctionFactory.create("image_resize", List.of("img" ,"w", "h"), (args, namedArgs, frame) -> { + NativeFunctionFactory.create("image_resize", List.of("img", "w", "h"), (args, namedArgs, frame) -> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
piccode/context/ctx.pics(1 hunks)piccode/filters/filter.pics(1 hunks)piccode/filters/metal/brushMetal.pics(1 hunks)piccode/image/image.pics(1 hunks)piccode/pen/draw.pics(1 hunks)src/main/java/org/editor/events/AccessEvents.java(1 hunks)src/main/java/org/editor/nativemods/PiccodeBrushedMetalFilterModule.java(2 hunks)src/main/java/org/editor/nativemods/PiccodeImageModule.java(2 hunks)src/main/java/org/editor/nativemods/PiccodePenModule.java(2 hunks)src/main/java/org/piccode/piccode/Piccode.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-06T23:34:19.770Z
Learnt from: hexaredecimal
PR: Glimmr-Lang/PicassoCode#26
File: tests/test_readme_validation.py:233-236
Timestamp: 2025-08-06T23:34:19.770Z
Learning: The PicassoCode project README has specific formatting requirements including: app icon at "./src/main/resources/applogo/appicon.png" with 40% width, repository URL "gitgithub.com:hexaredecimal/Piccode.git", JAR file "Piccode-1.0-SNAPSHOT-jar-with-dependencies.jar", and elaborate ASCII art with "Creativity + Logic + Math" tagline in the license section.
Applied to files:
src/main/java/org/piccode/piccode/Piccode.javasrc/main/java/org/editor/nativemods/PiccodeImageModule.javasrc/main/java/org/editor/nativemods/PiccodePenModule.java
🧬 Code Graph Analysis (1)
src/main/java/org/editor/nativemods/PiccodePenModule.java (1)
src/main/java/org/editor/nativemods/PiccodeGfxModule.java (2)
addFunctions(25-104)PiccodeGfxModule(23-106)
🔇 Additional comments (4)
src/main/java/org/piccode/piccode/Piccode.java (1)
5-9: Native module classes are present, but verify packagingAll imported modules under
org.editor.nativemodsexist and declareaddFunctions():
- PiccodeBrushedMetalFilterModule (has
addFunctions())- PiccodeFilterModule (has
addFunctions())- PiccodeImageModule (has
addFunctions())- PiccodeGfxModule (has
addFunctions())- PiccodePenModule (has
addFunctions())Please ensure these classes are correctly packaged into your runtime artifact (JAR/WAR/native bundle), as missing native modules will cause registration to fail at load time.
piccode/filters/metal/brushMetal.pics (1)
4-7: All brush metal native functions are present and registeredVerified that in
piccode/filters/metal/brushMetal.picsyou call:
- pic_nat_brush_metal_new
- pic_nat_brush_metal_set_rad
- pic_nat_brush_metal_set_amount
- pic_nat_brush_metal_set_shine
And in
src/main/java/org/editor/nativemods/PiccodeBrushedMetalFilterModule.javaeach of these is registered viaNativeFunctionFactory.create("brush_metal_<...>"). No missing symbols—no further action required.src/main/java/org/editor/nativemods/PiccodeBrushedMetalFilterModule.java (2)
77-81: Setter and float cast look correct for amountSwitching to setAmount(float) with a float-cast is the right fix here.
103-107: Setter and float cast look correct for shineSwitching to setShine(float) with a float-cast is the right fix here.
| // Returns: | ||
| // - (Unit) Nothing | ||
| drop :: (ctx) = pic_nat_gfx_(ctx) | ||
| drop :: (ctx) = pic_nat_gfx_drop(ctx) |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify native symbol change to pic_nat_gfx_drop.
Confirm that pic_nat_gfx_drop is implemented and registered in the native gfx module; otherwise this will fail at runtime.
🏁 Script executed:
#!/bin/bash
# Check that the native symbol exists somewhere in the codebase
rg -n "pic_nat_gfx_drop\b" -A2 || echo "Missing native symbol: pic_nat_gfx_drop"
# Optional: detect lingering old/typo variant to prevent drift
rg -n "pic_nat_gfx_$" || trueLength of output: 161
Missing native symbol registration for pic_nat_gfx_drop
The call to pic_nat_gfx_drop(ctx) isn’t backed by any implementation in the native gfx module. Without it, this will fail at runtime with an undefined symbol error. Please:
- Add a
pic_nat_gfx_dropdefinition in the native gfx source (e.g.native/gfx.c). - Register it in the module’s symbol table (e.g. in the array of native gfx symbols).
- Or, if the intended name differs (e.g.
pic_nat_gfx_destroy), update the call inpiccode/context/ctx.picsaccordingly.
🤖 Prompt for AI Agents
In piccode/context/ctx.pics at line 38, the function pic_nat_gfx_drop(ctx) is
called but lacks a corresponding native implementation and registration, causing
runtime errors. To fix this, add the pic_nat_gfx_drop function definition in the
native gfx source file (such as native/gfx.c), then register this function in
the native gfx module's symbol table. Alternatively, if the intended native
function has a different name, update the call in ctx.pics to match the correct
native function name.
Comment on lines
+82
to
+90
| PiccodeValue.verifyType(caller, path, Type.STRING); | ||
|
|
||
| try { | ||
| BufferedImage image = ImageIO.read(new File(path.raw().toString())); | ||
| return new PiccodeReference(image); | ||
| } catch (IOException ex) { | ||
| var def = (BufferedImage) ImageLoader.getImage(-1); | ||
| return new PiccodeReference(def); | ||
| } |
Contributor
There was a problem hiding this comment.
Casting bug and null-handling in image_new_from_path
- path is verified as STRING but used without casting; path.raw() will not compile unless cast to PiccodeString.
- ImageIO.read may return null; return a default/failure image in that case.
- PiccodeValue.verifyType(caller, path, Type.STRING);
-
- try {
- BufferedImage image = ImageIO.read(new File(path.raw().toString()));
- return new PiccodeReference(image);
- } catch (IOException ex) {
- var def = (BufferedImage) ImageLoader.getImage(-1);
- return new PiccodeReference(def);
- }
+ PiccodeValue.verifyType(caller, path, Type.STRING);
+
+ try {
+ var _path = ((PiccodeString) path).raw();
+ BufferedImage image = ImageIO.read(new File(_path));
+ if (image == null) {
+ var def = (BufferedImage) ImageLoader.getImage(-1);
+ return new PiccodeReference(def);
+ }
+ return new PiccodeReference(image);
+ } catch (IOException ex) {
+ var def = (BufferedImage) ImageLoader.getImage(-1);
+ return new PiccodeReference(def);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| PiccodeValue.verifyType(caller, path, Type.STRING); | |
| try { | |
| BufferedImage image = ImageIO.read(new File(path.raw().toString())); | |
| return new PiccodeReference(image); | |
| } catch (IOException ex) { | |
| var def = (BufferedImage) ImageLoader.getImage(-1); | |
| return new PiccodeReference(def); | |
| } | |
| PiccodeValue.verifyType(caller, path, Type.STRING); | |
| try { | |
| // Cast to PiccodeString so we can call raw() | |
| var _path = ((PiccodeString) path).raw(); | |
| BufferedImage image = ImageIO.read(new File(_path)); | |
| // ImageIO.read may return null if it can't read the file | |
| if (image == null) { | |
| var def = (BufferedImage) ImageLoader.getImage(-1); | |
| return new PiccodeReference(def); | |
| } | |
| return new PiccodeReference(image); | |
| } catch (IOException ex) { | |
| // Fall back to default image on I/O errors | |
| var def = (BufferedImage) ImageLoader.getImage(-1); | |
| return new PiccodeReference(def); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/editor/nativemods/PiccodeImageModule.java lines 82 to 90,
the variable path is verified as a STRING but not cast to PiccodeString before
calling raw(), causing a compilation error. Also, ImageIO.read can return null,
which is not handled and may cause issues. Fix this by casting path to
PiccodeString before calling raw(), and check if the BufferedImage returned by
ImageIO.read is null; if so, return the default image instead of a null
reference.
Comment on lines
+109
to
+121
| if (!(_buffered_image instanceof BufferedImage)) { | ||
| throw new PiccodeException(caller.file, caller.line, caller.column, "Expected a buffer image. Found " + _buffered_image); | ||
| } | ||
|
|
||
| var bufferedmage = (BufferedImage) _buffered_image; | ||
|
|
||
| var _w = (int) (double) ((PiccodeNumber) w).raw(); | ||
| var _h = (int) (double) ((PiccodeNumber) h).raw(); | ||
|
|
||
| Image resultingImage = bufferedmage.getScaledInstance(_w, _h, Image.SCALE_DEFAULT); | ||
| BufferedImage outputImage = new BufferedImage(_w, _h, BufferedImage.TYPE_INT_RGB); | ||
| outputImage.getGraphics().drawImage(resultingImage, 0, 0, null); | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve alpha, improve scaling path, and dispose graphics
- Error text says “buffer image” instead of “buffered image”.
- getScaledInstance drops quality and you’re forcing TYPE_INT_RGB (alpha lost). Preserve alpha if present.
- Dispose the Graphics2D to avoid resource leaks.
- if (!(_buffered_image instanceof BufferedImage)) {
- throw new PiccodeException(caller.file, caller.line, caller.column, "Expected a buffer image. Found " + _buffered_image);
- }
-
- var bufferedmage = (BufferedImage) _buffered_image;
+ if (!(_buffered_image instanceof BufferedImage)) {
+ throw new PiccodeException(caller.file, caller.line, caller.column, "Expected a buffered image. Found " + _buffered_image);
+ }
+
+ var bufferedImage = (BufferedImage) _buffered_image;
var _w = (int) (double) ((PiccodeNumber) w).raw();
var _h = (int) (double) ((PiccodeNumber) h).raw();
-
- Image resultingImage = bufferedmage.getScaledInstance(_w, _h, Image.SCALE_DEFAULT);
- BufferedImage outputImage = new BufferedImage(_w, _h, BufferedImage.TYPE_INT_RGB);
- outputImage.getGraphics().drawImage(resultingImage, 0, 0, null);
-
- return new PiccodeReference(outputImage);
+
+ int destType = bufferedImage.getColorModel().hasAlpha()
+ ? BufferedImage.TYPE_INT_ARGB
+ : BufferedImage.TYPE_INT_RGB;
+ BufferedImage outputImage = new BufferedImage(_w, _h, destType);
+ Graphics2D g2 = outputImage.createGraphics();
+ try {
+ g2.drawImage(bufferedImage, 0, 0, _w, _h, null);
+ } finally {
+ g2.dispose();
+ }
+
+ return new PiccodeReference(outputImage);🤖 Prompt for AI Agents
In src/main/java/org/editor/nativemods/PiccodeImageModule.java lines 109 to 121,
update the exception message to say "Expected a buffered image" instead of
"buffer image". Replace getScaledInstance with a higher-quality scaling method
such as using Graphics2D's drawImage with rendering hints to improve quality.
Detect if the original BufferedImage has alpha and create the output
BufferedImage with a type that preserves alpha (e.g., TYPE_INT_ARGB) instead of
forcing TYPE_INT_RGB. Finally, cast the Graphics object to Graphics2D and call
dispose() on it after drawing to properly release resources.
Comment on lines
+206
to
+232
| var _ctx = namedArgs.get("ctx"); | ||
| var _text = namedArgs.get("text"); | ||
| var x = namedArgs.get("x"); | ||
| var y = namedArgs.get("y"); | ||
|
|
||
| var ctx = frame == null ? | ||
| Context.top | ||
| : Context.getContextAt(frame); | ||
| var caller = ctx.getTopFrame().caller; | ||
|
|
||
| PiccodeValue.verifyType(caller, _ctx, Type.REFERENCE); | ||
| PiccodeValue.verifyType(caller, _text, Type.STRING); | ||
| PiccodeValue.verifyType(caller, x, Type.NUMBER); | ||
| PiccodeValue.verifyType(caller, y, Type.NUMBER); | ||
|
|
||
| var obj = (PiccodeReference) _ctx; | ||
| var _gfx = obj.deref(); | ||
| if (!(_gfx instanceof Graphics2D)) { | ||
| throw new PiccodeException(caller.file, caller.line, caller.column, "Context is not a correct object. Expected Graphics2D"); | ||
| } | ||
| var gfx = (Graphics2D) _gfx; | ||
| var _x = (int) (double) ((PiccodeNumber) x).raw(); | ||
| var _y = (int) (double) ((PiccodeNumber) y).raw(); | ||
|
|
||
| gfx.drawString(_text.toString(), _x, _y); | ||
| return obj; | ||
| }, null); |
Contributor
There was a problem hiding this comment.
draw_text uses toString() on PiccodeString instead of the raw string (renders wrong text)
Use the underlying string value; toString() will likely print the object identity.
- var gfx = (Graphics2D) _gfx;
- var _x = (int) (double) ((PiccodeNumber) x).raw();
- var _y = (int) (double) ((PiccodeNumber) y).raw();
-
- gfx.drawString(_text.toString(), _x, _y);
+ var gfx = (Graphics2D) _gfx;
+ var _x = (int) (double) ((PiccodeNumber) x).raw();
+ var _y = (int) (double) ((PiccodeNumber) y).raw();
+ var text = ((PiccodeString) _text).raw();
+
+ gfx.drawString(text, _x, _y);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var _ctx = namedArgs.get("ctx"); | |
| var _text = namedArgs.get("text"); | |
| var x = namedArgs.get("x"); | |
| var y = namedArgs.get("y"); | |
| var ctx = frame == null ? | |
| Context.top | |
| : Context.getContextAt(frame); | |
| var caller = ctx.getTopFrame().caller; | |
| PiccodeValue.verifyType(caller, _ctx, Type.REFERENCE); | |
| PiccodeValue.verifyType(caller, _text, Type.STRING); | |
| PiccodeValue.verifyType(caller, x, Type.NUMBER); | |
| PiccodeValue.verifyType(caller, y, Type.NUMBER); | |
| var obj = (PiccodeReference) _ctx; | |
| var _gfx = obj.deref(); | |
| if (!(_gfx instanceof Graphics2D)) { | |
| throw new PiccodeException(caller.file, caller.line, caller.column, "Context is not a correct object. Expected Graphics2D"); | |
| } | |
| var gfx = (Graphics2D) _gfx; | |
| var _x = (int) (double) ((PiccodeNumber) x).raw(); | |
| var _y = (int) (double) ((PiccodeNumber) y).raw(); | |
| gfx.drawString(_text.toString(), _x, _y); | |
| return obj; | |
| }, null); | |
| var obj = (PiccodeReference) _ctx; | |
| var _gfx = obj.deref(); | |
| if (!(_gfx instanceof Graphics2D)) { | |
| throw new PiccodeException(caller.file, caller.line, caller.column, "Context is not a correct object. Expected Graphics2D"); | |
| } | |
| var gfx = (Graphics2D) _gfx; | |
| var _x = (int) (double) ((PiccodeNumber) x).raw(); | |
| var _y = (int) (double) ((PiccodeNumber) y).raw(); | |
| var text = ((PiccodeString) _text).raw(); | |
| gfx.drawString(text, _x, _y); | |
| return obj; |
🤖 Prompt for AI Agents
In src/main/java/org/editor/nativemods/PiccodePenModule.java between lines 206
and 232, the drawString call uses toString() on the PiccodeString object, which
may print the object identity instead of the actual string content. Replace the
toString() call with a method or property that retrieves the raw underlying
string value from the PiccodeString instance to ensure the correct text is
rendered.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores