source/blender/nodes/geometry/nodes/node_geo_attribute_vector_math.cc
161–163	Using `std::string` is nice and we should do it more, but in this case I think using `BLI_snprintf` would probably be simpler? That or `const std::string new_label = "Vector " + name;` at least.

Charlie Jolly (charlie) added a project: Nodes.Mar 24 2021, 4:38 PM

Charlie Jolly (charlie) marked an inline comment as done.

Address comment

Harbormaster completed remote builds in B13705: Diff 35555.Mar 24 2021, 4:39 PM

Charlie Jolly (charlie) added a reviewer: Hans Goudey (HooglyBoogly).Mar 24 2021, 4:42 PM

Charlie Jolly (charlie) edited the summary of this revision. (Show Details)Mar 24 2021, 6:12 PM

Hans Goudey (HooglyBoogly) added inline comments.Mar 26 2021, 2:04 AM

source/blender/nodes/geometry/nodes/node_geo_attribute_vector_math.cc
162	Hmm, do you think translation will work on the string after you've added "Vector " to it? I'm not sure it would. Sort of a tricky situation...

Charlie Jolly (charlie) marked an inline comment as done.Mar 26 2021, 10:27 AM

Charlie Jolly (charlie) added inline comments.

source/blender/nodes/geometry/nodes/node_geo_attribute_vector_math.cc
162	I think it should because BLI_strncpy is just copying the translated string back to itself. It is set in BLI_snprintf then translated then copied back.

Hans Goudey (HooglyBoogly) added inline comments.Mar 26 2021, 9:37 PM

source/blender/nodes/geometry/nodes/node_geo_attribute_vector_math.cc
162	My point is, `IFACE` will only work if the string already has a translation. And since the translations don't necessarily know about the string "Vector Add" (it is generated dynamically right here), it will likely not be translated. A quick and dirty improvement is probably to simply use `IFACE` on "Vector %s", and just translate the two strings separately. Hopefully that makes sense for most languages.

Use IFACE_ macro with both parts of the label.
Use Vector: instead of Vector as this maybe better for translations.
BLI_snprintf(label, maxlen, IFACE_("Vector: %s"), IFACE_(name));

Charlie Jolly (charlie) marked an inline comment as done.Mar 27 2021, 12:24 AM

Harbormaster completed remote builds in B13753: Diff 35647.Mar 27 2021, 12:24 AM

I like this, the only thing I would change is not using the colon. I see why you used it but I'm not sure it's worth it. Maybe Pablo has an opinion on that.

Looking good!