Opened 3 years ago

Closed 10 months ago

#13578 closed defect (fixed)

wxToolBar doesn't recompute button size when switching icons off

Reported by: catalin Owned by:
Priority: low Milestone:
Component: wxMSW Version: dev-latest
Keywords: wxToolBar toolbar bitmap Cc:
Blocked By: Blocking:
Patch: yes

Description

This is a problem when initially the tools show both images and text, then the style is changed to only show the text.
Or, in the sample if the toolbar is switched to vertical, when recreated it no longer adds the combo box among the tools but it considers the combo box' size.

In the toolbar sample this is easier to see for vertical toolbars

Attachments (3)

toolbar-vert-text.png download (20.1 KB) - added by awi 10 months ago.
Vertical toolbar with text items.
toolbar.png download (22.9 KB) - added by awi 10 months ago.
Vertical toolbar with text only.
toolbar-bitmap-size.patch download (2.6 KB) - added by awi 10 months ago.
Patch to fix bitmap size calculation.

Download all attachments as: .zip

Change History (20)

Changed 10 months ago by awi

Vertical toolbar with text items.

comment:1 Changed 10 months ago by awi

  • Keywords wxToolBar toolbar added

The issue seems to fixed both in 2.8.12 and 3.0 (see attached screenshot).

Note: wxToolBar::Recreate() seems to be no longer in use.

comment:2 Changed 10 months ago by vadz

  • Resolution set to fixed
  • Status changed from new to closed

comment:3 follow-up: Changed 10 months ago by catalin

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version changed from stable-latest to dev-latest

This is not fixed. Even the attached image clearly shows that the buttons in the vertical toolbar are too tall because they still account for the size of the image.

Again, there are more ways to reproduce it:
a) start the toolbar sample, switch to text only buttons (Ctrl+Alt+T).
b) start the toolbar sample, set toolbar at the left of the window, switch to text only buttons (Ctrl+Alt+T).

When text only buttons are chosen Recreate() is called, putting a breakpoint there helps.
It does something like this:

    // ...
    const wxSize size = GetSize();

    // ...

    if ( !MSWCreateToolbar(pos, size) )
    // ...

I believe the size should be recalculated there, not reuse the old one because that includes the space for images.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 10 months ago by awi

Replying to catalin:

This is not fixed. Even the attached image clearly shows that the buttons in the vertical toolbar are too tall because they still account for the size of the image.

Again, there are more ways to reproduce it:
a) start the toolbar sample, switch to text only buttons (Ctrl+Alt+T).
b) start the toolbar sample, set toolbar at the left of the window, switch to text only buttons (Ctrl+Alt+T).

When text only buttons are chosen Recreate() is called, putting a breakpoint there helps.
It does something like this:

    // ...
    const wxSize size = GetSize();

    // ...

    if ( !MSWCreateToolbar(pos, size) )
    // ...

I believe the size should be recalculated there, not reuse the old one because that includes the space for images.

So, the problem is not with toolbar item widths (where combox box size could be a cause) but with their heights?
I think that in v2.8.12 it looks good, so maybe it is a regression in v3.0.

comment:5 in reply to: ↑ 4 Changed 10 months ago by catalin

So, the problem is not with toolbar item widths (where combox box size could be a cause) but with their heights?

I don't know, or at least I don't remember. Now that you mention it, I think the combo width was giving the width of the vertical toolbar, which was wrong.
But still, the size of the buttons without images doesn't look good. Maybe it has a different reason indeed.

I think that in v2.8.12 it looks good

I wonder about that. In v2.8.12 the drop-down arrow is missing from "New" button, could it be due to the button being clipped or just because that kind of button did not exist yet?

Changed 10 months ago by awi

Vertical toolbar with text only.

comment:6 Changed 10 months ago by awi

  • Keywords bitmap added
  • Patch set

OK, it looks that bitmaps are taken into account when the size of toolbar items is calculated even if toolbar has wxTB_NOICONS style.
Attached patch fixes this issue.
Results on attached screenshot.

P.S. Indeed, in v2.8.12 toolbar sample there is no drop-down list linked to the "New" button. Perhaps it's a new feature introduced in v3.0.

comment:7 Changed 10 months ago by vadz

Sorry, I didn't follow this bug really closely. @catalin, could you please confirm that the latest patch (thanks @awi!) does fix the problem you reported?

comment:8 follow-up: Changed 10 months ago by catalin

The latest patch did not fix the problem for me. I've just updated trunk sources and applied it. I always get the same result as in toolbar.png no patch.
wxToolBar::SetToolBitmapSize() which it modifies is not called when toggling button images off. Maybe it is incomplete?
OTOH, if only images are shown on buttons they are sized correctly and the function is still not called.
Tried on Win7 x64, VS2008.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 10 months ago by awi

Replying to catalin:

The latest patch did not fix the problem for me. I've just updated trunk sources and applied it. I always get the same result as in toolbar.png no patch.
wxToolBar::SetToolBitmapSize() which it modifies is not called when toggling button images off. Maybe it is incomplete?
OTOH, if only images are shown on buttons they are sized correctly and the function is still not called.
Tried on Win7 x64, VS2008.

In the toolbar sample SetToolBitmapSize is executed e.g. when toolbar is switched to vertical layout (via OnChangeOrientation -> RecreateToolbar -> PopulateToolbar).
Did you check switching to vertical toolbar?

comment:10 in reply to: ↑ 9 ; follow-up: Changed 10 months ago by catalin

Replying to awi:

Did you check switching to vertical toolbar?

Yes, in the order mentioned in a comment above:
b) start the toolbar sample, set toolbar at the left of the window, switch to text only buttons (Ctrl+Alt+T).

comment:11 in reply to: ↑ 10 Changed 10 months ago by awi

Replying to catalin:

Replying to awi:

Did you check switching to vertical toolbar?

Yes, in the order mentioned in a comment above:
b) start the toolbar sample, set toolbar at the left of the window, switch to text only buttons (Ctrl+Alt+T).

I see - the order of operations matters. If you first toggle to text only mode and next switch to vertical layout then everything is OK.
This is because when the toolbar is toggled to text only it is not recreated/repopulated by the application - only it's internal flag is set.
I don't know if this could be considered as a problem with wxToolbar itself or with toolbar sample only. If toolbar would be recreated when text mode changes (as it is done when toolbar is changed to vertical/horizontal) then its size would be OK.

comment:12 follow-up: Changed 10 months ago by vadz

  • Priority changed from normal to low
  • Summary changed from wxToolBar::Recreate() never calculates new sizes to wxToolBar doesn't recompute button size when switching icons off

I guess it's a bug in wxToolbar but a low priority one as there is a simple workaround and, also, vertical toolbars with just text are just not very common at all.

Do I understand correctly that toolbar-bitmap-size.patch should still be applied as it does help, even if it doesn't fix the last problem?

comment:13 in reply to: ↑ 12 Changed 10 months ago by catalin

Replying to vadz:

Do I understand correctly that toolbar-bitmap-size.patch should still be applied as it does help, even if it doesn't fix the last problem?

Well, I don't think it should be applied, it still looks incomplete to me in either cases.
With the patch applied after starting the sample (the toolbar is by default with both text and icons and at the top of the window) I see the following cases:

Case A:
1) switch the icons off - the buttons keep the original (incorrect) size;
2) move the toolbar to the left side - the buttons are correctly sized;
3) move the toolbar to the top - the buttons are correctly sized;

Case B:
1) move the toolbar to the left side;
2) switch the icons off - the buttons keep the original (incorrect) size;
3) move the toolbar to the top - the buttons are correctly sized;
4) move the toolbar to the left size - the buttons are correctly sized;

When the text only buttons are correctly sized, toggling images back on and then off will leave the buttons with wrong sizes again.
So there is some effect after applying the patch, but not when toggling off the images (no matter where the toolbar is positioned).

Changed 10 months ago by awi

Patch to fix bitmap size calculation.

comment:14 follow-up: Changed 10 months ago by awi

New patch should fix the problem with images/texts of the buttons.
It also handles of the situation when toolbar has not only buttons but also controls with labels.
If toolbar has "one-line" buttons (only with text or image) then its height is small and there is no room to display both controls and its labels. In this case the labels should be rather disabled.

comment:15 in reply to: ↑ 14 Changed 10 months ago by catalin

Replying to awi:

New patch should fix the problem with images/texts of the buttons.

Yes, looks good here as well.

comment:16 Changed 10 months ago by vadz

Great, thanks @awi! Will apply soon.

comment:17 Changed 10 months ago by VZ

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [75846]) Fix wxToolBar size in wxMSW when not using icons.

There were several problems when the toolbar style was toggled to not show
icons, fix them by adding missing checks for wxTB_NOICONS style.

Closes #13578.

Note: See TracTickets for help on using tickets.