blender/blender

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

Damien Picard commented

2023-02-25 01:50:05 +01:00

Member

In order to properly translate UI messages, they sometimes need to be
disambiguated using translation contexts. Until now, node sockets had
no way to specify contexts and collisions occurred.

This commit adds a way to declare contexts for each socket using:
.translation_context()

If no context is specified, the default null context is used.

This is a new approach for D15869. Since then, node systems have been ported to the C++ node declaration system.

In order to properly translate UI messages, they sometimes need to be disambiguated using translation contexts. Until now, node sockets had no way to specify contexts and collisions occurred. This commit adds a way to declare contexts for each socket using: .translation_context() If no context is specified, the default null context is used. ----- This is a new approach for [D15869](https://archive.blender.org/developer/D15869). Since then, node systems have been ported to the C++ node declaration system.

Damien Picard added the

Nodes & Physics

labels 2023-02-25 01:51:36 +01:00

Damien Picard reviewed 2023-02-25 01:59:32 +01:00

source/blender/editors/space_node/node_draw.cc

						
				@ -184,0 +187,4 @@

				  const nodes::SocketDeclaration &socket_decl = *socket.runtime->declaration;

				  /* The node is not explicitly defined. */

				  if (&socket_decl == nullptr) {

Damien Picard commented

2023-02-25 01:59:32 +01:00

Author

Member

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

Owner

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

Member

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

Author

Member

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

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

Damien Picard requested review from Bastien Montagne 2023-02-25 02:00:17 +01:00

Damien Picard requested review from Hans Goudey 2023-02-25 02:00:18 +01:00

Damien Picard referenced this pull request

2023-02-25 14:29:47 +01:00

Japanese translation | "Wrong" translation (Value) #105113

~~Bastien Montagne referenced this pull request 2023-02-26 16:39:20 +01:00~~

T99807: Add support for exporting to USDZ #104556

Bastien Montagne commented

2023-02-27 14:47:21 +01:00

Owner

@blender-bot build

@blender-bot build

Bastien Montagne requested changes 2023-02-27 15:17:40 +01:00

Bastien Montagne left a comment

Owner

Generally LGTM, though there are a few points to be addressed as noted below.

Would also summon @JacquesLucke here. ;)

Generally LGTM, though there are a few points to be addressed as noted below. Would also summon @JacquesLucke here. ;)

source/blender/editors/space_node/node_draw.cc

						
				@ -184,0 +198,4 @@

				  }

				  output << translation_context.data();

				  return BLI_strdup(output.str().c_str());

Bastien Montagne commented

2023-02-27 15:16:39 +01:00

Owner

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

Member

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

Author

Member

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.

pioverfour marked this conversation as resolved

Bastien Montagne requested review from Jacques Lucke 2023-02-27 15:18:01 +01:00

Jacques Lucke approved these changes 2023-02-27 15:56:37 +01:00

Jacques Lucke left a comment

Member

Besides what was mentioned in the code comments, this looks good to me.

Besides what was mentioned in the code comments, this looks good to me.

Damien Picard force-pushed dp_per_node_socket_translation_context from 04b5877712 to 8e79420a20

2023-02-27 16:18:27 +01:00

Compare

Damien Picard commented

2023-02-27 16:26:04 +01:00

Author

Member

@JacquesLucke @mont29 Do you reckon this can go into 3.5?

@JacquesLucke @mont29 Do you reckon this can go into 3.5?

Hans Goudey requested changes 2023-02-28 05:33:18 +01:00

Hans Goudey left a comment

Member

Noticed one issue, big picture seems good.

Noticed one issue, big picture seems good.

source/blender/editors/space_node/node_draw.cc

						
				@ -184,0 +195,4 @@

				    return nullptr;

				  }

				  return BLI_strdup(translation_context.data());

Hans Goudey commented

2023-02-28 05:33:07 +01:00

Member

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`

pioverfour marked this conversation as resolved

Bastien Montagne approved these changes 2023-02-28 09:29:11 +01:00

Bastien Montagne left a comment

Owner

Besides point raised by @HooglyBoogly, LGTM. But don't think this should go in 3.5, it's not that trivial of a change...

Besides point raised by @HooglyBoogly, LGTM. But don't think this should go in 3.5, it's not that trivial of a change...

Damien Picard added 1 commit 2023-02-28 19:56:05 +01:00

Address review for #105195 22d36554b6

Use a `StringRefNull` to store the translation context instead of
passing it through `BLI_strdup()`.

Hans Goudey approved these changes 2023-02-28 20:02:23 +01:00

Hans Goudey referenced this issue from a commit

2023-03-06 14:24:49 +01:00

I18n: add per-socket translation contexts for nodes

Hans Goudey merged commit 4dc59c7311 into main

2023-03-06 14:24:49 +01:00

Bastien Montagne deleted branch dp_per_node_socket_translation_context

2023-03-06 15:21:17 +01:00

~~Damien Picard referenced this pull request 2023-03-12 16:27:05 +01:00~~

I18n: disambiguate a few messages using per-socket contexts #105693

Bastien Montagne referenced this pull request

2023-03-13 12:23:41 +01:00

WIP: I18n: add per-label translation contexts for nodes #105690

Damien Picard referenced this pull request

2023-03-13 15:21:30 +01:00

WIP: I18n: add per-label translation contexts for nodes #105690

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Automated Testing

Blender Asset Bundle

Code Documentation

Dependency Graph

Development Management

Images & Movies

Motion Tracking

Nodes & Physics

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Viewport & EEVEE

Virtual Reality

Asset Browser Project

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

Nodes & Physics

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Viewport & EEVEE

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

5 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#105195

No description provided.