I only looked at the code but that seems to be okay. A few minor comments but
nothing serious.
Sorry for my ignorance, but: Is this the new or the old font renderer? If it is
the old one, feel free to ignore some of my comments.
Diff comments:
> === modified file 'src/graphic/text/rt_render.cc'
> --- src/graphic/text/rt_render.cc 2017-11-27 21:21:06 +0000
> +++ src/graphic/text/rt_render.cc 2017-12-06 08:32:17 +0000
> @@ -375,7 +375,7 @@
> // Remove trailing spaces
> while (!rv->empty() && rv->back()->is_non_mandatory_space() &&
> shrink_to_fit) {
> x -= rv->back()->width();
> - delete rv->back();
> + rv->back().reset();
I think the reset() is not needed here, is called by pop_back() anyway.
> rv->pop_back();
> }
>
> @@ -404,7 +407,7 @@
> if ((*rv->rbegin())->halign() == UI::Align::kCenter) {
Okay, not part of your changes. But wouldn't it be easier to use rv->back() ?
Same construct is also somewhere else.
> remaining_space /= 2; // Otherwise, we align
> right
> }
> - for (RenderNode* node : *rv) {
> + for (std::shared_ptr<RenderNode> node : *rv) {
> node->set_x(node->x() + remaining_space);
> }
> }
> @@ -422,21 +425,20 @@
> * Take ownership of all nodes, delete those that we do not render anyways
> (for
> * example unneeded spaces), append the rest to the vector we got.
> */
> -uint16_t
> -Layout::fit_nodes(std::vector<RenderNode*>& rv, uint16_t w, Borders p, bool
> shrink_to_fit) {
> +uint16_t Layout::fit_nodes(std::vector<std::shared_ptr<RenderNode>>& rv,
> uint16_t w, Borders p, bool shrink_to_fit) {
Here rv is passed by reference, in the method above its passed by pointer. In
both cases rv is modified.
> assert(rv.empty());
> h_ = p.top;
>
> uint16_t max_line_width = 0;
> while (idx_ < all_nodes_.size()) {
> - std::vector<RenderNode*> nodes_in_line;
> + std::vector<std::shared_ptr<RenderNode>> nodes_in_line;
> size_t idx_before_iteration_ = idx_;
> uint16_t biggest_hotspot = fit_line(w, p, &nodes_in_line,
> shrink_to_fit);
>
> int line_height = 0;
> int line_start = INFINITE_WIDTH;
> // Compute real line height and width, taking into account
> alignement
> - for (RenderNode* n : nodes_in_line) {
> + for (std::shared_ptr<RenderNode> n : nodes_in_line) {
> line_height = std::max(line_height, biggest_hotspot -
> n->hotspot_y() + n->height());
> n->set_y(h_ + biggest_hotspot - n->hotspot_y());
> if (line_start >= INFINITE_WIDTH || n->x() <
> line_start) {
> @@ -1278,25 +1274,27 @@
> }
> }
>
> - void emit_nodes(std::vector<RenderNode*>& nodes) override {
> - RenderNode* rn = nullptr;
> + void emit_nodes(std::vector<std::shared_ptr<RenderNode>>& nodes)
> override {
> if (!fill_text_.empty()) {
> - if (space_ < INFINITE_WIDTH)
> - rn = new FillingTextNode(font_cache_,
> nodestyle_, space_, fill_text_);
> - else
> - rn = new FillingTextNode(font_cache_,
> nodestyle_, 0, fill_text_, true);
> + std::shared_ptr<FillingTextNode> node;
> + if (space_ < INFINITE_WIDTH) {
> + node = std::shared_ptr<FillingTextNode>(new
> FillingTextNode(font_cache_, nodestyle_, space_, fill_text_));
You could also use node.reset(new ...) here.
Also at other places.
> + } else {
> + node = std::shared_ptr<FillingTextNode>(new
> FillingTextNode(font_cache_, nodestyle_, 0, fill_text_, true));
> + }
> + nodes.push_back(node);
> } else {
> - SpaceNode* sn;
> - if (space_ < INFINITE_WIDTH)
> - sn = new SpaceNode(nodestyle_, space_, 0);
> - else
> - sn = new SpaceNode(nodestyle_, 0, 0, true);
> -
> - if (background_image_)
> - sn->set_background(background_image_,
> image_filename_);
> - rn = sn;
> + std::shared_ptr<SpaceNode> node;
> + if (space_ < INFINITE_WIDTH) {
> + node = std::shared_ptr<SpaceNode>(new
> SpaceNode(nodestyle_, space_, 0));
> + } else {
> + node = std::shared_ptr<SpaceNode>(new
> SpaceNode(nodestyle_, 0, 0, true));
> + }
> + if (background_image_) {
> + node->set_background(background_image_,
> image_filename_);
> + }
> + nodes.push_back(node);
> }
> - nodes.push_back(rn);
> }
>
> private:
> @@ -1335,7 +1333,7 @@
> : TagHandler(tag, fc, ns, image_cache, init_renderer_style,
> fontsets),
> shrink_to_fit_(shrink_to_fit),
> w_(max_w),
> - render_node_(new DivTagRenderNode(ns)) {
> + render_node_(std::shared_ptr<DivTagRenderNode>(new
> DivTagRenderNode(ns))) {
render_node_(new DivTag) should be fine here, too. Are you preferring the
explicit shared_ptr here?
> }
>
> void enter() override {
> @@ -1605,13 +1604,13 @@
>
> std::shared_ptr<const UI::RenderedText>
> Renderer::render(const std::string& text, uint16_t width, const TagSet&
> allowed_tags) {
> - std::unique_ptr<RenderNode> node(layout_(text, width, allowed_tags));
> + std::shared_ptr<RenderNode> node(layout(text, width, allowed_tags));
> return std::shared_ptr<const
> UI::RenderedText>(node->render(texture_cache_));
> }
>
> IRefMap*
> Renderer::make_reference_map(const std::string& text, uint16_t width, const
> TagSet& allowed_tags) {
This method doesn't seem to be used anywhere. Is it obsolete or not yet used?
If it is new: Why not return a smart pointer?
> - std::unique_ptr<RenderNode> node(layout_(text, width, allowed_tags));
> + std::shared_ptr<RenderNode> node(layout(text, width, allowed_tags));
> return new RefMap(node->get_references());
> }
> }
--
https://code.launchpad.net/~widelands-dev/widelands/fh1-sharedptr/+merge/334791
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/fh1-sharedptr into lp:widelands.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
