Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing vector legends in grid when sorting #267

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

chrismayer
Copy link
Contributor

This ensures that the vector legends draw in a grid column won't disappear when the grid is sorted.
When updating the legend (which is triggered by sorting) now the default feature object for the given geometry type is used instead of directly passing the given feature to the renderer (fixes #266).

I guess the root cause is somewhere in the deep of our parent libraries but this will fix it. Since the FeatureRenderer class screams for a re-write (feature rendering with OpenLayers is much easier than it was in early 2015) I suggest to merge this.

I also adjusted the feature grid example to show that this also works with other geometry types than points.

Please review.

This ensures that the vector legends draw in a grid column won't
disappear when the grid is sorted.
This is done by using the default feature object for the given
geometry type instead of passing the given feature directly to
the renderer (fixes geoext#266).
This changes the feature grid example so that on of the grids shows
features (incl. correct legends) with polygons as underlying
geometries and not just point features as before.
@marcjansen
Copy link
Member

I fear this is not the right fix, even though it works for the inline example.

In an application we have, this makes things worse:

Before:

before

After:

after

with an error thrown: FeatureRenderer.js?_dc=1489651106312:452 Uncaught TypeError: me[(("get" + geom.getType(...)) + "Feature")] is not a function

Probably because the geometry is MultiPolygon?!

If I understand the diff correctly, whith ypour changes we would simply loose the possibility to use an actually passed feature for the rendering (which might make a lot of sense when you look at polygons). This is also something which is not tested right now, I fear

I will think about this a bit further… but if you come up with something, that'd be much appreciated.

@chrismayer
Copy link
Contributor Author

chrismayer commented Mar 16, 2017

Hi @marcjansen!
It seems we have several problems here:

1.) The geometry type MultiPolygon (and other MultiGeom types) doesn't seem to be supported as default geometries in the FeatureRendere yet.
2.) I was not aware that it is possible to render other geometries than the default ones with the applied style object. This is of course broken by the changes of this PR.

Handling 1.) would not be a problem. Fixing 2.) would restore the old (but broken while sorting) behavior.

EDIT: I am also not sure if we need the rendering of passed features. I never missed this.

@marcjansen
Copy link
Member

There are even more issues, as the rendering is way too complicated with a dedicated map etc. OpenLayers has more lightweight ways of doing this...

The rendering of passed features is quite nice for polygons...

@chrismayer
Copy link
Contributor Author

rendering is way too complicated with a dedicated map etc. OpenLayers has more lightweight ways of doing this...

That's what I meant in the PR description by

Since the FeatureRenderer class screams for a re-write (feature rendering with OpenLayers is much easier than it was in early 2015)

So, how do we proceed on this?

@marcjansen
Copy link
Member

So, how do we proceed on this?

Good question. I think we need to keep the rendering of specific features, as this is the complete purpose of the feature renderer, we are not talking about a specific gx_featurecolumn, but we are talking about application logic (e.g. in our own examples). Maybe we find time to look at this during FOSSGIS?

@chrismayer
Copy link
Contributor Author

I think we need to keep the rendering of specific features, as this is the complete purpose of the feature renderer,

Oh right, I mixed this up with the SymbolizerColumn. Of course we need to render features in their whole fashion.

Maybe we find time to look at this during FOSSGIS?

That's exactly what I thought. It's easier to solve this face-to-face.
BTW: I started playing around a bit with the canvas rendering context of OL in the FeatureRenderer. Looks promising, but brings other problems (of course 😉)

@marcjansen
Copy link
Member

Is the original issue still present? if so, we should have a chat about this anytime soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FeatureRenderer in grid column disappear when sorting
2 participants