Add 'Load From SVG Resource' for bitmap property #817
Conversation
Signed-off-by: Vadym Hrynchyshyn <vadym.hrynchyshyn@qsc.com>
There was a problem hiding this comment.
Thanks for this addition. I am also only using C++ and know only very little about the other languages, but doing nothing for the other languages leads to the generation of invalid code, this is not acceptable. Currently, the code generators output a Null-Bitmap for unsupported or not implemented options to at least generate valid output. It would be great if you could do that for the other generators, otherwise i will do it myself later.
Besides this point, the PR is looking good to me, i have just some remarks regarding code style, maybe you can address these, please see the following review.
src/rad/inspector/wxfbadvprops.h
Outdated
| @@ -96,6 +96,7 @@ class wxFBBitmapProperty : public wxPGProperty | |||
| wxPGProperty* CreatePropertyFilePath(); | |||
| wxPGProperty* CreatePropertyResourceName(); | |||
| wxPGProperty* CreatePropertyIconSize(); | |||
| wxPGProperty* CreatePropertyDefSize(); | |||
There was a problem hiding this comment.
All other methods use the full name of the property instead of an abbrevation, so CreatePropertyDefaultSize() would fit the current naming scheme better.
| wxPGProperty* wxFBBitmapProperty::CreatePropertyDefSize() | ||
| { | ||
| // Create 'def_size' property ('Load From SVG Resource' only) | ||
| wxPGProperty* propDefSize = new wxFBSizeProperty(wxT("def_size"), wxPG_LABEL, {16, 16}); |
There was a problem hiding this comment.
Why is 16 used as default value? Pretty much in every other place of wxFB a defined default value like wxDefaultSize (-1, -1) is used, can't this be used here as well?
There was a problem hiding this comment.
wxDefaultSize(-1, -1) crashes the app. wxBitmapBundle::FromSVGResource expects real WxH because SVG doesn't have size, it will be rasterized to BMP with passed size. 16x16 is used for menu items, 24x24 for toolbars, etc.
src/rad/inspector/wxfbadvprops.cpp
Outdated
| @@ -502,7 +518,7 @@ wxVariant wxFBBitmapProperty::ChildChanged(wxVariant& thisValue, const int child | |||
| const auto count = GetChildCount(); | |||
|
|
|||
| // childValue.GetInteger() returns the chosen item index | |||
| switch (childValue.GetInteger()) { | |||
| switch (auto child_val = childValue.GetInteger()) { | |||
There was a problem hiding this comment.
Current code usually uses CamelCase, childVal would be more fitting.
src/rad/inspector/wxfbadvprops.cpp
Outdated
| auto prop = child_val == 3 ? bp->CreatePropertyIconSize() : bp->CreatePropertyDefSize(); | ||
| bp->AppendChild(prop); |
There was a problem hiding this comment.
There is no need to store the result of the ternary operator in a local variable, the result could be passed in-place to AppendChild()
|
Not part of this PR, but related: This is the source of assertions that occur when changing certain child values like |
Signed-off-by: Vadym Hrynchyshyn <vadym.hrynchyshyn@qsc.com>
|
Added wxNullBitmap generation for other languages, can't check that, but generated source files look OK. |
In this order all resource based options are grouped together.
Although both types have a common child, they have a different child for their size property, so a child update is necessary on type change.
Adds ability to create a bitmap property from SVG resource on Windows or Mac. wxBitmapBundle::FromSVGResource is used for that. The code generation is implemented for C++ only, I am not familiar with wxWidgets library for Python, Lua, etc.