GNOME Bugzilla – Bug 764433
Setting world on a default-constructed bounding box freezes the view.
Last modified: 2016-04-02 18:51:56 UTC
Using: ChamplainBoundingBox *bbox = champlain_bounding_box_new(); champlain_view_set_world(view, bbox); then any other view interaction such as: champlain_view_zoom_out(view); causes an infinite loop in champlain-viewport.c: hadjustment_value_notify_cb() (the notify callback function modifies the property so it causes another call) I've come up with a fix, but I'm not really sure about a few things: 1) champlain_bounding_box_new() returns an invalid bounding box (champlain_bounding_box_is_valid(bbox) is false). Is there a reason why that constructor sets the bounding box extremities totally wrong like this: bbox->left = CHAMPLAIN_MAX_LONGITUDE; bbox->right = CHAMPLAIN_MIN_LONGITUDE; bbox->bottom = CHAMPLAIN_MAX_LATITUDE; bbox->top = CHAMPLAIN_MIN_LATITUDE; Shouldn't it be: bbox->left = CHAMPLAIN_MIN_LONGITUDE; bbox->right = CHAMPLAIN_MAX_LONGITUDE; bbox->bottom = CHAMPLAIN_MIN_LATITUDE; bbox->top = CHAMPLAIN_MAX_LATITUDE; ? 2) Also, setting the world with an invalid bbox shouldn't be allowed anyway( it totally messes up the view ), so I've attached a patch that adds a check in the champlain_view_set_world function.
Created attachment 325112 [details] [review] champlain-vew: Add valid bounding-box check for champlain_view_set_world
(In reply to Marius Stanciu from comment #0) > Using: > ChamplainBoundingBox *bbox = champlain_bounding_box_new(); > champlain_view_set_world(view, bbox); > > then any other view interaction such as: > champlain_view_zoom_out(view); > > causes an infinite loop in champlain-viewport.c: > hadjustment_value_notify_cb() > (the notify callback function modifies the property so it causes another > call) > > I've come up with a fix, but I'm not really sure about a few things: > > 1) > champlain_bounding_box_new() returns an invalid bounding box > (champlain_bounding_box_is_valid(bbox) is false). > > Is there a reason why that constructor sets the bounding box extremities > totally wrong like this: > > bbox->left = CHAMPLAIN_MAX_LONGITUDE; > bbox->right = CHAMPLAIN_MIN_LONGITUDE; > bbox->bottom = CHAMPLAIN_MAX_LATITUDE; > bbox->top = CHAMPLAIN_MIN_LATITUDE; > > Shouldn't it be: > > bbox->left = CHAMPLAIN_MIN_LONGITUDE; > bbox->right = CHAMPLAIN_MAX_LONGITUDE; > bbox->bottom = CHAMPLAIN_MIN_LATITUDE; > bbox->top = CHAMPLAIN_MAX_LATITUDE; > > ? This is intentional. Basically the idea is to have the initial values set to the opposite extremes so when extended with another bounding box using champlain_bounding_box_extend(), its value becomes the new bounding box. Or in other words, champlain_bounding_box_new() creates a bounding box corresponding go "empty set" and champlain_bounding_box_extend() behaves like union. This is for instance used in get_bounding_box() in champlain-path-layer.c. > > 2) > Also, setting the world with an invalid bbox shouldn't be allowed anyway( it > totally messes up the view ), so I've attached a patch that adds a check in > the champlain_view_set_world function. This is a problem though. Maybe I'd just not issue a warning when this happens and test the champlain_bounding_box_is_valid() using normal if like we already do in champlain_view_ensure_visible(). Could you update the patch this way?
Created attachment 325227 [details] [review] champlain-view: Add valid bouding-box check for champlain_view_set_world
Applied, thanks!