Kendo UI: Added CheckboxColumn, CheckboxBehavior and CheckboxEvent

[haritos30]

Fixes #327

[haritos30]

OK tomorrow I'll see if I can add something under com.googlecode.wicket.jquery.ui.samples.kendoui.datatable

[haritos30]

ok I added a sample page but the selection does not seem to work properly. The 'selectedLines' is not returned correctly. Something funny is going on with Javascript. With the old implementation (1st commit on CheckboxBehavior.java) I got NULL selected lines, with the new implementation (2nd commit on CheckboxBehavior.java) I only get the currently selected line. I will try to fix it tomorrow after a good night's sleep :)

[mdbergmann]

You got it resolved?

[haritos30]

Ok I fixed the selection issue by setting the table option 'selectable' to false (when selectable is true you add event onChangeAjaxBehavior on DataTableBehavior.java and we don't want that).

However I have 2 questions/issues:

1. My SelectRowOnClickBehavior behavior doesn't work because somehow the table id (in the JS code) is magically replaced with something else on runtime (hence the error in the javascript console when loading the page)
2. I REALLY don't understand why If I define my OnCheckedAjaxBehavior OUTSIDE the CheckboxBehavior class the code suddenly stops working and the selectedLines comes back NULL (check 1st vs 2nd commit for CheckboxBehavior.java)

Finally I think you can probably come up with a better solution for (1).

Otherwise the implementation is ready :)

[sebfz1]

Many thanks for this! It will take for me to perform the review, test and probably refactor a bit...

1. You should not be bothered with the tableId (either you set it manually or not - but I don't recommend to set it manually because depending on when you set it, it might be too late : the behavior already used it). You can use yourKendoBehavior#getSelector or JQueryWidget.getSelector(yourComponent)
2. Maybe there a priority conflict with the js event and the event sender might not be the one you think, hence you cannot retrieve the value.

At a first look at the code, I think it would better if you can retrieve the dataItem (json object) or a dataItem property (the property linked to the IDPropertyColumn by default for instance?). As a user, when I click a row, I want to know which of my bean ID has been selected, I don't want to have to parse the selected line (if I understood the code correctly). Does it sound right?

[haritos30]

Good Morning.

The SelectRowOnClickBehavior is extending the DataboundBehavior which is extending the JQueryAbstractBehavior so there is no getSelector() method available.

Also the reason I return the line numbers and not the dataItems is that usually when you have a checkbox column in a table it's because you want to perform an operation on multiple items simultaneously (like bulk delete), so in such a case it's much more useful (for the developer) to know the line numbers than to get back the data items. Also the typical user doesn't care about the ID (the ID column could be hidden), the user only wants to select say 3 lines and press a button to perform some operation like delete/update whatever.

[sebfz1]

Not sure I completely agree on this, but as I didn't had a close look I cannot say for sure... Does it still works if/when you sort the table (by a column)?

[haritos30]

Yes it does, because the numbers actually come from the ID property column.

[mdbergmann]

I'm not sure there should be this indirection for selection with line numbers. How does this work with paging?

I think there are two use cases:

1. selecting (checking) a single line and perform some operation on that single item
2. select (check) multiple items, maybe even on multiple pages, and perform an operation on the selected items

I can't imagine that 2. works with line numbers, or?

[haritos30]

Hello Manfred,

It's not 'line numbers' per se. The numbers come from the ID property column and that's why as I said above it keeps working even if you sort by any column. Also regarding (2) yes of course it works on multiple pages thanks to the Kendo option persistSelection. Check the sample page in the pr.

[mdbergmann]

OK, thanks Haritos. Sounds OK.

[mdbergmann]

Hi Guys.

@sebfz1 Can I help something to get this sorted out?

[sebfz1]

Hi Manfred, I'm super sorry I'm very busy these days! :-/ I will try to find a few hours this weekend to review, test and complete this PR. So far there is not so much you can do for now...

[sebfz1]

Hi @haritos30, Thanks again for this PR! I reworked it to ingrate it to the DataTableBehavior directly, so the user does not bother to override a custom one. If a CheckboxColumn is provided, its change event supersedes the grid's one (even if selectable has been set to true). I also removed the javascript part, seems the grid already does the work for us. Tell me if I missed something though...

Basically the usage simply becomes:

private static List<IColumn> newColumnList()
{
    List<IColumn> columns = Generics.newArrayList();

    columns.add(new IdPropertyColumn("ID", "id", 50));
    columns.add(...);
    columns.add(new CheckboxColumn());

    return columns;
}

final DataTable<Product> table = new DataTable<Product>("datatable", newColumnList(), newDataProvider(), 25, options) {

    @Override
    public void onChecked(AjaxRequestTarget target, Object[] selectedKeys)
    {
        final String message = "Selected keys: " + Arrays.asList(selectedKeys);

        feedback.info(message);
        feedback.refresh(target);
    }
};

form.add(table);

The full commit is here: 4507bdc4b (there is some minor things unrelated to this feature but that should not be a problem to understand the commit).

edit: I realized the javascript part was to check the checkbox whenever the user clicks on the row. I think that is not needed, either the user wants to select the row (selectable is true) or wants to use checkbox. But having both is might be a little bit confusing IMHO...

[haritos30]

Hello @sebfz1

That was quite the refactoring, I see that not much of my original code has survived :smiley: I was a wicket newbie when I wrote that code 3 years ago so of course your solution is much cleaner and better integrated to the rest of the code. So maybe you should update the commit message since CheckboxBehavior does not exist anymore.

One question I have is about the CheckboxEvent. Why is the selectedKeys an array of Objects instead of an array of Strings since the getQueryParameterValues method returns a list of Strings?

Also regarding the SelectRowOnClickDataBoundBehavior I think it can be quite useful for some use cases, and I think we should at least include it so if anyone needs it in the future they don't have to reinvent the wheel.

Finally looking at the sample page I wonder why does the column menu ignore the Checkbox column?

P.S This is totally unrelated but Wicket 7.15.0 still does not show up on the central maven repository.

[sebfz1]

Thanks for your review! I'll will take care of your comments asap! :)

[sebfz1]

Hi @haritos30 ,

Sorry for the delay! (I had to build a new PC in between...)

One question I have is about the CheckboxEvent. Why is the selectedKeys an array of Objects instead of an array of Strings since the getQueryParameterValues method returns a list of Strings?

That's a good point! It actually return a List of StringValue which is a little bit different. I added the needed to make the conversion to a List of String.

Also regarding the SelectRowOnClickDataBoundBehavior I think it can be quite useful for some use cases, and I think we should at least include it so if anyone needs it in the future they don't have to reinvent the wheel.

Agreed, I will add it in the library so users can directly take it. But I need to slightly rework it first, so let's say that will be for the release after

Finally looking at the sample page I wonder why does the column menu ignore the Checkbox column?

What do you mean? I see a checkbox in the column menu, and when clicking, it selects/checks all rows. Do you have a different behavior?

I will release this feature soon... @mdbergmann , sorry again for the delay it took. I hope you could test the snapshot!

[haritos30]

Hello@sebfz1,

Good to know this feature is finally being released to the public :-) Regarding the 3rd point I was referring to the column menu. Please see the attached image:

I'm guessing it's because the checkbox column is missing a name... (Also I tried adding a name to the checkbox column but it gets ignored by Kendo...)

[sebfz1]

Ok, I see your point now! I also think it is because the column does not have a title, but I probably prefer this way. It would have been a little bit weird to have a checkbox to select the checkbox column...

I will roll releases this week-end. Also wicket 7.x series (7.17.0). 7.15.0 seems to never have reached nexus...

[mdbergmann]

@mdbergmann , sorry again for the delay it took. I hope you could test the snapshot!

No problem. We had to shift priorities a bit (unrelated to this feature). Unfortunately I was busy with other things, so I couldn't really check. But I will check soon.

Unrelated: is it worth upgrading to Wicket 9, also for Kendo?

[sebfz1]

Kendo UI is always the same version in all branches (wicket7.x, wicket8.x, wicket9.x)

I upgraded to the latest version yesterday so it will be in all releases...

[sebfz1]

Released 8.11.0 & 9.2.1 (upgraded to kendo-ui-2021.1.119). Also released 7.17.0 (but it does not includes this feature)