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 WCompositeWidget::formWidgetImpl() as an alternative to "WCompositeFormWidget": #90

Closed
wants to merge 1 commit into from

Conversation

SaiFi0102
Copy link
Contributor

-Added feature to add WFormModel::validator to WCompositeWidget's WFormWidget Impl by WTemplateFormView
-Made the WCompositeWidget::setDisabled function similar to WWebWidget::setDisabled so that propagateSetEnabled should only be called when isEnabled() has changed
-Should have fixed WCompositeWidget::setDisabled so that it doesn't call impl_->propagateSetEnabled() twice, since impl_->setDisabled() should also call the same function
-Added WTemplate::Functions::fwId() to get WCompositeWidget::formWidgetImpl()'s ID or WObject::id()
-Added a test in WSuggestionPopup::forEdit so that multiple calls to forEdit shouldn't cause trouble

…siteFormWidget":

-Added feature to add WFormModel::validator to WCompositeWidget's WFormWidget Impl by WTemplateFormView
-Made the WCompositeWidget::setDisabled function similar to WWebWidget::setDisabled so that propagateSetEnabled should only be called when isEnabled() has changed
-Should have fixed WCompositeWidget::setDisabled so that it doesn't call impl_->propagateSetEnabled() twice, since impl_->setDisabled() should also call the same function
-Added WTemplate::Functions::fwId() to get WCompositeWidget::formWidgetImpl()'s ID or WObject::id()
-Added a test in WSuggestionPopup::forEdit so that multiple calls to forEdit shouldn't cause trouble
@SaiFi0102 SaiFi0102 changed the title Added WCompositeWidget::formWidgetImpl() as an alternative to "WCompo…siteFormWidget": Added WCompositeWidget::formWidgetImpl() as an alternative to "WCompositeFormWidget": Mar 15, 2016
@emweb
Copy link
Collaborator

emweb commented Oct 5, 2017

Could you illustrate the use case of your added features, because I can't clearly make out what these changes are meant to solve?

In any case, this does not cleanly merge anymore. Sorry for not responding to this PR earlier.

Regards,
Roel

@SaiFi0102
Copy link
Contributor Author

SaiFi0102 commented Feb 4, 2018

I just saw your comment. The use case is that if a Wt user wants to create a form widget using Wt::WTemplate, it is not considered as an actual form widget since it is not derived from WFormWidget, rather from WTemplate. The problem arises when WTemplateFormView does not treat the WTemplate as a form widget, so you can't apply a WValidator on it and there were some other problems that I had to overcome but I can't remember clearly, because I made this PR a long time ago.

Two WTemplate based form widgets that I'm using are:

  • Custom file upload widget
  • Custom WSuggestionPopup: with search, select from list and add new entry in one template

Though these can be implemented using virtual overrides, however, the amount of effort required to implement such custom template form widgets is too high as compared to allowing WCompositeWidget (therefore WTemplate) to act as a form widget. This PR makes code management and reusability and etc etc easier.

The merge conflicts shouldn't be a problem, I've updated my repository to Wt4 and I can update this branch if you'd like to merge this PR.

@SaiFi0102
Copy link
Contributor Author

Additionally: I have made a few more modifications on Wt that makes sense to be merged on Wt, however, Wt is not very proactive in accepting community contributions. My point is, let me know if Wt is interested, then I could make the effort of organizing them into pull requests.

@emweb
Copy link
Collaborator

emweb commented Feb 5, 2018

We apologize for not having proactive in accepting contributions in the past. I am making an effort to improve this. However, PRs that introduce multiple changes without a lot of argumentation for why these changes were made, and without a clear explanation behind them, are quite difficult for us to digest. I think that some example code illustrating exactly what you're doing or what issues you are solving would go a long way to make this more digestible. Also, if there are other changes that may be valuable, it is best to split them up in separate pull requests.

This bit, for example, looks a bit scary: https://github.com/emweb/wt/pull/90/files#diff-35963b581e8b8e49690fad08608a7ab3R228

I am not comfortable merging code like that without very close inspection.

Regards,
Roel

@SaiFi0102
Copy link
Contributor Author

SaiFi0102 commented Feb 5, 2018

Thanks Roel for making the effort. I shall work on making the pull requests more formal so that it is easy to understand the rationale behind the changes. I'll probably make another pull request for this or resolve the conflicts.

However the scary code you're talking about it is to prevent propagateSetEnabled() from being called twice, which was a bug I found. I'm not sure if it is still a problem, however, I'll explain in an update on this modification soon.

@SaiFi0102 SaiFi0102 closed this May 18, 2019
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.

2 participants