Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for wxBitmapToggleButton #547

Merged
merged 2 commits into from Nov 1, 2019

Conversation

@jmoraleda
Copy link
Contributor

jmoraleda commented Oct 3, 2019

Added support for wxBitmapToggleButton (https://docs.wxwidgets.org/trunk/classwx_bitmap_toggle_button.html) in the Additional tab of the Component Palette, next to the wxToggleButton component.

I added support for all languages that wxFormBuilder can generate templates for.

@sodevel

This comment has been minimized.

Copy link
Member

sodevel commented Oct 4, 2019

I am quite unsure what to do about this. Isn't the wxBitmapToggleButton superfluous like the wxBitmapButton? Since the wxButton rework of wxWidgets all buttons can display bitmaps, maybe the wxToggleButton and wxBitmapToggleButton implementation should be updated like the wxButton and wxBitmapButton one was before. These two buttons even share all properties of a regular wxButton minus the default ability and auth ability (which is currently missing in wxFB), i don't know if its worth to extract these base properties into a base class and use the inherits feature to use this in all four button types.

The second issue is the code style of the plugin code. I know this is just a copy-paste of the wxToggleButton code, but this code doesn't adhere to the current coding style and usually i don't accept this for new code. I might add a code review about these details later but i'm currently short on time. I think it's worth to think about the button refactoring above or at least use the inherits feature between both toggle buttons.

@jmoraleda

This comment has been minimized.

Copy link
Contributor Author

jmoraleda commented Oct 8, 2019

Hello @sodevel. Thank you for your comprehensive reply. I very much agree that removing code duplication is very desirable. A common base class for all four button types sounds like a good idea. It would make sense to refactor all four buttons at once.

I am happy to help if it would be useful, but I don't know the form builder plugin framework (as you realized I just copy pasted from toggle button taking the bitmap info from bitmap button). In particular, does inheritance work only in the cpp code, or also in the xml?

If you can point me to an example of multiple similar components sharing a base class that adheres to the current code style guidelines, I could give the refactoring a try.

@sodevel

This comment has been minimized.

Copy link
Member

sodevel commented Oct 10, 2019

I don't know all the details about how that inheritance feature works, it's been a while i worked on the code, but maybe i can give a small overview.

Actually it is reversed, there is no inheritance used on the c++ side, the plugin code, allmost every component is implemented independent.

On the XML side however it is different, there exists the inherits tag which pulls in all definitions from the inherited element. You can see the effect in the object properties grid on the right side, these inherited definitions end up in subgroups. It is possible to override inherited properties, most prominent example is the name.

This also affects the code templates, all inherited code templates get applied automatically. It is possible to override these as well but this has different effects which i don't know in detail and discovered more or less by try and error. For example the construction template totally overrides the parent one but the settings templates get merged.

As an example of how to use inheritance take a look at wxButton and wxBitmapButton, about code style i currently have no better advice as to look at the commits and take a look at the .clang-format and .clang-tidy files.

@jmoraleda

This comment has been minimized.

Copy link
Contributor Author

jmoraleda commented Oct 18, 2019

I just looked at this again. Is it possible to inherit in xml in the additional plugin from the common plugin or do I need to have the base and derived components be part of the same plugin? If so, should I move the toggle button and toggle bitmap button to the common plugin?

@sodevel

This comment has been minimized.

Copy link
Member

sodevel commented Oct 18, 2019

Apparently inherits works across xml files, at least it works with default.xml, all plugins inherit elements from that one. To not create dependencies between plugins i would prefer that if you need a common base for different plugins to store that inside default.xml like wxTreeCtrlBase currently is.

It's not that great to store something that is not of such great relevance like the other elements of that file inside there but there is currently no other common file to store elements that are required by multiple plugins.

@jmoraleda jmoraleda force-pushed the jmoraleda:BitmapToggleButton branch from 10a67a3 to d223420 Oct 21, 2019
…ode and use inheritance.
@jmoraleda jmoraleda force-pushed the jmoraleda:BitmapToggleButton branch from d223420 to f68291d Oct 21, 2019
@jmoraleda

This comment has been minimized.

Copy link
Contributor Author

jmoraleda commented Oct 21, 2019

I refactored the code so that all buttons inherit from a common class wxAnyButton (same name as the base class in wx) and placed the common code in default.xml. Please take a look at it when you have a chance and check if this is what you had in mind. Thank you.

My thoughts on the style: The xml feels elegant (minus the issue that you already point out that having a dedicated anybutton.xml might be cleaner than throwing every common class into default.xml). However the cpp code has a lot of code duplication between the four button components. It would be nice if there was a clean way of deriving from a common base class in some future version. Thank you.

@sodevel

This comment has been minimized.

Copy link
Member

sodevel commented Oct 24, 2019

Thanks for your work, this is looking good and what i had in mind. Despite some small issues the style-police found there are only two bigger issues, but take a look at the code review. It would be great if you could fix these so i can merge this easily.

@@ -57,6 +57,10 @@ Python code generation written by
<template name="settings">#ifnotequal $value "0" @{ self.$name.SetValue( True ) @}</template>
<template name="evt_connect_OnToggleButton">self.$name.Bind( wx.EVT_TOGGLEBUTTON, #handler )</template>
</templates>

This comment has been minimized.

Copy link
@sodevel

sodevel Oct 24, 2019

Member

Trailing whitespace

@@ -81,16 +81,23 @@ Written by
<property name="tab_width" type="text" help="The number of spaces per tab character.">4</property>
</objectinfo>

<objectinfo class="wxToggleButton" startgroup="1" icon="toggle_button.xpm" type="widget">

This comment has been minimized.

Copy link
@sodevel

sodevel Oct 24, 2019

Member

Extra empty line

@@ -81,16 +81,23 @@ Written by
<property name="tab_width" type="text" help="The number of spaces per tab character.">4</property>
</objectinfo>

<objectinfo class="wxToggleButton" startgroup="1" icon="toggle_button.xpm" type="widget">

<objectinfo class="wxToggleButton" icon="toggle_button.xpm" type="widget">

This comment has been minimized.

Copy link
@sodevel

sodevel Oct 24, 2019

Member

Any specifiy reason why this element shouldn't start a group anymore? I think it makes even more sense now that there is an additional toggle-button

<property name="value" type="bool">0</property>
<event name="OnToggleButton" class="wxCommandEvent" help="Handles a wxEVT_TOGGLEBUTTON event."/>
</objectinfo>

<objectinfo class="wxBitmapToggleButton" icon="bitmap_toggle_button.xpm" type="widget">

This comment has been minimized.

Copy link
@sodevel

sodevel Oct 24, 2019

Member

The specified icon file does not exist, i think you had one before, maybe it got lost during force-push?

#ifnotnull $margins
@{ #nl $name->SetBitmapMargins( $margins ); @}
</template>
<template name="settings"> #ifnotequal $default "0" @{ #nl $name->SetDefault(); @} </template>

This comment has been minimized.

Copy link
@sodevel

sodevel Oct 24, 2019

Member

Extra whitespace around the tag content


window->SetValue( ( obj->GetPropertyAsInteger(_("value")) != 0 ) );
window->Connect( wxEVT_COMMAND_TOGGLEBUTTON_CLICKED, wxCommandEventHandler( ToggleButtonComponent::OnToggle ), NULL, this );

This comment has been minimized.

Copy link
@sodevel

sodevel Oct 24, 2019

Member

This removal breaks the updating of the value if the button gets clicked in the preview, i see no reason why this feature should be removed

}
#endif

if ( !obj->IsNull( _("disabled") ) )

This comment has been minimized.

Copy link
@sodevel

sodevel Oct 24, 2019

Member

Extra padding around parentheses

button->SetBitmapPressed(obj->GetPropertyAsBitmap(_("pressed")));
}

if ( !obj->IsNull( _("focus") ) )

This comment has been minimized.

Copy link
@sodevel

sodevel Oct 24, 2019

Member

Extra padding around parentheses

return button;
}

void OnToggle( wxCommandEvent& event )

This comment has been minimized.

Copy link
@sodevel

sodevel Oct 24, 2019

Member

Extra padding around parentheses

}

button->SetValue( ( obj->GetPropertyAsInteger(_("value")) != 0 ) );

This comment has been minimized.

Copy link
@sodevel

sodevel Oct 24, 2019

Member

Here too the connect of the wxEVT_COMMAND_TOGGLEBUTTON_CLICKED is missing so that button clicks don't update the value in the model

@sodevel
sodevel approved these changes Nov 1, 2019
Copy link
Member

sodevel left a comment

Thanks for your patience and addressing these issues.

@sodevel sodevel merged commit bef1496 into wxFormBuilder:master Nov 1, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr Build is passing
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jmoraleda jmoraleda deleted the jmoraleda:BitmapToggleButton branch Nov 1, 2019
@jmoraleda

This comment has been minimized.

Copy link
Contributor Author

jmoraleda commented Nov 1, 2019

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.