CATROID-1615 "Write embroidery data to file" Brick Not Saving .dst File on Smartphone#5161
CATROID-1615 "Write embroidery data to file" Brick Not Saving .dst File on Smartphone#5161doppem15 wants to merge 3 commits intoCatrobat:developfrom
Conversation
|
wslany
left a comment
wslany
left a comment
There was a problem hiding this comment.
I think this PR needs regression coverage for the behavior moved into BaseFileStorageAction:
- a legacy read test for explicit non-
.txtextensions likeconfig.csv - a null-safety test proving
act()does not crash whenformula == null(and ideally also with missingscopewhere relevant)
| protected fun getFileName(): String { | ||
| var fileName = Utils.sanitizeFileName(formula?.interpretString(scope)) | ||
| if (!fileName.endsWith(fileExtension)) { | ||
| fileName += fileExtension |
There was a problem hiding this comment.
This looks like a behavior change for ReadVariableFromFileAction on legacy storage.
Previously the read action only added .txt when there was no extension at all. With the shared helper, config.csv now becomes config.csv.txt, which would break existing projects reading non-.txt files.
Could we keep the old extension handling for the read action?
| override fun act(delta: Float): Boolean { | ||
| if (checkIfDataIsReady()) { | ||
| startFileInteraction() | ||
| } else { | ||
| showDataMissingMessage() | ||
| } |
There was a problem hiding this comment.
I think this refactor removed the old null guard.
Before, these actions returned early when formula == null (and some also tolerated missing scope). Now act() can still reach getFileName() or scope!! with null state and crash instead of no-oping.
Could we restore a common formula/scope guard in the base class?
There was a problem hiding this comment.
Pull request overview
This PR addresses storage/file-writing failures on older Android versions by centralizing file picker vs. legacy external storage handling into a new shared action base class, and adds/updates tests around plot/laser/embroidery/variable file operations.
Changes:
- Introduces
BaseFileStorageActionto unify URI (SAF) vs. legacy external storage flows for multiple “save/write/read to file” actions. - Refactors plot/laser/embroidery/variable file actions to use the new base logic and adds new error strings for laser/variable operations.
- Adds new/updated unit and instrumented tests for the refactored actions and base behavior.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| catroid/src/main/java/org/catrobat/catroid/content/actions/BaseFileStorageAction.kt | New shared implementation for legacy vs. SAF flows (core of the refactor). |
| catroid/src/main/java/org/catrobat/catroid/content/actions/SavePlotAction.kt | Migrates plot saving to the shared base class; adds plot-data readiness check/toast. |
| catroid/src/main/java/org/catrobat/catroid/content/actions/SaveLaserAction.kt | Extends plot saving to laser data; adds laser-data readiness check/toast. |
| catroid/src/main/java/org/catrobat/catroid/content/actions/WriteEmbroideryToFileAction.kt | Migrates DST writing to the shared base class. |
| catroid/src/main/java/org/catrobat/catroid/content/actions/WriteVariableToFileAction.kt | Migrates variable write-to-file to the shared base class. |
| catroid/src/main/java/org/catrobat/catroid/content/actions/ReadVariableFromFileAction.kt | Migrates variable read-from-file and optional deletion to the shared base class. |
| catroid/src/main/java/org/catrobat/catroid/io/StorageOperations.java | Adds copyUriContentToFile(...) helper for SAF reads. |
| catroid/src/main/java/org/catrobat/catroid/plot/SVGPlotGenerator.kt | Adds hasPlotData() / hasLaserData() helpers used by actions. |
| catroid/src/main/java/org/catrobat/catroid/content/Project.java | Removes storage permissions requirements on Android Q+ at the project level. |
| catroid/src/main/java/org/catrobat/catroid/content/bricks/SavePlotBrick.kt | Declares storage permissions as required resources for the brick. |
| catroid/src/main/java/org/catrobat/catroid/content/bricks/SaveLaserBrick.kt | Declares storage permissions as required resources for the brick. |
| catroid/src/main/java/org/catrobat/catroid/content/ActionFactory.java | Minor edit in plot action creation (formatting/indentation). |
| catroid/src/main/java/org/catrobat/catroid/utils/Utils.java | Adds isLegacyStorageManager() utility. |
| catroid/src/main/res/values/strings.xml | Adds new error strings for laser missing and variable read/write failures. |
| catroid/src/test/java/org/catrobat/catroid/test/content/actions/WriteVariableToFileActionTest.kt | Reworks JVM test to cover new variable write logic. |
| catroid/src/test/java/org/catrobat/catroid/test/content/actions/ReadVariableFromFileActionTest.kt | Reworks JVM test to cover new variable read logic. |
| catroid/src/androidTest/java/org/catrobat/catroid/test/content/actions/BaseFileStorageActionTest.kt | Adds instrumentation coverage for base storage action behavior. |
| catroid/src/androidTest/java/org/catrobat/catroid/test/content/actions/SavePlotActionTest.kt | Adds instrumentation coverage for plot saving logic. |
| catroid/src/androidTest/java/org/catrobat/catroid/test/content/actions/SaveLaserActionTest.kt | Adds instrumentation coverage for laser saving logic. |
| catroid/src/androidTest/java/org/catrobat/catroid/test/content/actions/WriteEmbroideryToFileActionTest.kt | Adds instrumentation coverage for embroidery DST writing. |
| catroid/src/androidTest/java/org/catrobat/catroid/test/content/actions/SimpleTest.kt | Adds a basic instrumentation sanity test. |
| catroid/src/*/java/org/catrobat/catroid/common/FlavoredConstants.java (multiple flavors) | Updates help/privacy/base URLs for multiple product flavors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| protected fun getFileName(): String { | ||
| var fileName = Utils.sanitizeFileName(formula?.interpretString(scope)) | ||
| if (!fileName.endsWith(fileExtension)) { | ||
| fileName += fileExtension | ||
| } | ||
| return fileName |
There was a problem hiding this comment.
getFileName() calls Utils.sanitizeFileName(formula?.interpretString(scope)); if formula is null (or interpretation returns null), this will pass null into a Java method and crash at runtime. Make getFileName() null-safe (e.g., default to an empty filename or early-return with an error) and/or ensure act()/checkIfDataIsReady() prevents execution when formula is missing.
| if (handleFileWork(file)) { | ||
| onPostExecute(fileName, null) | ||
| } else { |
There was a problem hiding this comment.
performLegacyOperation() invokes handleFileWork(file) without any exception handling. Current implementations call File.readText()/writeText() and generators that can throw (e.g., IOException), which would crash the stage instead of showing errorMessageResId. Wrap handleFileWork in try/catch (at least IOException) and return/show the error toast on failures.
| if (handleFileWork(file)) { | |
| onPostExecute(fileName, null) | |
| } else { | |
| try { | |
| if (handleFileWork(file)) { | |
| onPostExecute(fileName, null) | |
| } else { | |
| showErrorMessage() | |
| } | |
| } catch (e: IOException) { | |
| Log.e("FileStorageAction", "Error, legacy storage operation failed", e) |
| protected fun performUriOperation(uri: Uri?) { | ||
| if (uri == null) { | ||
| Log.e("FileStorageAction", "User cancelled or URI is invalid") | ||
| } | ||
| val activity = StageActivity.activeStageActivity?.get() ?: return | ||
| val contentResolver = activity.contentResolver | ||
| val fileName = if (uri != null) { | ||
| StorageOperations.resolveFileName(contentResolver, uri) ?: getFileName() | ||
| } else { | ||
| getFileName() | ||
| } | ||
| val cacheFile = File(Constants.CACHE_DIRECTORY, fileName) | ||
| try { | ||
| if (isReadAction) { | ||
| StorageOperations.copyUriContentToFile(contentResolver, uri, cacheFile) | ||
| } else if (!cacheFile.exists()) { | ||
| cacheFile.createNewFile() |
There was a problem hiding this comment.
performUriOperation() logs when uri == null but continues execution and (for read actions) passes the null URI into StorageOperations.copyUriContentToFile(...), which can cause a crash. If the user cancels (or URI is invalid), return early and avoid any file work/toasts; also make the URI parameter non-null for the read path.
| override fun checkIfDataIsReady(): Boolean = | ||
| SVGPlotGenerator(scope!!.sprite.plot!!).hasPlotData() |
There was a problem hiding this comment.
checkIfDataIsReady() dereferences scope!!/plot!!. If scope is unexpectedly null (or action is constructed incorrectly), this will throw before you can show the intended "Plot is missing" toast. Prefer a null-safe check (return false when scope is null) and let showDataMissingMessage() handle it.
| override fun checkIfDataIsReady(): Boolean = | |
| SVGPlotGenerator(scope!!.sprite.plot!!).hasPlotData() | |
| override fun checkIfDataIsReady(): Boolean { | |
| val plot = scope?.sprite?.plot ?: return false | |
| return SVGPlotGenerator(plot).hasPlotData() | |
| } |
| override fun checkIfDataIsReady(): Boolean = | ||
| SVGPlotGenerator(scope!!.sprite.plot!!).hasLaserData() |
There was a problem hiding this comment.
checkIfDataIsReady() dereferences scope!!/plot!!, but handleFileWork() is already null-safe and returns false when scope/plot is missing. Align checkIfDataIsReady() with handleFileWork() to avoid a potential NPE before showing the "Laser is missing" toast.
| override fun checkIfDataIsReady(): Boolean = | |
| SVGPlotGenerator(scope!!.sprite.plot!!).hasLaserData() | |
| override fun checkIfDataIsReady(): Boolean { | |
| val scope = this.scope ?: return false | |
| val plot = scope.sprite.plot ?: return false | |
| return SVGPlotGenerator(plot).hasLaserData() | |
| } |
| SavePlotAction action = Actions.action(SavePlotAction.class); | ||
| Scope scope = new Scope(ProjectManager.getInstance().getCurrentProject(), sprite, sequence); | ||
| action.setScope(scope); | ||
| action.setScope(scope); |
There was a problem hiding this comment.
There’s an extra indentation level before action.setScope(scope); compared to the surrounding code. If the repo enforces Java formatting via Spotless/Checkstyle, this can fail CI. Please align indentation with adjacent lines.
| action.setScope(scope); | |
| action.setScope(scope); |
| import org.jetbrains.annotations.NotNull; | ||
| import org.jetbrains.annotations.Nullable; | ||
|
|
There was a problem hiding this comment.
Unused imports NotNull/Nullable were added but not used anywhere in this file. Please remove them to avoid lint/formatting noise and keep the import list clean.
| import org.jetbrains.annotations.NotNull; | |
| import org.jetbrains.annotations.Nullable; |
| @Test | ||
| fun testCheckIfDataIsReady() { | ||
| action.userVariable = null | ||
| assertTrue("ction should not be ready when no user variable is assigned", !action.checkIfDataIsReady()) |
There was a problem hiding this comment.
Test assertion message has a typo: "ction should not be ready...". Fixing the message improves clarity when the test fails.
| assertTrue("ction should not be ready when no user variable is assigned", !action.checkIfDataIsReady()) | |
| assertTrue("Action should not be ready when no user variable is assigned", !action.checkIfDataIsReady()) |
| class WriteVariableToFileActionTest( | ||
| private val name: String, | ||
| private val formula: Formula?, | ||
| private val userVariable: UserVariable?, | ||
| private val expectedFileContent: String, | ||
| private val createFile: Int, | ||
| private val writeToFile: Int | ||
| private val shouldWork: Boolean | ||
| ) { | ||
| private lateinit var sprite: Sprite | ||
| private lateinit var sequence: SequenceAction | ||
| private lateinit var file: File | ||
| private lateinit var mockContext: Context | ||
| private lateinit var mockResources: Resources | ||
| private lateinit var testFile: File | ||
|
|
There was a problem hiding this comment.
This parameterized test defines shouldWork, plus mockContext/mockResources/sprite, but none of them are used in the assertions. Unused constructor params/fields can be flagged by IDE/static analysis and make the test intent harder to follow—either use them (e.g., assert failure paths when shouldWork == false) or remove them.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
detekt found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| SavePlotAction action = Actions.action(SavePlotAction.class); | ||
| Scope scope = new Scope(ProjectManager.getInstance().getCurrentProject(), sprite, sequence); | ||
| action.setScope(scope); | ||
| action.setScope(scope); |
| import android.provider.OpenableColumns; | ||
| import android.util.Log; | ||
|
|
||
| import org.jetbrains.annotations.NotNull; |
| import android.util.Log; | ||
|
|
||
| import org.jetbrains.annotations.NotNull; | ||
| import org.jetbrains.annotations.Nullable; |
wslany
left a comment
wslany
left a comment
There was a problem hiding this comment.
Please resolve copilot findings.
|
| return if (file.exists()) file else null | ||
| } | ||
|
|
||
| protected fun getFileName(): String { |
There was a problem hiding this comment.
I rechecked the latest version after the rebase/fix commits. The original ReadVariableFromFileAction extension regression and null guard issue look addressed, but I think one related filename regression remains.
BaseFileStorageAction.getFileName() now preserves any explicit extension for all actions. That matches the old read-variable behavior, but it changes the write actions: SavePlotAction, SaveLaserAction, WriteEmbroideryToFileAction, and WriteVariableToFileAction previously appended their required extension unless the filename already ended with that exact extension.
For example, Write embroidery data to "foo.txt" used to save foo.txt.dst; now it saves foo.txt, and Save plot to "foo.txt" no longer produces an .svg filename.
Could we keep the “preserve arbitrary extension” behavior only for reads, while write/export actions continue enforcing their own output extension (.txt, .svg, .dst)? A small regression test for a write action with a mismatched explicit extension would help lock this down.
I fixed a bug where the "Save plot to" and "Write embroidery data" bricks failed on older Android devices (SDK <29). While writing the tests, I noticed a lot of duplicated code, so I created a new base class to unify the logic. This makes the storage handling cleaner and ensures it now works reliably across all Android versions.
Additionally, many new tests were added to make the code more reliable:
-Instrumented Tests were required for SVG, Laser, and Embroidery.
-Instrumented Tests were also used for the Base class.
-Unit Tests were required for Variable actions.
https://catrobat.atlassian.net/browse/CATROID-1615
Your checklist for this pull request
Please review the contributing guidelines and wiki pages of this repository.