You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@wicket.apache.org by duesenklipper <gi...@git.apache.org> on 2018/09/14 14:07:19 UTC

[GitHub] wicket pull request #292: WICKET-6587 make CheckBoxSelector extensible

GitHub user duesenklipper opened a pull request:

    https://github.com/apache/wicket/pull/292

    WICKET-6587 make CheckBoxSelector extensible

    

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/duesenklipper/wicket wicket-8/WICKET-6587-extensible-checkbox-selector

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/wicket/pull/292.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #292
    
----
commit 90b152b6c97d61aabe25aa5ed8ef481c1fbdc6ab
Author: Carl-Eric Menzel <cm...@...>
Date:   2018-09-14T14:00:16Z

    WICKET-6587 make CheckBoxSelector extensible

----


---

[GitHub] wicket issue #292: WICKET-6587 make CheckBoxSelector extensible

Posted by duesenklipper <gi...@git.apache.org>.

Github user duesenklipper commented on the issue:

    https://github.com/apache/wicket/pull/292
  
    That is a possibility, yes, though in your example you do hold on to the connectedCheckBoxes. My main idea was to keep it simple, by providing addCheckBox() users can just add checkboxes in a loop, e.g. in ListView#populateItem.
    
    Any checkboxes that are removed from the component hierarchy are cleared from connectedCheckBoxes by the attached behavior, so there shouldn't be a leak.
    
    But I'm not heavily invested in this design, if you prefer a lookup method, I can do that.


---

[GitHub] wicket issue #292: WICKET-6587 make CheckBoxSelector extensible

Posted by svenmeier <gi...@git.apache.org>.

Github user svenmeier commented on the issue:

    https://github.com/apache/wicket/pull/292
  
    Why not add a lookup to CheckBoxSelector instead:
    
    ```
    public CheckBoxSelector(String id, CheckBox... boxes)
    {
    	super(id);
    
    	connectedCheckBoxes = boxes;
    }
    
    /**
     * Override if you need want the boxes to be collected dynamically.
     */
    protected List<Check> getCheckBoxes() {
        return Arrays.asList(connectedCheckBoxes);
    }
    
    @Override
    protected CharSequence getFindCheckboxesFunction()
    {
    	return String.format("Wicket.CheckboxSelector.getCheckboxesFunction(%s)",
    		buildMarkupIdJSArrayLiteral(getCheckBoxes()));
    }
    ```
    No need to hold references to all checkboxes, with the possibility of leakage when one is removed.


---

[GitHub] wicket issue #292: WICKET-6587 make CheckBoxSelector extensible

Posted by svenmeier <gi...@git.apache.org>.

Github user svenmeier commented on the issue:

    https://github.com/apache/wicket/pull/292
  
    Ah, I didn't notice your cleanup behavior. That's an improvement to the old solution already +1.
    
    I just think that a dynamic lookup with an overriden #getCheckBoxes() would be more flexible. Why not do both: Add the cleanup all checkboxes passed in the constructor *and* let users override the lookup method if they want/need to.


---

[GitHub] wicket pull request #292: WICKET-6587 make CheckBoxSelector extensible

Posted by asfgit <gi...@git.apache.org>.

Github user asfgit closed the pull request at:

    https://github.com/apache/wicket/pull/292


---