widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

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.