PDA

View Full Version : Grid Constructor Inconsistencies



gte619n
11 Dec 2010, 10:24 AM
Hey guys,

So I'm trying to extend Grid to show a specific type of object using Gin injection. I believe the constructor structure is a little inconsistent WRT the protected constructor.

My grid uses an RPC proxy to pull the Model objects down from the server. I would like to do all my column configuration and loader setup in the constructor, then call reconfigure to set my store/model. If done using super() I get a NPE on view.initData(store, cm) on line 492 of Grid. If I call super( null, null ) in my constructor to get the grid to instantiate the GridView, then everything works as expected.

I would think the Grid constructor would look more like this:



protected Grid() {
this.view = new GridView();
focusable = true;
disabledStyle = null;
baseStyle = "x-grid-panel";
setSelectionModel(new GridSelectionModel<M>());
disableTextSelection(true);
}

public Grid(ListStore<M> store, ColumnModel cm) {
this();
this.store = store;
this.cm = cm;
}


Is this a minor bug or am I structuring the component incorrectly?

Thanks!

Evan

sven
11 Dec 2010, 10:27 AM
Why should this be a big?

I would like to do all my column configuration and loader setup in the constructor, then call reconfigure to set my store/model

You can do that in your custom grid subclass, no need to call reconfigure. Why dont you call super with the just created columnmodel and liststore?

sven
11 Dec 2010, 10:28 AM
If you want to use reconfigure, simple create your grid with an empty ColumnModel and empty ListStore. Both are required for a Grid.

Please post your code and somebody is probably able to correct it.

gte619n
11 Dec 2010, 10:30 AM
You can do that in your custom grid subclass, no need to call reconfigure. Why dont you call super with the just created columnmodel and liststore?

I'm a little unclear here. I'd like to do some configuration on the constructor itself. The super() call must be the first statement of the constructor?

sven
11 Dec 2010, 10:32 AM
I'm a little unclear here. I'd like to do some configuration on the constructor itself. The super() call must be the first statement of the constructor?

I dont know how you create your stuff, so i cannot answer this, but yes, super needs to be the first line. I dont even fully understand your problem. Why do you want to call super() and not super(store,columnmodel).

gte619n
11 Dec 2010, 10:40 AM
Sven,

I'd like to configure the store/columns in the constructor. Here's what I've got:



@Inject // This is the gin stuff...
private UserMananger userManager;

@Inject
private Application application;

public UserGrid()
{
super( null, null ); // Ugly.

RpcProxy<GXTModelList<User>> proxy = new RpcProxy<GXTModelList<User>>()
{
@Override
protected void load( Object loadConfig, AsyncCallback<GXTModelList<User>> callback )
{
BaseListLoadConfig blC = (BaseListLoadConfig) loadConfig;
userManager.getUserList( blC, callback );
}

};

this.loader = new BaseListLoader<GXTModelList<Group>>( proxy );
this.loader.addLoadListener( new LoadListener()
{
public void loaderLoadException( LoadEvent le )
{
if (le.exception instanceof SessionExpiredException)
{
application.sessionExpired();
}
else
{
Window.alert( "Problem with Service." );
}
}
} );

this.userStore = new ListStore<GXTModelFactory<User>>( this.loader );
this.reconfigure( this.userStore, getColumnListModel() );
this.setLoadMask( true );
}


Effectively, calling super( null, null ) gives you the empty effect. Give the protected scope of the Grid's empty constructor, I'm a little confused of its utility. If it's going to cause NPEs all over the place, shouldn't it be private? I believe changing around the constructor structure would give the same effect and eliminate the need for the weird super( null, null ) requirement.

sven
11 Dec 2010, 10:43 AM
If it's going to cause NPEs all over the place, shouldn't it be private?
Its there so we maybe could add somethign there if needed and also for backward compatibility (it was used long time ago). If you want to use it, than you will need to setup a view too, because its not creating it at the moment. Same for a SelectionModel.

I dont see why super(null, null) is ugly, but ok. If you want to skip that, you need to setup a view on your own and the problem will be gone.

This change is far too big for any minor release, so we cannot do it. I will change the code as much as i can (leaving view and selectionmodel there, but moving everything else)

Index: user/src/com/extjs/gxt/ui/client/widget/grid/Grid.java
===================================================================
--- user/src/com/extjs/gxt/ui/client/widget/grid/Grid.java (revision 2333)
+++ user/src/com/extjs/gxt/ui/client/widget/grid/Grid.java (working copy)
@@ -299,17 +299,17 @@
* @param cm the column model
*/
public Grid(ListStore<M> store, ColumnModel cm) {
+ this();
this.store = store;
this.cm = cm;
this.view = new GridView();
- disabledStyle = null;
- baseStyle = "x-grid-panel";
setSelectionModel(new GridSelectionModel<M>());
- disableTextSelection(true);
}

protected Grid() {
-
+ disabledStyle = null;
+ baseStyle = "x-grid-panel";
+ disableTextSelection(true);
}

@Override

gte619n
11 Dec 2010, 10:55 AM
Sven,

Not trying to give you a hard time, but this implementation will still NPE all over. If initializing the view and selection model is outside the purview of the Grid itself, I'd recommend something like this:



protected Grid() {
focusable = true;
disabledStyle = null;
baseStyle = "x-grid-panel";
disableTextSelection(true);
}

protected Grid( GridView view, GridSelectionModel<M> selectionModel ){
this();
this.view = view;
setSelectionModel(selectionModel);
}

public Grid(ListStore<M> store, ColumnModel cm) {
this();
this.store = store;
this.cm = cm;
}


That way, the stupid people that don't read the API (me) can very easily see what is required to extend Grid.

Thanks for your explanation!

sven
11 Dec 2010, 10:58 AM
That change is a far too big change as i said and i am not going to add it to any GXT 2.X release. It also wont work, because view and selectionmodel are not setup if you use the liststore contructor.

What i gave you is the smallest possible change that i can do.

If you extend classes you need to understand them. You cannot just extend something and hope it works. If a required contructor is no longer used you should know, that you need to do more.

sven
11 Dec 2010, 11:06 AM
With the change i can do your code would look like this:


@Inject // This is the gin stuff...
private UserMananger userManager;

@Inject
private Application application;

public UserGrid()
{
super(); //not required anymore, as default one.

RpcProxy<GXTModelList<User>> proxy = new RpcProxy<GXTModelList<User>>()
{
@Override
protected void load( Object loadConfig, AsyncCallback<GXTModelList<User>> callback )
{
BaseListLoadConfig blC = (BaseListLoadConfig) loadConfig;
userManager.getUserList( blC, callback );
}

};

this.loader = new BaseListLoader<GXTModelList<Group>>( proxy );
this.loader.addLoadListener( new LoadListener()
{
public void loaderLoadException( LoadEvent le )
{
if (le.exception instanceof SessionExpiredException)
{
application.sessionExpired();
}
else
{
Window.alert( "Problem with Service." );
}
}
} );

this.userStore = new ListStore<GXTModelFactory<User>>( this.loader );
this.store = this.userStore;
this.cm = getColumnListModel();
this.view = new GridView();
setSelectionModel(new GridSelectionModel<M>());
this.setLoadMask( true );
}

gte619n
11 Dec 2010, 11:07 AM
Sven,

That's what I was thinking. Thanks for the change!

E

sven
11 Dec 2010, 11:08 AM
Take a look at my last reply, you probably missed it because we made them at the same time.