Update card collapsed state handling to change child view model visibility attribute

[dalthviz]

For an initial description of the approach followed for the proposed change you can check #8198 (comment)

Fixes #8198

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.93%. Comparing base (d93db73) to head (6abc4ea).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8274      +/-   ##
==========================================
- Coverage   86.01%   85.93%   -0.08%     
==========================================
  Files         349      349              
  Lines       54647    54660      +13     
==========================================
- Hits        47004    46972      -32     
- Misses       7643     7688      +45     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[philippjfr]

Hmm, I guess this is still problematic, because once visible=False it won't be restored to true subsequently.

[dalthviz]

Maybe setting the visibility here to false should only be done once using some flag variable like initially_colapsed? So initializing at some point:

this.initially_collapsed = this.model.collapsed

and then changing these lines to something like:

     if (this.initially_collapsed) {
        child_view.model.visible = false
        this.initially_collapsed = false
      }

With that seems to me that the only logic that would change the model visible attribute after that initial collapsed setup is inside the _collapse function that gets triggered when the model collapsed property changes

[dalthviz]

Note: Closing and reopening to check if a CI rerun helps with the failing test_server_thread_pool_change_event[tornado] test

[philippjfr]

That one is flaky and unrelated.

[dalthviz]

Yep, in my last commit I added a flaky decorator to that test and CI now is green. Let me know if there is something else that should be done here or if that last change I did should be reverted/moved to a different PR!

[dalthviz]

Note: Closing and reopening to retrigger CI

[dalthviz]

Seems like the assert here should be to_be_visible since visible=True?

Suggested change

	w1.visible = True
	expect(text_input).not_to_be_visible()
	w1.visible = True
	expect(text_input).to_be_visible()

[philippjfr]

No it's correct, since collapsed=True.

[philippjfr]

Meant to comment, the test was meant to highlight that the solution here is not sufficient.

[dalthviz]

I think the new changes added (7efff7a) should take care of that case (the new test passes with them).

Also, while doing the changes mentioned above, noticed that another test was failing (panel/tests/ui/widgets/test_texteditor.py::test_texteditor_regression_copy_paste[chromium]). It seems unrelated to the changes here so I tried adding a flaky decorator to it.

Let me know if something else is needed or if there is a better approach to take care of the case the new test illustrates. Let me know too if I should remove the flaky decorators that I added to some tests (panel/tests/ui/widgets/test_texteditor.py::test_texteditor_regression_copy_paste and panel/tests/test_server.py::test_server_thread_pool_change_event)

[philippjfr]

Sorry for the long back and forth on this. I suspect this needs to be changed though, right now it'll add a new callback every time the component is rendered and never removed which means that you end up accumulating a bunch. Will have to dig in to figure out a good way to handle this.

[dalthviz]

No problem and thank you for all your feedback here! Thinking about a way to overcome that, maybe adding the logic above + some logic to disconnect remove old children related callbacks (if available) inside update_children could work ? 🤔 Anyhow, let me know if you find a better way to handle things!

[dalthviz]

Just to be sure, would it make sense for me to try the idea above (moving callback connections to the update_children) here or would be preferable to leave this PR as it is for the moment? Let me know!