Skip to content

[graf2d] Fix Setting window size in batch mode#21136

Open
rlalik wants to merge 1 commit into
root-project:masterfrom
rlalik:fix_canvas_set_window
Open

[graf2d] Fix Setting window size in batch mode#21136
rlalik wants to merge 1 commit into
root-project:masterfrom
rlalik:fix_canvas_set_window

Conversation

@rlalik
Copy link
Copy Markdown
Contributor

@rlalik rlalik commented Feb 3, 2026

The current implementation doesn't make sense at all? Was it some accidentally committed code?

@linev It was your commit? Was it mistake?

771c44692bcb graf2d/gpad/src/TCanvas.cxx (Sergey Linev      2023-07-06 16:42:42 +0200 2151)    if (fBatch && !IsWeb())
d023aecbdda4 graf2d/gpad/src/TCanvas.cxx (Sergey Linev      2020-05-04 19:06:00 +0200 2152)       SetCanvasSize((ww + fCw) / 2, (wh + fCh) / 2);
d023aecbdda4 graf2d/gpad/src/TCanvas.cxx (Sergey Linev      2020-05-04 19:06:00 +0200 2153)    else if (fCanvasImp)
d023aecbdda4 graf2d/gpad/src/TCanvas.cxx (Sergey Linev      2020-05-04 19:06:00 +0200 2154)       fCanvasImp->SetWindowSize(ww, wh);
  • tested changes locally
  • updated the docs (if necessary)

The current implementation doesn't make sense at all? Was it some accidentally commited code?
@rlalik rlalik requested a review from couet as a code owner February 3, 2026 17:00
@linev
Copy link
Copy Markdown
Member

linev commented Feb 3, 2026

@rlalik

No, this kind of calculation was there since a while. See:

https://github.com/root-project/root/blob/v6-24-00-patches/graf2d/gpad/src/TCanvas.cxx#L2201

I only add special handling for web case

@linev
Copy link
Copy Markdown
Member

linev commented Feb 3, 2026

I have no idea why code looks like this but I prefer not to touch it.
SetWindowSize has no real meaning for the batch - one should use SetCanvasSize instead.

@rlalik
Copy link
Copy Markdown
Contributor Author

rlalik commented Feb 3, 2026

Well, in the batch mode there is indeed no window but this code changes the canvas size. And I would say, for compatibility reasons, should work properly. Otherwise macro run in batch mode will produce wrongly sized canvases.

@ferdymercury
Copy link
Copy Markdown
Collaborator

related: #11004 and c3de92f

@couet couet removed their request for review March 27, 2026 16:36
@ferdymercury
Copy link
Copy Markdown
Collaborator

@linev It was your commit? Was it mistake?

It was this commit by @couet suggested by Wile in 2017

281c670
https://root-forum.cern.ch/t/tcanvas-setwindowsize-extension-proposal/26226

According to Wile, it's because:

TCanvas shows the typical usage “c->SetWindowSize(w + (w - c->GetWw()), h + (h - c->GetWh()));”, so we need to “recalculate” parameters’ values (in order to get the requested original “w” and “h”).

@rlalik
Copy link
Copy Markdown
Contributor Author

rlalik commented May 14, 2026

I do not understand the logic behind it. In the batch mode SetCanvasSize((ww + fCw) / 2, (wh + fCh) / 2); causes that the new canvas size has size of half window. I don't know how it explains the message in commit 281c670. Perhaps it worked with some other logic which was changed in the meantime?

Did Wile test his suggestion? Is there any trace in the issue or in the forum about that? Perhaps there was miscommunication and Wile's suggestion as implemented incorrectly?

@rlalik
Copy link
Copy Markdown
Contributor Author

rlalik commented May 14, 2026

Ok, now I indeed understand, because c->SetWindowSize(c->GetWindowWidth() + (c->GetWindowWidth() - c->GetWw()), c->GetWindowHeight() + (c->GetWindowHeight() - c->GetWh())); the Ww() is 0 so we set double of window width and we need to divide by half later.

But you see that this function will work properly only in this specific case, someone need to call it in this particular way, otherwise it will give wrong result. This is so wrong in my opinion. The logic is flaved here and should be handled different way.

@ferdymercury
Copy link
Copy Markdown
Collaborator

But you see that this function will work properly only in this specific case, someone need to call it in this particular way, otherwise it will give wrong result. This is so wrong in my opinion. The logic is flaved here and should be handled different way.

Yes, I agree all this is really messy.

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