blender
/
blender
136
413
Fork 596
Code Issues 6.3k Pull Requests 771 Projects 19 Wiki Activity

I18n: add per-socket translation contexts for nodes #105195

Merged
Hans Goudey merged 3 commits from pioverfour/blender:dp_per_node_socket_translation_context into main 2023-03-06 14:24:49 +01:00
Conversation 2 Commits 3 Files Changed 4 +4 -7
1 changed files with 4 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit ddc864b09c - Show all commits

11
source/blender/editors/space_node/node_draw.cc
View File

@ -183,22 +183,19 @@ namespace blender::ed::space_node {
static const char *node_socket_get_translation_context(const bNodeSocket &socket)
{
std::stringstream output;
const nodes::SocketDeclaration &socket_decl = *socket.runtime->declaration;
/* The node is not explicitly defined. */
if (&socket_decl == nullptr) {
if (socket.runtime->declaration == nullptr) {
return nullptr;
}
Damien Picard commented 2023-02-25 01:59:32 +01:00
Review
Copy Link

This is needed to avoid a crash when certain nodes are added (for instance, Render Layers in the compositing nodes).

GCC gives a warning:
the compiler can assume that the address of ‘socket_decl’ will never be NULL [-Waddress].

I suppose it means this should not happen because a null pointer would mean a bug occurs before reaching this point. But I don’t know how to fix this better since it appears not all nodes have a declaration.

This is needed to avoid a crash when certain nodes are added (for instance, Render Layers in the compositing nodes). GCC gives a warning: `the compiler can assume that the address of ‘socket_decl’ will never be NULL [-Waddress]`. I suppose it means this should not happen because a null pointer would mean a bug occurs before reaching this point. But I don’t know how to fix this better since it appears not all nodes have a declaration.
Bastien Montagne commented 2023-02-27 14:49:46 +01:00
Review
Copy Link

You cannot use a reference if there is a chance that the pointer will be NULL. So if socket.runtime->declaration can be nullptr, you need to use a pointer for socket_decl - or do the check before assigning it to your reference.

You cannot use a reference if there is a chance that the pointer will be NULL. So if `socket.runtime->declaration` can be `nullptr`, you need to use a pointer for `socket_decl` - or do the check before assigning it to your reference.
Iliya Katushenock commented 2023-02-27 14:55:51 +01:00
Review
Copy Link

At first check socket.runtime->declaration != nullptr, and after that dereference pointer

At first check `socket.runtime->declaration != nullptr`, and after that dereference pointer
Damien Picard commented 2023-02-27 16:21:22 +01:00
Review
Copy Link

Thanks for the help! I hope I fixed this correctly.

Thanks for the help! I hope I fixed this correctly.
blender::StringRef translation_context = socket.runtime->declaration->translation_context;
/* Default context. */
blender::StringRef translation_context = socket_decl.translation_context;
if (translation_context.is_empty()) {
return nullptr;
}
output << translation_context.data();
return BLI_strdup(output.str().c_str());
return BLI_strdup(translation_context.data());
pioverfour marked this conversation as resolved
Hans Goudey commented 2023-02-28 05:33:07 +01:00
Review
Copy Link

Why BLI_strdup? Could you just use StringRefNull and return that directly? It looks like the result isn't being freed, and this negates the benefit of any small string optimization in std::string

Why `BLI_strdup`? Could you just use `StringRefNull` and return that directly? It looks like the result isn't being freed, and this negates the benefit of any small string optimization in `std::string`
}
static void node_socket_add_tooltip_in_node_editor(const bNodeTree &ntree,
pioverfour marked this conversation as resolved
Bastien Montagne commented 2023-02-27 15:16:39 +01:00
Review
Copy Link

Not sure why you are using an intermediate std::stringstream here? Can't you just do return BLI_strdup(socket_decl.translation_context.c_str()); ?

Not sure why you are using an intermediate `std::stringstream` here? Can't you just do `return BLI_strdup(socket_decl.translation_context.c_str());` ?
Jacques Lucke commented 2023-02-27 15:54:43 +01:00
Review
Copy Link

That sounds reasonable.
I guess the reason for why the std::stringstream was used is that you assigned it to a StringRef which does not have c_str method, because it might not be null-terminated. Using const StringRefNull or what @mont29 would be better though.

That sounds reasonable. I guess the reason for why the `std::stringstream` was used is that you assigned it to a `StringRef` which does not have `c_str` method, because it might not be null-terminated. Using `const StringRefNull` or what @mont29 would be better though.
Damien Picard commented 2023-02-27 16:23:30 +01:00
Review
Copy Link

To be quite honest I copied code doing a similar thing from node_socket_get_tooltip(), but I see that it doesn’t really apply here.
I’ll go with @mont29 ’s solution.

To be quite honest I copied code doing a similar thing from `node_socket_get_tooltip()`, but I see that it doesn’t really apply here. I’ll go with @mont29 ’s solution.