Skip to content

[gui] Remove unnecessary AddReference()#22043

Closed
bellenot wants to merge 1 commit intoroot-project:masterfrom
bellenot:fix-pictures-refcount
Closed

[gui] Remove unnecessary AddReference()#22043
bellenot wants to merge 1 commit intoroot-project:masterfrom
bellenot:fix-pictures-refcount

Conversation

@bellenot
Copy link
Copy Markdown
Member

AddReference() is already called in TGPicturePool::GetPicture()

`AddReference()` is already called in `TGPicturePool::GetPicture()`
@bellenot bellenot requested a review from linev April 24, 2026 10:43
@bellenot bellenot self-assigned this Apr 24, 2026
@linev
Copy link
Copy Markdown
Member

linev commented Apr 24, 2026

Fix can be dangerous if we have something like:

auto pic1 = fClient->GetPicture("checked_dis_t.xpm");
auto pic2 = fClient->GetPicture("unchecked_dis_t.xpm");

item1->SetCheckBoxPictures(pic1, pic2);
item2->SetCheckBoxPictures(pic1, pic2);
item3->SetCheckBoxPictures(pic1, pic2);

I guess, original implementation was done for such usecase.
There is no such code in ROOT itself, but probably in users code.

Copy link
Copy Markdown
Member

@linev linev left a comment

Choose a reason for hiding this comment

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

It is potentially dangerous change

@bellenot
Copy link
Copy Markdown
Member Author

@linev you're right, and there are no such use cases in ROOT, but since there is no way to know how the pictures passed as argument were created (without changing the API), I'll let the code as it is now...

@bellenot bellenot closed this Apr 24, 2026
@linev
Copy link
Copy Markdown
Member

linev commented Apr 24, 2026

Another solution - change caller code in ROOT.

auto pic1 = fClient->GetPicture("checked_dis_t.xpm");
auto pic2 = fClient->GetPicture("unchecked_dis_t.xpm");
parent->SetCheckBoxPictures(pic1, pic2);
fClient->FreePicture(pic1);
fClient->FreePicture(pic2);

Not elegant, but correctly handle references

@bellenot
Copy link
Copy Markdown
Member Author

@linev OK, will implement this. And I see there are already a couple of cases where it's already like this.

@bellenot
Copy link
Copy Markdown
Member Author

@linev superseded by #22046

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.

2 participants