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

Use "X" and "Y" instead of "E/W" and "N/S". Closes #3337. #3340

Merged
merged 1 commit into from Oct 19, 2020

Conversation

morevnaproject
Copy link
Contributor

@morevnaproject morevnaproject commented Jun 10, 2020

Closes #3337.

@RodneyBaker
Copy link
Collaborator

Correct me if I am wrong but this is more a cosmetic change right?
Expressions still need to refer to the old east/west and north/south terms (ex: col1.ew and not col1.x).

@morevnaproject
Copy link
Contributor Author

Yes, this is cosmetic change.

I think we shouldn't change expressions, as this make old scripts incompatible.

Actually, it would be nice to make modification so OT will understand both col1.ew and col1.x, but I would like to know what people think about this idea.

@RodneyBaker
Copy link
Collaborator

Actually, it would be nice to make modification so OT will understand both col1.ew and col1.x, but I would like to know what people think about this idea.

That would be nice.
I agree that changing the underlying expressions would be a breaking change not welcome by many older users and this would also break compatibility with Toonz and other older products.

Aside:
Perhaps one step more would be to give channels a 'title' that can be changed.
Then that title could be used in the expression (or the underlying object that is there currently).
Then long expressions could be shortened by referencing the title.
The 'title' could be 'col1.x', or 'BG.pan' but the object would remain 'col1.ew'
Yes, I agree this would be a complex undertaking.
Thus I'm not suggesting it be implemented... just taking the idea to a logical conclusion that allows for maximum customization and flexibility.

`

@gab3d
Copy link
Contributor

gab3d commented Jun 10, 2020

@morevnaproject
I strongly agree with all the scope of your proposal (both cosmetic and functional).
Of course compatibility must be preserved.

In fact I have changed E/W and N/S in favor of X and Y in the Spanish version of the UI since the very begining of the translation, as most people tend to be more familiar with the X and Y notation.

Copy link
Member

@shun-iwasawa shun-iwasawa left a comment

Choose a reason for hiding this comment

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

It's fine with us to change the terms "E/W" and "N/S" to "X" and "Y".
And as @RodneyBaker pointed, the expression should also be modified at the same time.

Actually, it would be nice to make modification so OT will understand both col1.ew and col1.x,

You can add x and y to the expressions by modifying the code of txsheetexpr.cpp as follows:

   TStageObject::Channel matchChannelName(const Token &token) const {
     std::string s = toLower(token.getText());
-    if (s == "ns")
+    if (s == "y" || s == "ns")
       return TStageObject::T_Y;
-    else if (s == "ew")
+    else if (s == "x" || s == "ew")
       return TStageObject::T_X;
     else if (s == "rot" || s == "ang" || s == "angle")
       return TStageObject::T_Angle;

toonz/sources/toonzlib/tstageobject.cpp Outdated Show resolved Hide resolved
toonz/sources/translations/french/tnztools.ts Outdated Show resolved Hide resolved
toonz/sources/toonzqt/functiontreeviewer.cpp Outdated Show resolved Hide resolved
@morevnaproject
Copy link
Contributor Author

You can add x and y to the expressions by modifying the code of txsheetexpr.cpp as follows:

Thank you! Will do that. ^__^

@manongjohn
Copy link
Collaborator

FYI... Put "Closes #3337" in the PR description so it will close that PR automatically when merged. I don't think it does that from the PR's title.

@morevnaproject
Copy link
Contributor Author

I have done all changes. Please review. ^__^

Copy link
Member

@shun-iwasawa shun-iwasawa left a comment

Choose a reason for hiding this comment

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

Sorry for the many comments, but I marked all points I could found.

Note that you also need to lrelease the translation file (i.e. making.qm files from updated .ts files and overwriting the old ones in stuff/config/loc/) to actually update the translated UI texts in the software. Please let me know if it's unfamiliar to you.

toonz/sources/translations/french/tnztools.ts Outdated Show resolved Hide resolved
toonz/sources/translations/french/tnztools.ts Outdated Show resolved Hide resolved
toonz/sources/translations/french/tnztools.ts Outdated Show resolved Hide resolved
toonz/sources/translations/french/tnztools.ts Outdated Show resolved Hide resolved
toonz/sources/translations/italian/tnztools.ts Outdated Show resolved Hide resolved
toonz/sources/translations/italian/tnztools.ts Outdated Show resolved Hide resolved
toonz/sources/translations/italian/tnztools.ts Outdated Show resolved Hide resolved
toonz/sources/translations/italian/tnztools.ts Outdated Show resolved Hide resolved
toonz/sources/translations/italian/tnztools.ts Outdated Show resolved Hide resolved
@RodneyBaker
Copy link
Collaborator

As the suggested changes at this point all relate to translations perhaps someone on the translation team can give this PR an assist.

@morevnaproject
Copy link
Contributor Author

@RodneyBaker Thank you for reminder! I am going to make fixes now. ^__^

@morevnaproject
Copy link
Contributor Author

Done! ^__^

@morevnaproject
Copy link
Contributor Author

Note that you also need to lrelease the translation file (i.e. making.qm files from updated .ts files and overwriting the old ones in stuff/config/loc/) to actually update the translated UI texts in the software. Please let me know if it's unfamiliar to you.

Yes, I am not sure how to do that properly. I am aware on how to run lrelease to generate .qm files one-by-one. But maybe there is some script to update all qm files automatically?

@shun-iwasawa
Copy link
Member

Jenkins

@shun-iwasawa
Copy link
Member

Sorry for the long silence! Thank you @morevnaproject for the modification.
Releasing .qm files can be done in the future in separate PR, probably together with updating translations of other terms.
LGTM

@shun-iwasawa shun-iwasawa merged commit c7649c4 into opentoonz:master Oct 19, 2020
@RodneyBaker RodneyBaker mentioned this pull request Nov 21, 2020
5 tasks
@morevnaproject morevnaproject deleted the upstream-pr1 branch December 29, 2020 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use "X" and "Y" instead of "E/W" and "N/S"
5 participants