user specified filter controls in the datagrid

[vakopian]

I'd like to have a way of giving the user (programmer) the ability to put whatever QControl's they like as filter controls (instead of the default textbox or listbox). Imagine, users might want to do calendars, or check-lists or any other "wild" control they come up with. Vexed, if you have already thought of this and have some preferences, please let me know. Otherwise, I'll try to come up with my own implementation.

-Vartan

[VexedPanda]

I hadn't really thought of that. My original plan was to simply extend the built-in options until they covered everything. The benefit of that approach is that the SetDataGridColumnFilter? function can be designed to automatically pick the most appropriate one(s) for the column type.

So I'd still like to be able to support SetDataGridColumnFilter?, by using appropriate core QControls as the defaults, but I'd love to see the ability to extend it to support non-core controls as well. Any ideas you have on how to accomplish that are more than welcome. :)

I haven't thought about it much before, apart from wanting to ensure we can do really useful stuff like have a selection of comparators beside the "value" control.

ie: It'd be great for a user to be able to choose "Greater Than" from a dropdown, before picking a date from a calendar.

[vakopian]

Replying to VexedPanda:

I hadn't really thought of that. My original plan was to simply extend the built-in options until they covered everything.

I don't think trying to add all scenarios is doable, simply because we should allow for controls that are not part of core, like plugin controls.

The other problem with SetDataGridColumnFilter approach is that QDataGrid generates the controlId's for the filter controls internally, I think we should keep it that way. So I don't think we can let the use instantiate a control and then set it into the datagrid.

Instead, I propose a factory approach. We add a FilterControlFactory? property to QDataGrid. The factory has a method in its interface:

public function createFilterControl($strControlId, QDataGridColumn $objColumn, $value);

We'll have a default implementation, that does what the lines 885-894 in QDataGridBase.class.php do. And these lines will just be replaced by

$ctlFilter = $this->FilterControlFactory->createFilterControl($strControlId, $objColumn, $value);

What do you think?

[VexedPanda]

SetDataGridColumnFilter? is just an automated way to set the FilterType? / etc based on the type of QQNode, so I don't think it conflicts with anything you're suggesting.

Your line numbers are off, but I'm assuming you meant refactoring the swtich on filter type into its own function.

So then the intent would be to have people able to define their own FilterType?, and override this function to use them within their own QDataGrid.class.php files.

I think this seems perfectly reasonable, but there's more to it than just the creation of the controls. Other functions could potentially also be affected:
SetFilters?
GetFilters?
ClearFilters?
GetFilterControlValue?
btnFilter_Click

Not all of these are as easy to fix by re-factoring switch statements, but it may still be doable.

[vakopian]

Replying to VexedPanda:

SetDataGridColumnFilter? is just an automated way to set the FilterType? / etc based on the type of QQNode, so I don't think it conflicts with anything you're suggesting.

I'm not really clear what SetDataGridColumnFilter would be doing.

Your line numbers are off, but I'm assuming you meant refactoring the swtich on filter type into its own function.

Yes.

So then the intent would be to have people able to define their own FilterType?, and override this function to use them within their own QDataGrid.class.php files.

Well, I wonna get rid of the FilterType altogether, and instead model it with real classes. So I'm thinking to have an interface, something like this:

interface QDataGridFilterControl {
	public function GetValue();
	public function SetValue($mixValue);
	public function Clear();
}

The FilterFactory::CreateFilter() will return an instance of QDataGridFilterControl.
The actual filter controls can implement this interface and inherit from QControl's. Like this:

class QDataGridFilterTextBox extends QTextBox implements QDataGridFilterControl {
//...
}
class QDataGridFilterListBox extends QListBox implements QDataGridFilterControl {
//...
}

And so the users can define their own (wrapper) classes the same way, and make their factory return their control object.

I think this seems perfectly reasonable, but there's more to it than just the creation of the controls. Other functions could potentially also be affected:
SetFilters?
GetFilters?

In principle those don't have to be affected much, because I only wonna change the controls, not the filters. But I noticed that these methods have a couple of problems: a) $col->Name doesn't have to be unique, so the key to the array is not a good one. b) either line 1134 or line 1135 in QDataGridBase.class.php should be removed, since FilterActivate() also sets the filter.

ClearFilters?
GetFilterControlValue?

See the corresponding methods in the QDataGridFilterControl interface.

btnFilter_Click

This also doesn't need to be affected much.

Not all of these are as easy to fix by re-factoring switch statements, but it may still be doable.

I know :-) But the filter handling looks a bit messy right now. Some of the code in QDataGridBase (e.g. in SetFilters and btnFilter_Click) should be moved into QDataGridColumn and maybe merged into a single ActivateFilter() method there.

Anyway, I think I'll be doing this in stages. I'll first try to do some cleanup type of refactoring for the existing code. Then I'll try to introduce the interface and the factory.

[VexedPanda]

SetDataGridColumnFilter? uses info contained in a QQNode to automatically set the appropriate filters for a column, which is what MetaControls? use to "magically" have filters set up.

As for the rest of your ideas, that sounds pretty good to me. :)
Just as long as the syntax for the control is still very similar, and we maintain backwards compatibility (for 1.1 at least. 2.0 I could be persuaded to ditch QFilterType, but not for much longer).

As for lines 1134-1135, SetFilters? is intended to also apply them, not just fill in the textboxes. So it's behaving as desired.

The only other word of caution I want to put out there is that we must still support custom filtering. See Example B at  http://examples.qcu.be/assets/_core/php/examples/datagrid/advanced_filtering.php

[vakopian]

Replying to VexedPanda:

SetDataGridColumnFilter? uses info contained in a QQNode to automatically set the appropriate filters for a column, which is what MetaControls? use to "magically" have filters set up.

As for the rest of your ideas, that sounds pretty good to me. :)
Just as long as the syntax for the control is still very similar, and we maintain backwards compatibility (for 1.1 at least. 2.0 I could be persuaded to ditch QFilterType, but not for much longer).

Hopefully, the cleanup part can apply to 1.1, but the reset (interface, factory, etc), I'm thinking 2.0 only.

As for lines 1134-1135, SetFilters? is intended to also apply them, not just fill in the textboxes. So it's behaving as desired.

Yes, you're right, but this is another place I would really like to cleanup so it's less confusing: assigning to $col->Filter means something very different from assigning to $col->objFilter. I would probably prefer to rename $objFilter to $objActiveFilter, keep the "Filter" property as a write-only (updating the array), and rename the "Filter" read-property to "ActiveFilter?".

The only other word of caution I want to put out there is that we must still support custom filtering. See Example B at  http://examples.qcu.be/assets/_core/php/examples/datagrid/advanced_filtering.php

duly noted :-)

[vakopian]

Here is a first shot at the cleanup stage I was talking about. This patch assumes that the patch from #566 is already applied (I can provide a combined patch if needed). These changes are supposed to be fully backward compatible with 1.1 (but the patch is against 2.0). Here is what I tried to do in this stage:
1) reduce the coupling between QDataGridBase and the QDataGridColumn
2) refactor common code into QDataGridColumn methods.
3) rename $objFitler to $objActiveFilter in QDataGridBase
4) small stylistic and formatting issues and annotations (mainly to help the IDE).

Once I start implementing the factory idea, I'll see if more refactoring is needed; so there maybe another cleanup stage.

I would really appreciate any feedback, before I move too far along this path.

[vakopian]

patch for stage 1

[VexedPanda]

Looks great!
There are just a couple tweaks needed, mostly in the comments:
1) QDataGrid->Owner should be listed as a read/write property
2) QDataGridColumn Filter and ActiveFilter? should have types specified (mixed)
3) HasFilter? should be not be checking ActiveFilter?. A column can have a filter, regardless of if it is currently applied.
4) I'm unsure why you removed the FilterByCommand? setter. How should this be set then?
5) I'm also unsure what the FilterInfo? getter is for..?

So the next step then is to make QTextBoxFilter and QListBoxFilter and QCustomFilter classes, and move most of the code in QDataGridColumn over into them, but without the need for switch statements/etc?

So instead of $col->FilterType? = QFilterType::TextFilter?, we could do $col->FilterControl? = new QTextBoxFilter(); (and leave the FilterType? setter to do this for old code).

I love it. :)

[vakopian]

Replying to VexedPanda:

Looks great!
There are just a couple tweaks needed, mostly in the comments:
1) QDataGrid->Owner should be listed as a read/write property

ok

2) QDataGridColumn Filter and ActiveFilter? should have types specified (mixed)

ok

3) HasFilter? should be not be checking ActiveFilter?. A column can have a filter, regardless of if it is currently applied.

Agreed, but all it's saying is that "if ActiveFilter is set, then it has a filter". Is this false? Could the column have an ActiveFilter set, but "not have filter"? What's the scenario?

4) I'm unsure why you removed the FilterByCommand? setter. How should this be set then?

It's not removed, it was there twice before, so just a cleanup.

5) I'm also unsure what the FilterInfo? getter is for..?

To help FilterInfo getter on the QDataGrid side. It hides the details how you obtain FilterInfo for each column. Ideally, I would wonna replace the FilterByCommand array by a real class, so for now, I'm trying to make it as self contained as possible. It will also depend how close I can get in making it backward compatible.

So the next step then is to make QTextBoxFilter and QListBoxFilter and QCustomFilter classes, and move most of the code in QDataGridColumn over into them, but without the need for switch statements/etc?

right.

So instead of $col->FilterType? = QFilterType::TextFilter?, we could do $col->FilterControl? = new QTextBoxFilter(); (and leave the FilterType? setter to do this for old code).

This is where the tricky part starts. The user cannot be doing $col->FilterControl = new QMyNiceFilterControl();, because the id, the parent and the lifespan of the filter control is managed by QDataGrid. So the user still has to manage the controls by assigning _something_ to $col->FilterType (then the factory can instantiate the controls based on this type). So the question is, what should this FilterType be, considering that the user cannot really add custom types to QFilterType? One solution is to allow any string, so for example the user would do $col->FilterType = 'MyNiceControl', and then in her factory, she can do

class MyFilterFactory extends QDefaultFilterFactory {
	public function createFilterControl($strControlId, QDataGridBase $objDataGrid, QDataGridColumn $objColumn, $value) {
		if ($objColumn->FilterType === 'MyNiceControl') {
			$ctl = new QMyNiceFilterControl($strControlId, $objDataGrid);
			$ctl->SetValue($value);
			return $ctl;
		}
		return parent::createFilterControl($strControlId, $objDataGrid, $objColumn, $value);
	}
}

The second approach is to make the FilterType hold the class name of the filter control (and the user would do $col->FilterType = 'QMyNiceFilterControl'). With this, the default filter factory can use that class name to instantiate the control (without the need for the user to create a custom factory). This way, the default factory can handle most of the situations, but will still allow the custom factory approach above, if the control constructor is not a standard one.

I'm not sure if my explanation is clear enough. It would probably be easier to see the code. As of now am leaning in the direction of the second approach, because it would be easier on the user, and is still generic (can still do the first approach).

Any thoughts?

I love it. :)

I hope you still love it after the details above :-)

[VexedPanda]

3) You're right of course. Sorry, somehow thought those were &&s.
4) Doh, thanks.
5) Sounds good. I'm also hoping that "custom" will be just another filter control in the end. :)

The second approach does sound good, though I'm still holding out hope that the syntax I specified would also work. What if we take what's is normally in a QControl's constructor and just place that in an "initialize" that the QDataGrid can call with the appropriate values. Wouldn't that give the QDataGrid the control needed to use an externally-constructed class?

Since learning about it, I'm a fan of Dependency Injection, and it seems like a perfect fit here. :)

[alex94040]

Folks - it looked like you were basically done here with stage 1. Are we ready to check that stuff in?

[VexedPanda]

I want to delay this checkin until after 2.0.2 since it's a big restructuring, and may cause issues. Other than that, I'd say plug it into head as soon as 2.0.2 is out the door, and we'll continue polishing it for 2.0.3. :)

[alex94040]

Good news - 2.0.2 was just forked :)))

[vakopian]

(In [1298]) As agreed previously, committing stage1 since 2.0.2 is already released.
reviewed by VexedPanda?. Fixes #568

[vakopian]

(In [1304]) Add omitted return statement
Refs #568

[kon]

I tested the new code and on columns where i used $col->FilterAddListItem?() it broke the filters and the dropdown ended up having only one entry "0".

I tracked it down to the following code on line 258 (or 259? something like that, haven't got a clean 2.0 branch HEAD here right now)
$QDatagridColumn->SetActiveFilterState?:

$this->Filter = $mixFilterValue; // should we preserve $arrFilterList instead of replacing it? 

This is obviously because in the 'Filter' setter of QDatagridColumn, there is this line of code:

$this->arrFilterList = array($mixValue);

Now, i am not quite sure what the first line is for, where the Filter setter is used in SetActiveFilterState?.
But according to the comment in the code, there seems to have been some doubt about this already.

On another note, whether it's actually required or not, i fixed this by changing the Filter setter line from above to:

$this->arrFilterList = is_array($mixValue) ? $mixValue : array($mixValue);

Shouldn't it only set the ActiveFilter?? Maybe someone could shed some light on this for me, please. why is the Filter being overwritten in the SetActiveFilterState??
I don't know if the correct solution would be to preserve it there, or use the updated setter from the code just above this paragraph.

[vakopian]

kon,

In the patch for #694, that snippet is removed, and SetFilters/GetFilters? is also simplified.
It would be great if you could give that a try, to see if it solves the problem you described here, as well as the SetFilters/GetFilters? functionality itself.

Thanks

[kon]

Thanks vakopian!
It works great after merging your diff from #694, i'd say this can be closed again :)

[vakopian]

(In [1321]) tested by kon. Fixes #694, Fixes #568