MantisBT - VCMI
View Issue Details
0000963VCMIGUI - Hero screen / Exchange windowpublic2012-05-19 19:192014-05-30 17:41
Warmonger 
Ivan 
normalblockalways
closedfixed 
PCWindows 7SP1
 
0.89 
0000963: Clicking artifacts in hero window causes crash
At least two artifatcs need to be present.

Tested in r2695 (before my latest commit related to commander artifacts)
Use attached map. Click Equestrian gloves on hero screen - assertion fail related to GUI destructors.
I'm pretty sure in the morning it worked ok.
No tags attached.
? Testy7 Arts2.h3m (3,133) 2012-05-19 19:19
https://bugs.vcmi.eu/file_download.php?file_id=940&type=bug
Issue History
2012-05-19 19:19WarmongerNew Issue
2012-05-19 19:19WarmongerFile Added: Testy7 Arts2.h3m
2012-05-19 19:24WarmongerSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=1651#r1651
2012-05-19 19:42IvanAssigned To => Ivan
2012-05-19 19:42IvanStatusnew => assigned
2012-05-19 19:43IvanNote Added: 0002505
2012-05-19 21:39IvanNote Added: 0002506
2012-05-19 21:39IvanStatusassigned => resolved
2012-05-19 21:39IvanFixed in Version => 0.89
2012-05-19 21:39IvanResolutionopen => fixed
2012-05-19 22:38TowNote Added: 0002507
2012-05-19 22:51TowFixed in Version0.89 =>
2012-05-19 23:16IvanNote Added: 0002510
2014-05-30 17:41beegeeStatusresolved => closed

Notes
(0002505)
Ivan   
2012-05-19 19:43   
Will fix later today.
Now I'm completely convinced that our GUI system needs some fixing. At least to automate deletion\activation\deactivation of objects.
(0002506)
Ivan   
2012-05-19 21:39   
Resolve in rev 2697
(0002507)
Tow   
2012-05-19 22:38   
The delChildNUll method has a second argument that allows automatic deactivation before deleting. It may be worth to switch it default value, current behaviour is indeed overly strict (aking it easy to shoot yourself in the foot.

However I believe listSelection picture should be created in constructor — it is not changed upon update anyway. Switching heroes just creates new hero window, update() is called after construction and upon artifacts movement.
(0002510)
Ivan   
2012-05-19 23:16   
Yes but:
1) There is no way to pass "activate if needed" during construction so you won't solve this issue completely
2) listSelection may not be present (or at least inactive) - if selected hero is in garrison or tavern. But making it inactive will need even more management!

In some cases activate/deactivate code takes too much place despite that in (almost) all cases it can be moved to constructor or method like addChild.

One more problem is moveChild\addChild - even if you don't care about current parent you still need to write code like this:
if (parent) move() else add()

I think that in tabs\lists half of code was such children management.

And one more problem that started to appear recently - positioning. It's quite difficult to track who and when moved this object incorrectly. Probably CIntObject::pos should be protected somehow.