Skip to content

Fix IDE 324 : ClassCastException in DataListFragment and stabilize UI tests#5177

Open
harshsomankar123-tech wants to merge 4 commits intoCatrobat:developfrom
harshsomankar123-tech:fix/formula-editor-undo-final
Open

Fix IDE 324 : ClassCastException in DataListFragment and stabilize UI tests#5177
harshsomankar123-tech wants to merge 4 commits intoCatrobat:developfrom
harshsomankar123-tech:fix/formula-editor-undo-final

Conversation

@harshsomankar123-tech
Copy link
Copy Markdown
Member

@harshsomankar123-tech harshsomankar123-tech commented Mar 29, 2026

JIRA TICKET - https://catrobat.atlassian.net/browse/IDE-324
Description:
This pull request addresses a ClassCastException in DataListFragment and resolves test regressions within the FormulaEditorUndoTest. Additionally, it stabilizes UI interactions for Espresso tests involving platform popups.

Note: This pull request is a continuation of (#5171)

Detailed Changes

1. Fixed ClassCastException in DataListFragment.kt

  • Issue: The variable editing logic in DataListFragment had a type-safety issue when trying to update user variables, which could lead to a ClassCastException or incorrect data types being saved.
  • Solution: Added an explicit check for UserVariable types in editItem. It now intelligently tries to parse the input value as a Double first (for numeric variables) and falls back to a String if it's not a number. This ensures the underlying data model remains consistent.

2. Resolved FormulaEditorUndoTest Regressions

  • Issue: The Espresso tests for Formula Editor undo were failing due to inconsistent UI states and strict equality checks on floating-point numbers.
  • Solution: Added closeSoftKeyboard() calls to prevent the soft keyboard from obstructing the view during automated tests. Updated numeric assertions to use a delta comparison (assertEquals(expected, actual, 0.001)), making the tests more robust against minor precision differences in floating-point math.

3. Stabilized UI Test Interactions

  • Issue: Espresso sometimes failed to find "Rename" or "Edit" buttons because they are located within platform popups (context menus).
  • Solution: Modified UserDataItemRVInteractionWrapper.java to explicitly search for these buttons within the platform popup root (isPlatformPopup()). This prevents common "No Matching View" errors in UI tests.

Files Modified:

  • DataListFragment.kt: Updated editField and editItem logic.
  • FormulaEditorUndoTest.java: Improved test stability and assertions.
  • UserDataItemRVInteractionWrapper.java: Fixed popup menu interaction logic.

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

@harshsomankar123-tech
Copy link
Copy Markdown
Member Author

@wslany Could you please add this PR issue to a Jira ticket? Apologies for raising the PR without linking one. If possible, may I create the ticket for this?
Thanks!

@wslany wslany self-assigned this Mar 29, 2026
Copy link
Copy Markdown
Member

@wslany wslany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wslany Could you please add this PR issue to a Jira ticket? Apologies for raising the PR without linking one. If possible, may I create the ticket for this? Thanks!

Thanks for the follow-up PR. Yes, I would be grateful if you could create the ticket for it. If you can't yet create it, let me set it up for you. I'll also look into how you can get write access to our jira.

I checked the changes and also ran the focused FormulaEditorUndoTest suite locally.

The good part first: the ClassCastException root cause analysis makes sense, and the new popup-menu test interactions (inRoot(isPlatformPopup())) are a nice stability improvement.

One thing I would still like to revisit in the production code, though, is that this seems to fix the symptom rather than the underlying type mismatch. DataListFragment is still declared as T : UserData, while UserVariable is actually UserData. The new UserData<*> cast in showEditDialog() and the UserVariable special case in editItem() avoid the current crash, but the model is still inconsistent. I think it would be safer to widen the fragment’s generic type / value handling more explicitly instead of relying on local casts.

On the tests: when I ran FormulaEditorUndoTest on this PR branch by itself, most of the suite was still red because of the already-known undo-button visibility issue on current develop. That looks expected to me, since this follow-up branch is based on develop and does not include the production undo fix from #5171. So I would evaluate this PR mainly as a follow-up to #5171, not as a standalone green fix on current develop.

So from my side:

  • the test helper changes look good
  • the crash fix direction is reasonable
  • but I would prefer one more refinement on the DataListFragment typing rather than keeping the current cast-based workaround as-is

@harshsomankar123-tech
Copy link
Copy Markdown
Member Author

Thanks for the follow-up PR. Yes, I would be grateful if you could create the ticket for it. If you can't yet create it, let me set it up for you. I'll also look into how you can get write access to our jira.

Yes, thank you! I’d appreciate that.

@harshsomankar123-tech
Copy link
Copy Markdown
Member Author

harshsomankar123-tech commented Mar 29, 2026

One thing I would still like to revisit in the production code, though, is that this seems to fix the symptom rather than the underlying type mismatch. DataListFragment is still declared as T : UserData, while UserVariable is actually UserData. The new UserData<*> cast in showEditDialog() and the UserVariable special case in editItem() avoid the current crash, but the model is still inconsistent. I think it would be safer to widen the fragment’s generic type / value handling more explicitly instead of relying on local casts.

@wslany Thanks for the detailed feedback!

I agree that the current approach relies on a cast-based workaround. I’ll take another pass to improve the type handling in DataListFragment and make the solution more robust and type-safe.
I think we can continue this work after #5171 is merged, so we can ensure all tests are passing locally.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

@harshsomankar123-tech harshsomankar123-tech changed the title Fix ClassCastException in DataListFragment and stabilize UI tests IDE 324Fix : ClassCastException in DataListFragment and stabilize UI tests Mar 30, 2026
@harshsomankar123-tech harshsomankar123-tech changed the title IDE 324Fix : ClassCastException in DataListFragment and stabilize UI tests Fix IDE 324 : ClassCastException in DataListFragment and stabilize UI tests Mar 30, 2026
@harshsomankar123-tech harshsomankar123-tech added the Active Member Tickets that are assigned to members that are still currently active label Apr 2, 2026
@wslany wslany requested a review from Copilot April 2, 2026 13:05
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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


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

@harshsomankar123-tech harshsomankar123-tech force-pushed the fix/formula-editor-undo-final branch from 17a6f17 to e14f088 Compare April 16, 2026 17:35
@harshsomankar123-tech
Copy link
Copy Markdown
Member Author

harshsomankar123-tech commented Apr 16, 2026

@wslany @reichli , please take a look When u have Time? All 11 tests are now passing.

@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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


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

Comment on lines +338 to +342
assertEquals(
(ProjectManager.getInstance().currentProject.getUserVariable(VARIABLE_NAME).value as Number).toDouble(),
NEW_VARIABLE_VALUE.toDouble(),
0.001
)
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertEquals(expected, actual, delta) argument order is reversed here: the current-project value is passed as the expected parameter and the constant as the actual parameter. This won’t usually fail the test, but it makes assertion failure messages misleading. Swap the first two arguments so NEW_VARIABLE_VALUE.toDouble()/VARIABLE_VALUE.toDouble() is the expected value and the project value is the actual.

Copilot uses AI. Check for mistakes.
Comment on lines +350 to +354
assertEquals(
(ProjectManager.getInstance().currentProject.getUserVariable(VARIABLE_NAME).value as Number).toDouble(),
VARIABLE_VALUE.toDouble(),
0.001
)
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: assertEquals(expected, actual, delta) has expected/actual swapped, which makes failures harder to interpret. Pass VARIABLE_VALUE.toDouble() as expected and the project value as actual.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Active Member Tickets that are assigned to members that are still currently active

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants