IDE-309 Fix FormulaEditorUndoTest NPE in checkVariables

[harshsomankar123-tech]

Description

This PR addresses a NullPointerException in FormulaEditorUndoTest and fixes a UI issue where the script view did not update immediately after an undo operation.

What's the issue?

1. NPE in Tests: 7 out of 9 tests in FormulaEditorUndoTest fail with a NullPointerException in ScriptFragment.checkVariables(). When exiting the formula editor via pressBack(), FormulaEditorFragment.exitFormulaEditorFragment() calls fragment.checkVariables(), which passes saved lists (like savedUserVariables) to Project.hasUserDataChanged(). These fields are null when no undo snapshot has been captured, causing oldUserData.size() to throw an NPE. The 2 passing tests don't hit this path because their formulas remain unchanged.
2. Stale UI after Undo: After performing an undo action, the script view was not reflecting the restored state immediately; the user had to leave and reopen the fragment to see the changes.

Fix

• Refined Null Handling: Updated ScriptFragment.checkVariables() to use more selective null handling. This ensures that valid undo states are no longer skipped when only some snapshots are missing.
• Immediate UI Refresh: Updated the UI logic so the script view now updates immediately after an undo. The adapter is explicitly refreshed with the updated data, triggering a proper fragment redraw.
• Fragment Check: Maintained the null check in FormulaEditorFragment.exitFormulaEditorFragment() for the fragment before calling checkVariables() to prevent an NPE if the ScriptFragment is not found by its tag.
• New Regression Test: Added testImmediateUndoRefresh to verify that the script view correctly reflects the restored state right after an undo without needing to leave and reopen the fragment.

Changes

• ScriptFragment.java:
  • Refactored checkVariables() to use selective if block null checks instead of a single chained boolean expression to better handle partial undo snapshots.
  • Added null checks for currentSprite, savedLocalUserVariables, and savedLocalLists before attempting to restore user data values.
  • Updated refreshFragmentAfterUndo() to include a null check for scriptFragment, and changed fragmentTransaction.commit() to commitNow() to enforce an immediate, synchronous UI update.
  • Added adapter.updateItems() inside onLoadFinished() to ensure the UI reflects the restored state.
• UndoTest.java: * Added the testImmediateUndoRefresh() UI test to verify that the script view correctly updates immediately after an undo action is performed.
• JobHandlerEventTest.java:
  • Updated testReceivedOnJobScheduledEvent() to use verifyNoMoreInteractions() instead of verifyNoInteractions() for the convertCallbackMock.

checklist for this pull request

Please review the contributing guidelines and wiki pages of this repository.

• Include the name of the Jira ticket in the PR’s title
• Include a summary of the changes plus the relevant context
• Choose the proper base branch (develop)
• Confirm that the changes follow the project’s coding guidelines
• Verify that the changes generate no compiler or linter warnings
• Perform a self-review of the changes
• Verify to commit no other files than the intentionally changed ones
• Include reasonable and readable tests verifying the added or changed behavior
• Confirm that new and existing unit tests pass locally
• Check that the commits’ message style matches the project’s guideline
• Stick to the project’s gitflow workflow
• Verify that your changes do not have any conflicts with the base branch
• After the PR, verify that all CI checks have passed
• Post a message in the catroid-stage or catroid-ide Slack channel and ask for a code reviewer

[wslany]

On tests, I don’t think the previously failing undo tests are enough to lock this down, because they cover the normal undo flow but not this new null-snapshot branch. It would be great to add a regression test that explicitly reproduces the recreated/missing-snapshot scenario and verifies the intended undo behavior, not just that the crash is gone.

[wslany]

I did some manual testing on current develop and found what seems to be a more fundamental undo issue that may be relevant here. When undo is triggered from the script view, the underlying project state appears to be restored, but the visible script view is often not updated immediately. For example, if I edit a formula, leave the formula editor, and press undo, the undo button disappears, but the old value still remains visible in the brick field until I leave the script view and enter it again. I see the same behavior in other undo situations as well, for example after deleting a brick and then undoing it. So this PR may indeed fix the NPE symptom, but it doesn’t seem to address the broader undo/view-refresh problem, and that broader issue might also be related to the failing tests. Because of that, I also don’t think the previously failing tests alone are sufficient; it would be helpful to have coverage that checks the script view is updated immediately after undo, not only that the underlying state is restored.

[harshsomankar123-tech]

Hi @wslany, thanks a lot for the detailed feedback!

I took a closer look at the broader undo issue you pointed out. You’re right the project state is being restored correctly, but the script view isn’t updating immediately. It seems like the adapter is still holding onto the old Sprite reference after undo, which is why the UI doesn’t refresh.

From what I’ve seen, we likely need to explicitly update the adapter with the new Sprite instance after undo to trigger a proper UI refresh. I’m currently working on that, along with adding a regression test to cover this behavior as you suggested.

I’ll push an updated fix soon .

[harshsomankar123-tech]

Hi @wslany
I’ve updated the PR with a more complete fix that addresses both the NPE and the UI refresh issue.

For the UI, the script view now updates immediately after undo by ensuring the adapter is refreshed with the updated data and triggering a proper fragment redraw. I also refined the null handling to be more selective, so we don’t skip valid undo states when only some snapshots are missing.

Additionally, I added a regression test (testImmediateUndoRefresh) to verify that the script view reflects the restored state right after undo, without needing to leave and reopen the fragment.

The changes are now pushed would appreciate it if you could take another look when you have time .
Thanks!

Screen.Recording.2026-03-28.at.01.04.19.mov

[wslany]

Hi @wslany I’ve updated the PR with a more complete fix that addresses both the NPE and the UI refresh issue.

Totally awesome! Many thanks! I'm looking at the details now, but could you please also edit the text of the description of the PR at the top so it reflects the new situation and the bugfix?

[harshsomankar123-tech]

Hi @wslany I’ve updated the PR with a more complete fix that addresses both the NPE and the UI refresh issue.

Totally awesome! Many thanks! I'm looking at the details now, but could you please also edit the text of the description of the PR at the top so it reflects the new situation and the bugfix?

Done

[wslany]

Screen.Recording.2026-03-28.at.01.04.19.mov

Thanks, this looks much better now. The selective null handling addresses my earlier concern about checkVariables(), and the new tests are not just duplicates of what we already had: they actually cover gaps in the existing undo coverage.

I checked the current undo-related tests in the repo, and there already are quite a few older Espresso tests for undo: UndoTest, FormulaEditorUndoTest, ActionBarUndoSpinnerTest, LookUndoTest, and BackpackUndoTest. So in principle undo was already covered in several areas. The interesting part is that some of the older tests are too weak for the regression we are discussing here. In particular, the old UndoTest only verified that the project state was restored, or that the correct state was visible after leaving and re-entering the script view. That means it could still pass even if undo did not refresh the script view immediately. So the new testImmediateUndoRefresh seems justified to me, because it closes a real gap in the old coverage rather than duplicating it. Likewise, the new recreation-related formula editor test adds coverage for a scenario that the old undo tests did not explicitly cover.

One thing I still think is missing, though, is the toolbar undo-button state after script-view undo. In the video, the top undo button still stayed visible after the undo operation. Looking at the code, loadProjectAfterUndoOption() starts the reload, but I still don’t see an explicit setUndoMenuItemVisibility(false) / showUndo(false) in that path, so it seems possible for the UI state to remain stale until a later lifecycle/menu refresh. I think that should still be fixed explicitly.

So from my side the remaining task would be: could we also cover the toolbar visibility with a regression test that verifies the undo button disappears immediately after undo in the script view / formula-editor flow? That seems to be the last important gap I still see in the current patch.

[harshsomankar123-tech]

Screen.Recording.2026-03-28.at.01.04.19.mov

Thanks, this looks much better now. The selective null handling addresses my earlier concern about checkVariables(), and the new tests are not just duplicates of what we already had: they actually cover gaps in the existing undo coverage.

I checked the current undo-related tests in the repo, and there already are quite a few older Espresso tests for undo: UndoTest, FormulaEditorUndoTest, ActionBarUndoSpinnerTest, LookUndoTest, and BackpackUndoTest. So in principle undo was already covered in several areas. The interesting part is that some of the older tests are too weak for the regression we are discussing here. In particular, the old UndoTest only verified that the project state was restored, or that the correct state was visible after leaving and re-entering the script view. That means it could still pass even if undo did not refresh the script view immediately. So the new testImmediateUndoRefresh seems justified to me, because it closes a real gap in the old coverage rather than duplicating it. Likewise, the new recreation-related formula editor test adds coverage for a scenario that the old undo tests did not explicitly cover.

One thing I still think is missing, though, is the toolbar undo-button state after script-view undo. In the video, the top undo button still stayed visible after the undo operation. Looking at the code, loadProjectAfterUndoOption() starts the reload, but I still don’t see an explicit setUndoMenuItemVisibility(false) / showUndo(false) in that path, so it seems possible for the UI state to remain stale until a later lifecycle/menu refresh. I think that should still be fixed explicitly.

So from my side the remaining task would be: could we also cover the toolbar visibility with a regression test that verifies the undo button disappears immediately after undo in the script view / formula-editor flow? That seems to be the last important gap I still see in the current patch.

@wslany Thanks! I’ll take care of it and update the PR with the fix.

[harshsomankar123-tech]

@wslany I’ve implemented the requested changes. Additionally, I explored improving the user experience by introducing a Redo button to complement the current Undo behavior.
At the moment, Undo only reverts the most recent change , which might feel limiting for users when working with multiple edits. Adding Redo would make the interaction more flexible and user-friendly.

Please let me know if you’re okay with this enhancement I'd be happy to push the changes as well.
Before

Screenrecorder-2026-03-28-15-11-24-985.mp4

After

Screenrecorder-2026-03-28-15-24-53-264.mp4

[wslany]

@harshsomankar123-tech Many thanks, this looks much better now.

I’ve checked the latest changes, and the explicit hiding of the undo button plus the added regression tests address the main remaining concern I had about the basic script-view undo flow. The selective null handling and the UI refresh/update changes also make the fix feel much more complete.

I also had another look at the older undo-related tests. There seems to be some broader legacy test debt in this area that predates your PR. For example, the undo-related backpack commit (8e39932) introduced its own Espresso coverage, and even there the test suite now shows some maintenance/discovery issues. So I would not expect this PR to clean up all older undo tests.

What still seems directly relevant here, though, is the formula-editor-related undo path: when I ran the current undo Espresso tests locally, UndoTest was green, but FormulaEditorUndoTest was still largely red, with several failures around the undo button still being visible after undo, and one separate failure in the variable edit flow. So from my side, the main remaining question is whether the formula-editor-to-script undo path is now fully fixed as well, or whether there is still a gap there.

The Redo/history idea sounds great as well. I think that would be a valuable UX improvement, but since it goes quite a bit beyond the current bugfix, my first impression is that it would be better in a separate ticket / PR.

Before going in that direction, I’d be interested in a few design details:

• how many steps of history would be supported?
• in which situations would the history be reset?
• would the undo/redo history apply only to the current view, or more broadly be preserved across views and editing actions?

For this PR, I’d be happy to keep the scope on the bugfix, and then discuss Redo/history, or broader “single undo in more places” support, as follow-up work in separate tickets/PRs.

[harshsomankar123-tech]

@wslany I’ve made all the changes based on your feedback. Please review it when you have time.
Thank you!

[harshsomankar123-tech]

The Redo/history idea sounds great as well. I think that would be a valuable UX improvement, but since it goes quite a bit beyond the current bugfix, my first impression is that it would be better in a separate ticket / PR.

Before going in that direction, I’d be interested in a few design details:

• how many steps of history would be supported?
• in which situations would the history be reset?
• would the undo/redo history apply only to the current view, or more broadly be preserved across views and editing actions?

For this PR, I’d be happy to keep the scope on the bugfix, and then discuss Redo/history, or broader “single undo in more places” support, as follow-up work in separate tickets/PRs.

@wslany thanks for the feedback!

Based on what I’ve implemented so far, here’s how it currently works:
History size
Right now it supports up to 20 undo steps. I kept this limit to balance usability and storage, since each step saves a snapshot of the project XML.

When history is reset
The history resets whenever a new editing session starts (when entering SpriteActivity)
The undo_history directory is cleared on initialization
Also, the redo stack gets cleared if a new action is performed after an undo

Scope of history
The history works across the whole project, not just a single screen:
It tracks changes in both ScriptFragment and FormulaEditorFragment
Each entry stores sceneName and spriteName, so undo can bring the user back to the correct context
Snapshots are stored on disk during the session, but everything resets when a new session starts

Let me know if this matches what you had in mind, or if you’d like me to adjust anything (for example, keeping history across app restarts).

If this approach looks good, I can open a new PR with these changes.
Also, could you please create a Jira ticket for this? I don’t have permission to create one yet.

[wslany]

@wslany I’ve made all the changes based on your feedback. Please review it when you have time. Thank you!

@harshsomankar123-tech Thanks, this looks much better now.

I rechecked the latest changes, and from my side the main concerns about the production code seem addressed now. In particular, the latest ScriptFragment changes make sense to me: always restoring variable values after reload is important here, and explicitly hiding the undo button again after the fragment refresh seems to fix the lifecycle/menu-refresh issue we were still seeing.

I also reran the focused undo Espresso tests locally. UndoTest is green now, and FormulaEditorUndoTest improved significantly as well: the earlier failures around the undo button still being visible after undo are gone, including the new regression test for that behavior.

At this point, I only still see two remaining test-related issues:

testUndoAfterActivityRecreationDoesNotCrash is still failing with NoActivityResumedException after recreate(), so that new recreation regression test seems to need a bit more work before it is reliable.
testUndoFormulaEditVariable is still failing with a ClassCastException in DataListFragment. That looks to me more like a separate pre-existing issue in the variable edit flow than something introduced by this PR.
So overall, the main bugfix itself now looks good to me. The only follow-up I would still ask for in this PR is whether you want to adjust/fix the new recreation test as well, since it is part of the added regression coverage. The separate variable-edit crash could likely be handled independently.

[harshsomankar123-tech]

At this point, I only still see two remaining test-related issues:

testUndoAfterActivityRecreationDoesNotCrash is still failing with NoActivityResumedException after recreate(), so that new recreation regression test seems to need a bit more work before it is reliable. testUndoFormulaEditVariable is still failing with a ClassCastException in DataListFragment. That looks to me more like a separate pre-existing issue in the variable edit flow than something introduced by this PR. So overall, the main bugfix itself now looks good to me. The only follow-up I would still ask for in this PR is whether you want to adjust/fix the new recreation test as well, since it is part of the added regression coverage. The separate variable-edit crash could likely be handled independently.

@wslany Thanks for the clarification!
ohk I will fix testUndoAfterActivityRecreationDoesNotCrash and update the PR shortly .

[wslany]

@wslany thanks for the feedback!

Based on what I’ve implemented so far, here’s how it currently works: History size Right now it supports up to 20 undo steps. I kept this limit to balance usability and storage, since each step saves a snapshot of the project XML.

When history is reset The history resets whenever a new editing session starts (when entering SpriteActivity) The undo_history directory is cleared on initialization Also, the redo stack gets cleared if a new action is performed after an undo

Scope of history The history works across the whole project, not just a single screen: It tracks changes in both ScriptFragment and FormulaEditorFragment Each entry stores sceneName and spriteName, so undo can bring the user back to the correct context Snapshots are stored on disk during the session, but everything resets when a new session starts

Let me know if this matches what you had in mind, or if you’d like me to adjust anything (for example, keeping history across app restarts).

If this approach looks good, I can open a new PR with these changes. Also, could you please create a Jira ticket for this? I don’t have permission to create one yet.

@harshsomankar123-tech Thanks, that sounds very promising.

The overall direction makes sense to me:

• limiting the history to 20 steps sounds reasonable
• clearing the redo stack after a new action also matches the usual expected behavior
• tracking the whole project/session instead of only a single screen is definitely the more useful approach
• restoring the correct scene/sprite context on undo/redo is especially nice
• Resetting the history when entering a new editing session also sounds like a good scope to me. I would keep it that way rather than trying to preserve history across app restarts

So from my side, yes, this sounds like a good basis for a separate PR.

About the Jira ticket: I agree this should have its own ticket. I can take care of creating one from our side. Before I create it, would you like to suggest a preferred title/scope wording for it, so that it matches the design you already prototyped as closely as possible?

[harshsomankar123-tech]

Hi @wslany i have done all 10 test please take a look ,

testUndoFormulaEditVariable is still failing with a ClassCastException in DataListFragment.

i see second test were fail becoz of the ClassCastException in DataListFragment.kt was causing testUndoFormulaEditVariable to fail.

Root Cause of the Failure:
testUndoFormulaEditVariable was failing due to a ClassCastException in DataListFragment.kt. The showEditDialog method was implicitly treating item.value as a String because of the generic constraint UserData, but the UserVariable was initialized with an Integer value (5), causing the crash at runtime.

My approach:
In showEditDialog, I replaced the implicit cast with a safer conversion:

(item as UserData<*>).value?.toString() ?: ""

This avoids unsafe casting and works correctly for all value types (Integer, Double, String, and null).
In editItem, I’m parsing the input string back to Double so it stays consistent with how the Formula Editor stores variable values.
Also, while looking into this, I noticed that testUndoFormulaRenameVariable manually calls userVariable.setName(VARIABLE_NAME) after undo. That made me think the undo logic might not be fully restoring the variable name on its own, which seems like a pre-existing issue.

I also improved the Espresso test stability by ensuring context menu clicks use inRoot(isPlatformPopup()) and adding closeSoftKeyboard() where necessary to prevent UI interference during the test run

Happy to dig into that separately and open another PR if that would help.

Let me know if this approach looks good .
Thanks!

[harshsomankar123-tech]

Title: Implement Project-Wide Multi-Step Undo/Redo System in Script Editor
Description:
Currently, the Script Editor only provides a single-step undo option. To improve usability and make editing smoother, this PR adds a redo button alongside undo, allowing users to move both backward and forward through their changes.
To support this, a centralized, session-scoped undo/redo manager has been introduced. It stores project states as XML snapshots (capped at 20) to handle more complex changes efficiently.
When a state is restored, the system also ensures that the user is automatically taken back to the correct Scene or Sprite, so the editing context remains intact.
Overall, this makes the editing experience more consistent and user-friendly, especially when working with multiple changes.

feat:
Snapshot Storage: Save state changes as XML files in undo_history/ using millisecond timestamps.

20-Step Limit: Strictly cap history at 20 steps. Auto-delete the oldest snapshot on the 21st push.

Session Management: Wipe undo_history/ clean upon entering SpriteActivity. Clear the Redo stack whenever a new state is pushed.

Contextual Restore: Undo/Redo actions must capture and restore the active sceneName and spriteName, automatically shifting UI focus to the correct location.

Dynamic UI: The action bar's Undo/Redo buttons must enable/disable based on history availability.

Async Reloading: Safely restore project states using ProjectLoader callbacks so the UI only refreshes after disk restoration is complete.

@wslany could you please take a look and let me know if any improvements are needed?
Thanks!

[wslany]

Hi @wslany i have done all 10 test please take a look ,

testUndoFormulaEditVariable is still failing with a ClassCastException in DataListFragment.

i see second test were fail becoz of the ClassCastException in DataListFragment.kt was causing testUndoFormulaEditVariable to fail.

Root Cause of the Failure: testUndoFormulaEditVariable was failing due to a ClassCastException in DataListFragment.kt. The showEditDialog method was implicitly treating item.value as a String because of the generic constraint UserData, but the UserVariable was initialized with an Integer value (5), causing the crash at runtime.

My approach: In showEditDialog, I replaced the implicit cast with a safer conversion:

(item as UserData<*>).value?.toString() ?: ""

This avoids unsafe casting and works correctly for all value types (Integer, Double, String, and null). In editItem, I’m parsing the input string back to Double so it stays consistent with how the Formula Editor stores variable values. Also, while looking into this, I noticed that testUndoFormulaRenameVariable manually calls userVariable.setName(VARIABLE_NAME) after undo. That made me think the undo logic might not be fully restoring the variable name on its own, which seems like a pre-existing issue.

I also improved the Espresso test stability by ensuring context menu clicks use inRoot(isPlatformPopup()) and adding closeSoftKeyboard() where necessary to prevent UI interference during the test run

Happy to dig into that separately and open another PR if that would help.

Let me know if this approach looks good . Thanks!

@harshsomankar123-tech Thanks for digging into this in so much detail.

I rechecked the latest PR head and reran the focused formula-editor undo tests locally. With your latest test fix, testUndoAfterActivityRecreationDoesNotCrash is now passing as well, so from my side the IDE-309 bugfix itself looks complete now.

Regarding testUndoFormulaEditVariable: thanks also for identifying the ClassCastException in DataListFragment. From your analysis, that indeed looks like a separate pre-existing issue in the variable edit flow rather than something caused by this PR. Your proposed fix with a safe toString() conversion sounds reasonable to me as a direction, but I would prefer to keep that as follow-up work in a separate PR / ticket so that this bugfix stays focused.

The same applies to the observation about testUndoFormulaRenameVariable manually restoring the variable name after undo. That also sounds worth investigating, but as a separate issue unless it turns out to be directly required for IDE-309.

So from my side, I don’t see further work required in this PR anymore. The remaining DataListFragment issue and the possible rename-undo gap would be good candidates for follow-up tickets/PRs.

I would again be grateful in case you could hep with the formulation of the ticket(s) for testUndoFormulaEditVariable and testUndoFormulaRenameVariable,

Thanks again, this was a very thorough fix.

[wslany]

Ah, just the missing line https://github.com/Catrobat/Catroid/security/code-scanning/2741

[wslany]

Title: Implement Project-Wide Multi-Step Undo/Redo System in Script Editor

[...]

@wslany could you please take a look and let me know if any improvements are needed? Thanks!

@harshsomankar123-tech I’ve been thinking a bit more about the desired UX, and I believe the history should ideally be project-scoped rather than only tied to one SpriteActivity session. In particular, I think it would be valuable if the history could survive temporary interruption, crash, or restart for a limited time window, and only be invalidated once changes are made in another project, or after management actions outside the editor (which should have their own history, in yet another ticket). I’m going to draft the first Jira ticket in that direction, and then we can refine the exact scope there together. I'll contact you via email to move the discussion away from this PR.

[wslany]

Looks good to me.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-advanced-security]

Checkstyle found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

[harshsomankar123-tech]

Hi @wslany @reichli , I refactored FormulaEditorUndoTest file to Kotlin. Now 10 tests are passing and only one is failing I’ll address that in PR #5177.

I also fixed a few SonarQube maintainability issues and improved some lifecycle handling:

• Persisted undo menu state across activity recreation
• Reduced complexity in checkVariables()
• Added safety checks for async callbacks in ScriptFragment
• Improved undo error recovery

Please Take a look Let me know if you’d like me to split anything further
Thank You so much!

[wslany]

@harshsomankar123-tech Thanks for the latest updates. I rechecked the current PR head and found two remaining points:

1. The recreation regression test still does not actually exercise undo after recreation. It creates an undoable change, recreates the activity, then makes a new formula edit and checks that the formula/undo button are visible. However, it never presses undo after recreation, so it does not prove that the original crash path is fixed or that undo restoration still works after activity recreation. Could you adjust the test so it clicks undo after recreation and asserts the restored formula/state, plus that the undo button is hidden afterwards?

2. ScriptFragment.java currently has a lot of indentation regressions in the touched blocks. It still compiles, but there are many warnings for this file, and the switch/try blocks are now much harder to read. For example, statements under case labels are aligned with the labels instead of being nested below them. Could you please clean up the formatting?

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[wslany]

On the current version I don’t have remaining review blockers for PR #5171: the recreation regression test now really performs the undo/assertions in FormulaEditorUndoTest.kt, and the indentation/readability issue in ScriptFragment.java is cleaned up.

I also tested the undo functionality on my phone, and it all looked good. Here's a video of my manual tests: https://photos.app.goo.gl/NTsyBfZt1JEDnZDn7

[reichli]

Looks good - thank you!