Opened 8 years ago

Last modified 6 years ago

#215 new enhancement

automatic GeoExt.Action map from GeoExt.MapPanel

Reported by: fredj Owned by:
Priority: minor Milestone: 1.2
Component: GeoExt.Action Version: trunk
Keywords: Cc:
State: Needs Discussion

Description

It should be possible to write something like:

var action = new GeoExt.Action({
    text: "max extent",
    control: new OpenLayers.Control.ZoomToMaxExtent()
});

var mapPanel = new GeoExt.MapPanel({
    border: false,
    renderTo: "div-id",    
    map: {
        maxExtent: new OpenLayers.Bounds(-90, -45, 90, 45)    
    },
    tbar: [action]
});  

'action' is constructed without the map argument: the MapPanel should build the OpenLayers.Map and pass it to the action.

Attachments (4)

geoext-215.patch (3.1 KB) - added by ahocevar 8 years ago.
patch-test-215-A0.diff (2.2 KB) - added by elemoine 8 years ago.
215.0.patch (3.7 KB) - added by fredj 7 years ago.
using findParentByType, not fully tested yet
215.1.patch (5.4 KB) - added by fredj 7 years ago.
clean code, basic unit test

Download all attachments as: .zip

Change History (20)

comment:1 Changed 8 years ago by ahocevar

  • State changed from None to Review

Tests still pass. Thanks for any review.

Changed 8 years ago by ahocevar

comment:2 follow-up: Changed 8 years ago by elemoine

  • State changed from Review to Needs Discussion

Andreas, your patch doesn't work if what's passed in the tbar option is a Toolbar including a menu, including menu items associated with GeoExt.Action. So the patch doesn't deal with all the cases, so I'm wondering what we should do. What do you think?

comment:3 in reply to: ↑ 2 Changed 8 years ago by ahocevar

Replying to elemoine:

Andreas, your patch doesn't work if what's passed in the tbar option is a Toolbar including a menu, including menu items associated with GeoExt.Action. So the patch doesn't deal with all the cases, so I'm wondering what we should do. What do you think?

We could bubble up the ownerCt hierarchy until we find a MapPanel, instead of assuming comp.ownerCt.ownerCt to be a map panel.

comment:4 Changed 8 years ago by ahocevar

Eric, can you provide a test or example with the cases that are not covered?

Changed 8 years ago by elemoine

comment:5 Changed 8 years ago by elemoine

See patch-test-215-A0.diff. But for some reason the first test ("action set in a button") doesn't pass either. It looks like the button has no ownerCt yet when the render listener is called, so comp.ownerCt instanceof Ext.Toolbar nevers evaluates to true. I've no time to research this more at the moment.

comment:6 Changed 8 years ago by fredj

see also findParentByType() in Ext.Component

http://www.extjs.com/deploy/dev/docs/?class=Ext.Component

comment:7 Changed 8 years ago by ahocevar

  • Owner set to ahocevar

Thanks elemoine for the patch, and thanks fredj for the pointer. I'm looking into it again now.

comment:8 follow-ups: Changed 8 years ago by ahocevar

Ok, the reason for the 1st test failing is that in Ext 2.2.1, buttons and menu items in a toolbar don't get an ownerCt assigned.

Would it be ok to provide this new functionality, but document that it requires Ext >= 3?

comment:9 in reply to: ↑ 8 Changed 8 years ago by elemoine

Replying to ahocevar:

Ok, the reason for the 1st test failing is that in Ext 2.2.1, buttons and menu items in a toolbar don't get an ownerCt assigned.

Would it be ok to provide this new functionality, but document that it requires Ext >= 3?

that's fine by me.

comment:10 in reply to: ↑ 8 ; follow-up: Changed 8 years ago by tschaub

Replying to ahocevar:

Ok, the reason for the 1st test failing is that in Ext 2.2.1, buttons and menu items in a toolbar don't get an ownerCt assigned.

Glad you confirmed the same. Thought I was going nuts yesterday when I suggested this looked Ext 3 specific.

Would it be ok to provide this new functionality, but document that it requires Ext >= 3?

I think that sounds good as well.

comment:11 Changed 8 years ago by ahocevar

  • Owner ahocevar deleted

Well, things are not that bright anyway. As soon as our action is part of a menu, it's a no-go also with Ext 3.0.0. But it works with Ext 3.1.0. I guess we need a different approach here. One thing that comes to my mind would be to have the MapPanel check its toolbars and look for actions, but this feels a bit weird. Sorry, I don't have time to investigate this further.

comment:12 Changed 8 years ago by ahocevar

Would it also be ok to write something like:

var zoomToMaxExtent = new OpenLayers.Control.ZoomToMaxExtent();

var mapPanel = new GeoExt.MapPanel({
    border: false,
    renderTo: "div-id",    
    map: {
        controls: [zoomToMaxExtent],
        maxExtent: new OpenLayers.Bounds(-90, -45, 90, 45)    
    },
    tbar: [new GeoExt.Action({
        text: "max extent",
        control: zoomToMaxExtent
    }]
});  

According to the API docs, this should be possible without any changes to the code.

comment:13 Changed 8 years ago by ahocevar

  • Milestone changed from 0.7 to 1.0

comment:14 in reply to: ↑ 10 Changed 7 years ago by mike3050

Replying to tschaub:

Replying to ahocevar:

Ok, the reason for the 1st test failing is that in Ext 2.2.1, buttons and menu items in a toolbar don't get an ownerCt assigned.

Glad you confirmed the same. Thought I was going home insurance quotes nuts yesterday when I suggested this looked Ext 3 specific.

Would it be ok to provide this new functionality, but document that it requires Ext >= 3?

I think that sounds good as well.

Yes it sounds ok to me.

comment:15 Changed 7 years ago by ahocevar

  • Milestone changed from 1.0 to 1.1

No working patch. Bumping.

Changed 7 years ago by fredj

using findParentByType, not fully tested yet

Changed 7 years ago by fredj

clean code, basic unit test

comment:16 Changed 6 years ago by ahocevar

  • Milestone changed from 1.1 to 1.2

Batch move of tickets to finish the 1.1 milestone.

Note: See TracTickets for help on using tickets.