Skip to content

Feature removable serie#39

Merged
lcallarec merged 14 commits intolcallarec:masterfrom
Robert-Ordis:feature-removable-serie
Dec 8, 2022
Merged

Feature removable serie#39
lcallarec merged 14 commits intolcallarec:masterfrom
Robert-Ordis:feature-removable-serie

Conversation

@Robert-Ordis
Copy link
Contributor

This has changes about removing serie/chart.

  • Serie: Removable from Chart.
  • Prevent to memory leak on removing.

But, stopping automatic-rendering is not able while removing from parent automatically.
So, I add how to remove LiveChart.Chart from Widget as "CAUTION" section.

 - Causes memory leak after removing LiveChart.Chart from Widget.
 - Test for function Serie.remove_serie(serie)
 - Proof of disconnecting serie.value_add: Check if bounds is updated or not.
 - Basically same as /LiveChart/Series/remove_serie#disconnect_serie.value_add
 - NOTE: Stopping auto-refresh is necessary to invoke destroy/Destructor.
@stsdc
Copy link
Contributor

stsdc commented Dec 7, 2022

This looks cool!

assert(chart.config.y_axis.get_bounds().upper == 200.0);
try{
series.get_by_name("Test 1");
assert(false);
Copy link
Owner

Choose a reason for hiding this comment

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

Not blocking at all : another option would be to use assert_not_reached(), technically equivalent, but IMHO, it's sementically a bit better :)

@lcallarec
Copy link
Owner

@Robert-Ordis : thanks for this one ! Outside my comments, looks good to me !

 - register(serie): Use Series.series.contains to check if the serie is already registered.
@Robert-Ordis
Copy link
Contributor Author

Thank you for your revieweing.
Is there any other pointing ?

If not, then, I'll go to prepare the next PR.

@lcallarec lcallarec merged commit 142f0b8 into lcallarec:master Dec 8, 2022
@lcallarec
Copy link
Owner

Good to merge ! Thanks !

@Robert-Ordis Robert-Ordis deleted the feature-removable-serie branch December 9, 2022 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants