wxRibbonArtProvider::GetButtonBarButtonSize not called (Issue #25687)
Gallicrow
Gallicrow created an issue (wxWidgets/wxWidgets#25687)Bug description:
wxRibbonArtProvider::GetButtonBarButtonSize doesn't seem to get called in wxWidgets 3.3.1
Expected vs observed behaviour:
In my program (Windows and Linux) I use wxRibbonBars quite a lot and I prefer the buttons to be the same width, otherwise they can look a bit squashed.
To achieve this I derived a class from wxRibbonMSWArtProvider (or wxRibbonAUIArtProvider on Linux) and overrode the GetButtonBarButtonSize method, setting the widths of button_size and normal_region to be a specified value in pixels.
Unfortunately when I recently tried using version 3.3.1, this failed to work and when I put a breakpoint in GetButtonBarButtonSize I found it was not being called.
Everything works fine until 3.2.8 and I haven't tried any versions between that one and 3.3.1
Platform and version information
- wxWidgets version you use: 3.3.1
- wxWidgets port you use: wxMSW
- OS and its version: Windows 11
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
PBfordev left a comment (wxWidgets/wxWidgets#25687)Did you change the parameter type from wxDC to wxReadOnlyDC in the overriden method?
But perhaps that should get caught by the compiler when using the override attribute (assuming you used it)...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Gallicrow
Gallicrow left a comment (wxWidgets/wxWidgets#25687)Fantastic! Thanks very much @PBfordev I made that change and everything worked perfectly. The original code did not include the override specifier but I've modified the other places where wxWidgets methods were overridden to include it so that this sort of mistake is less likely to happen again.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
PBfordev left a comment (wxWidgets/wxWidgets#25687)IMO, this is a bit tricky. Even if you read the changelog carefully, you may not get all the implications (i.e., remember where a wxDC is used in an overriden method).
The only protection is using the override specifier but the code may have be written before switching to modern C++.
On one hand, I don't know if this could have been handled better at wxWidgets side, nothing comes to me at the moment.
On the other hand, if it were on me, I would list all the affected classed in that changelog item. Better safe than sorry.
On the third(?) hand, I am dumb so I prefer to play very safe, so it may be just me...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Gallicrow
Gallicrow left a comment (wxWidgets/wxWidgets#25687)I suppose private methods with the old signatures (with wxDC) could have been put in the headers of all methods which had been changed to use wxReadOnlyDC. A bit ugly though.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
PBfordev left a comment (wxWidgets/wxWidgets#25687)But how would that help with this issue, i.e., the method with a new signature being called by wxWidgets?
Would this not actually hide the problem since this would compile even with override but still would not be called?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Gallicrow
Gallicrow left a comment (wxWidgets/wxWidgets#25687)OK. How about using final then?
e.g.
`// wxWidgets library
class wxReadOnlyDC
{
};
class wxDC : public wxReadOnlyDC
{
};
class wxRibbonArtProvider
{
public:
// New method which uses wxReadOnlyDC rather than wxDC.
virtual void method(const wxReadOnlyDC& dc);
// Prevent derived classes overriding method with wxDC argument.
virtual void method(const wxDC& dc) final;
};
// Application using wxWidgets
class CustomRibbonArtProvider : public wxRibbonArtProvider
{
public:
void method(const wxDC& dc);
};
`
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
PBfordev left a comment (wxWidgets/wxWidgets#25687)OK. How about using final then?
Well, I guess that may work and as much people hate having their builds broken, it may be better than their overriden methods not being called anymore.
However, I am not sure if wxWidgets devs are willing to go out of their way this much...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25687)Using final looks like a good idea! I thought that this would result in getting warnings about hiding the overloaded virtual function, but it doesn't seem to happen with any of gcc/clang/MSVC.
So now we "just" need to add private final overloads of all functions taking wxReadOnlyDC...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25687)I don't know how did I test this the last time, but now that I've tried to actually put this in place, it does result in tons of -Woverloaded-virtual (g++ 14.2), so I don't think we can do this. Unless we only do it for MSVC, but this doesn't sound very appealing...
So finally I'm afraid I'm just going to close this because I don't see what else to do.
For the record, here is the minimal patch I tried:
diff --git a/include/wx/aui/auibar.h b/include/wx/aui/auibar.h index d317d5b969..b151f47544 100644 --- a/include/wx/aui/auibar.h +++ b/include/wx/aui/auibar.h @@ -350,6 +350,25 @@ public: // Provide opportunity for subclasses to recalculate colours virtual void UpdateColoursFromSystem() {} + + // These functions are not called and must not be overridden any more, they + // are intentionally final in order to result in errors if you do it: + // override the overloads taking wxReadOnlyDC instead. + virtual wxSize GetLabelSize( + wxDC& dc, + wxWindow* wnd, + const wxAuiToolBarItem& item) final + { + return GetLabelSize(static_cast<wxReadOnlyDC&>(dc), wnd, item); + } + + virtual wxSize GetToolSize( + wxDC& dc, + wxWindow* wnd, + const wxAuiToolBarItem& item) final + { + return GetToolSize(static_cast<wxReadOnlyDC&>(dc), wnd, item); + } };
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.