GUI2: made widget initialization process more secure against memory leaks

[Vultraz]

Pulling this out of the old master branch.

This commit is the followup to a similar one I did regarding window initialization. Instead
of widgets being created on the heap and not being managed by a smart pointer until they're
added to a grid, they are now always managed by a shared_ptr. To that end, this commit covers
bunch of things:

• builder_widget::build (both overloads) and all its overrides (which have now been marked
  as such) now return shared_ptr.
• The builder_grid::build() override now returns the same instead of grid* since you can't
  use covariant return types with smart pointers.
• The two implementation build helpers in builder_grid have been combined with an optional
  replacement map as the second parameter. Uses of the version that took a grid pointer could
  be easily converted to pass a reference instead.
• The pane, matrix, and viewport build() functions were removed in favor of making the ctor
  public. In case there was a deprecated ctor, that was removed.
• The viewport now keeps a widget shared_ptr instead of a reference that was then deleted
  by address-of. This was both better design and necessary to fix a crash.
• build_single_widget_instance_helper and build_single_widget_instance were renamed to
  build_single_widget and build_single_widget_and_cast_to to better represent their roles
  and to indicate the latter was more a convenience extension for the latter than the other
  way around.

[CelticMinstrel]

This lengthy comment seems to suggest that the new code is not equivalent to the old code at all? But I have no idea what implications that would have.

[Vultraz]

It is, I just defer handling child grids until we've checked all widgets... though I'm just realizing the logic still doesn't work, since there may be more than one child grid...

[CelticMinstrel]

What do you mean, "it is"? How exactly is it equivalent?

[Vultraz]

It still looks at every widget and potentially sub-grids. I'm not sure why you're saying it's not equivalent at all.

[CelticMinstrel]

It looks like things happen in an entirely different order, and I have no idea what consequences that might have. It might give a different result in some cases, right?

[CelticMinstrel]

I've looked through all the places that call swap_child in this diff, and while I'm not 100% sure that's all calls in the entire game, it looks like this is the only place that relies on it returning nullptr if nothing was swapped. So… couldn't the owning problem be solved by instead returning the passed-in widget if nothing was swapped? Something like this (based on the original code here):

widget* test = w.get();
std::unique_ptr<widget> old = g->swap_child(id, w, true)
if(test != old) return old;

[Vultraz]

Well, it's just looking for the widget matching the given ID, so it shouldn't matter in what order it happens.

[CelticMinstrel]

There could be more than one widget with the ID. There's literally nothing to enforce that IDs are unique. In fact, they're guaranteed not to be unique if you're using anything like a listbox… though I don't know how relevant that is in this specific case.

Although neither the old nor the new could be called a sensible way of handling duplicate IDs, it certainly does change how they'd be handled.

[Vultraz]

widget* test = w.get();
std::unique_ptr<widget> old = g->swap_child(id, w, true)
if(test != old) return old;

I like this. Thanks!

[CelticMinstrel]

Is there any reason this can't be simplified to a direct assignment?

content_grid_ = get_grid().swap_child("_content_grid", std::move(content), true)

I suppose the cast gets in the way? Does static_pointer_cast work on unique_ptr or only on shared_ptr?

[Vultraz]

Only shared_ptr.

[Vultraz]

To elaborate, content_grid_ is a unique_ptr<grid>, and you can't automatically downcast from unique_ptr<widget>.