Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix translations update in window & Dialogs (AcceptDialog/ConfirmationDialog/FileDialog) (Fix 39320, 39258 & 45887) #46735

Merged
merged 1 commit into from
Mar 22, 2021

Conversation

fabriceci
Copy link
Contributor

@fabriceci fabriceci commented Mar 6, 2021

fix.mp4
fix2.mp4

[Edit]
Updated to also fix #45887

For reviewers:

For refreshing the window:

// I used
child_controls_changed();
// but this works too
if (embedder) {
	embedder->_sub_window_update(this);
}

However, I don't understand well the difference between _sub_windows_update and child_controls_changed as the last one end by calling _sub_windows_update. Both works, but I'm not sure which is the best.

@fabriceci fabriceci requested review from a team as code owners March 6, 2021 20:08
@Calinou Calinou added bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:gui labels Mar 6, 2021
@Calinou Calinou added this to the 4.0 milestone Mar 6, 2021
@fabriceci fabriceci force-pushed the fix-dialog-translation branch 2 times, most recently from 3ae3769 to 7d5dad9 Compare March 7, 2021 20:52
@fabriceci fabriceci changed the title fix translation update in Window & AcceptDialog/ConfirmationDialog (Fix 39320 & 39258) Fix translations update in window & Dialogs (AcceptDialog/ConfirmationDialog/FileDialog) (Fix 39320, 39258 & 45887) Mar 7, 2021
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I don't understand well the difference between _sub_windows_update and child_controls_changed as the last one end by calling _sub_windows_update. Both works, but I'm not sure which is the best.

I suppose it doesn't matter. The latter does more under the hood, so it might be more correct at the cost of performance, but you don't change translation very often anyways.

I tested the changes both in the editor and exported project and all seem to work fine.

@akien-mga
Copy link
Member

This likely breaks editor translations. Try to run make update and make merge in editor/translations/ after this PR and it will likely remove all translations for those strings. RTR() is what is used to extract strings to translate in the template.

@KoBeWi
Copy link
Member

KoBeWi commented Mar 12, 2021

Maybe we could add a new macro that fetches strings, but doesn't translate?

@KoBeWi
Copy link
Member

KoBeWi commented Mar 12, 2021

Seems like this is what TTRC does, but judging from the comment, it's not the intended use.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually TTRC is fine. So replace the removed RTRs with TTRCs.

@fabriceci fabriceci force-pushed the fix-dialog-translation branch from 08b7ff8 to 697c594 Compare March 12, 2021 16:51
@fabriceci
Copy link
Contributor Author

fabriceci commented Mar 12, 2021

@akien-mga thanks, I was unaware of this behaviour.

@KoBeWi thanks for the review and providing me a solution! Changes are done.

By curiosity, how I can see what functions are used for generating translations? (For example, is the tr() function is used too?).

@KoBeWi
Copy link
Member

KoBeWi commented Mar 12, 2021

By curiosity, how I can see what functions are used for generating translations? (For example, is the tr() function is used too?).

They are defined here:

patterns = ['RTR("', 'TTR("', 'TTRC("', 'TTRN("', 'RTRN("']

@akien-mga akien-mga merged commit 4bacb86 into godotengine:master Mar 22, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

This could be useful to backport to the 3.x branch, but it would need a dedicated PR and testing as Window is new in master, for 3.x it would likely need changes in the Dialog implementation itself.

@fabriceci fabriceci deleted the fix-dialog-translation branch August 11, 2021 19:30
@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jun 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants