Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

broadcast callback on non shared variables #1441

Merged
merged 24 commits into from
Jul 1, 2024

Conversation

FredLL-Avaiga
Copy link
Member

@FredLL-Avaiga FredLL-Avaiga commented Jun 21, 2024

resolves #1207

renamed current broadcast_callback to broadcast_callback_on_shared

Copy link
Contributor

github-actions bot commented Jun 21, 2024

Coverage report for frontend/taipy-gui

Branches coverage not met for global: expected >=80%, but got 62.079943402900604%

St.
Category Percentage Covered / Total
🟢 Statements
80.86% (+0.14% 🔼)
2776/3433
🟡 Branches
62.08% (-0.55% 🔻)
1755/2827
🟡 Functions
74.12% (-0.19% 🔻)
487/657
🟢 Lines
81.49% (+0.14% 🔼)
2585/3172
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / Dialog.tsx
80.43% 60.87% 100% 80.43%
🟡
... / TaipyRendered.tsx
64.58% 14.29% 50% 64.58%
🟡
... / index.ts
75.51% 26.67% 50% 75%
🟢
... / Expandable.tsx
100% 85% 100% 100%
🟢
... / PageContent.tsx
75% 100% 0% 100%
🟢
... / Pane.tsx
95.12% 73.81% 100% 94.87%
🟢
... / Part.tsx
88% 64.29% 75% 91.3%
🟡
... / Unregistered.tsx
62.5% 0% 0% 60%

Test suite run success

400 tests passing in 41 suites.

Report generated by 🧪jest coverage report action from 824c9fa

@FabienLelaquais
Copy link
Member

I think this PR is also trying to fix something that was a glitch.

@dinhlongviolin1 I need your blessing here.
This broadcast_callback_on_shared() to me is useless: the 'shared' state is not known by users, and should actually never been exposed.
What I expect broadcast_callback() to do is invoke a user-defined function, potentially with parameters, for every live state.
If then the user invokes state.assign() fine, so be it. But let the user decide.
What do you think?

Another important question is: does anyone see a use case for the _on_shared version?
Because I don't see any, and I'm willing to drop all this code...

Thanks!

@dinhlongviolin1
Copy link
Member

I think this PR is also trying to fix something that was a glitch.

@dinhlongviolin1 I need your blessing here. This broadcast_callback_on_shared() to me is useless: the 'shared' state is not known by users, and should actually never been exposed. What I expect broadcast_callback() to do is invoke a user-defined function, potentially with parameters, for every live state. If then the user invokes state.assign() fine, so be it. But let the user decide. What do you think?

Another important question is: does anyone see a use case for the _on_shared version? Because I don't see any, and I'm willing to drop all this code...

Thanks!

I think you are right on this one and I agree. We should drop this code as I myself don't see a use case for it. Don't remember why I add it but hopefully it wont affect anything. @FredLL-Avaiga Please help me drop it in this PR 😺

@FredLL-Avaiga
Copy link
Member Author

I think this PR is also trying to fix something that was a glitch.

@dinhlongviolin1 I need your blessing here. This broadcast_callback_on_shared() to me is useless: the 'shared' state is not known by users, and should actually never been exposed. What I expect broadcast_callback() to do is invoke a user-defined function, potentially with parameters, for every live state. If then the user invokes state.assign() fine, so be it. But let the user decide. What do you think?

Another important question is: does anyone see a use case for the _on_shared version? Because I don't see any, and I'm willing to drop all this code...

Thanks!

I think you are right on this one and I agree. We should drop this code as I myself don't see a use case for it. Don't remember why I add it but hopefully it wont affect anything. @FredLL-Avaiga Please help me drop it in this PR 😺

I will drop it then

Fred Lefévère-Laoide and others added 3 commits June 24, 2024 09:32
Copy link
Member

@FabienLelaquais FabienLelaquais left a comment

Choose a reason for hiding this comment

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

I would like to be closer to the legacy example.

doc/gui/examples/broadcast.py Outdated Show resolved Hide resolved
doc/gui/examples/broadcast.py Outdated Show resolved Hide resolved
doc/gui/examples/broadcast.py Outdated Show resolved Hide resolved
doc/gui/examples/broadcast.py Outdated Show resolved Hide resolved
frontend/taipy-gui/src/components/Taipy/PaginatedTable.tsx Outdated Show resolved Hide resolved
taipy/gui/gui.py Show resolved Hide resolved
doc/gui/examples/broadcast.py Outdated Show resolved Hide resolved
doc/gui/examples/broadcast.py Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Jun 24, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
18463 16079 87% 0% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
taipy/gui/gui.py 81% 🟢
taipy/gui/gui_actions.py 69% 🟢
taipy/gui_core/_context.py 55% 🟢
TOTAL 68% 🟢

updated for commit: 824c9fa by action🐍

@FredLL-Avaiga
Copy link
Member Author

can't approve as I am still an assignee but I would.
@FabienLelaquais I thought you wanted to add a Gui.broadcast(var_name: str, value: t.Any) that would call Gui.broadcast_callback ?

@FredLL-Avaiga
Copy link
Member Author

I would if you update the branch ;-)

@FabienLelaquais
Copy link
Member

I would if you update the branch ;-)

Let me confess I completely forgot.
Thanks!

Copy link
Contributor

Coverage report for ./frontend/taipy-gui

Caution

Coverage does not meet threshold
Branches coverage not met for global: expected >=80%, but got 62.079943402900604%

St.
Category Percentage Covered / Total
🟢 Statements
80.86% (+0.42% 🔼)
2776/3433
🟡 Branches
62.08% (-0.31% 🔻)
1755/2827
🟡 Functions
74.12% (+0.16% 🔼)
487/657
🟢 Lines
81.49% (+0.46% 🔼)
2585/3172
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / Dialog.tsx
80.43% 60.87% 100% 80.43%
🟡
... / TaipyRendered.tsx
64.58% 14.29% 50% 64.58%
🟡
... / index.ts
75.51% 26.67% 50% 75%
🟢
... / Expandable.tsx
100% 85% 100% 100%
🟢
... / PageContent.tsx
75% 100% 0% 100%
🟢
... / Pane.tsx
95.12% 73.81% 100% 94.87%
🟢
... / Part.tsx
88% 64.29% 75% 91.3%
🟡
... / Unregistered.tsx
62.5% 0% 0% 60%

Test suite run success

400 tests passing in 41 suites.

Report generated by 🧪jest coverage report action from fe16d5c

@FabienLelaquais FabienLelaquais requested review from FabienLelaquais and removed request for FabienLelaquais June 30, 2024 17:37
Copy link
Member Author

@FredLL-Avaiga FredLL-Avaiga left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@FabienLelaquais FabienLelaquais left a comment

Choose a reason for hiding this comment

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

Looking good!

@FabienLelaquais FabienLelaquais self-requested a review June 30, 2024 20:17
Copy link
Member

@FabienLelaquais FabienLelaquais left a comment

Choose a reason for hiding this comment

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

namnguyen20999
namnguyen20999 previously approved these changes Jul 1, 2024
Copy link
Member

@namnguyen20999 namnguyen20999 left a comment

Choose a reason for hiding this comment

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

LGTM

dinhlongviolin1
dinhlongviolin1 previously approved these changes Jul 1, 2024
Copy link
Member

@dinhlongviolin1 dinhlongviolin1 left a comment

Choose a reason for hiding this comment

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

Amazingly done!

Copy link
Member

@FabienLelaquais FabienLelaquais left a comment

Choose a reason for hiding this comment

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

👍

@FredLL-Avaiga FredLL-Avaiga merged commit c9a4729 into develop Jul 1, 2024
152 of 154 checks passed
@FredLL-Avaiga FredLL-Avaiga deleted the feature/#1207-broadcast-callback branch July 1, 2024 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gui: Back-End ✨New feature 🟥 Priority: Critical Must be addressed as soon as possible 📝Release Notes Impacts the Release Notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possibility to forward a change to a subset of clients (conditional broadcast)
4 participants