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.

Sign up for GitHub

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

Jump to bottom

ENH: Make Tables module translatable (SlicerLanguagePacks#12) #7283

Merged
merged 2 commits into from
Dec 6, 2023
Merged

ENH: Make Tables module translatable (SlicerLanguagePacks#12) #7283

merged 2 commits into from
Dec 6, 2023

Conversation

Papa96108
Copy link
Contributor

No description provided.

@Papa96108
Copy link
Contributor Author

Hi @lassoan I'm waiting for the merge to finalise my thesis.

@Papa96108 Papa96108 changed the title BUG: mark translatable string see ->SlicerLanguagePacks/issues #12 BUG: Mark translatable string see ->SlicerLanguagePacks/issues #12 Oct 23, 2023
@Papa96108 Papa96108 force-pushed the table-module-internationalization branch 2 times, most recently from 36e6f6a to aba750f Compare October 24, 2023 12:20
@jcfr jcfr changed the title BUG: Mark translatable string see ->SlicerLanguagePacks/issues #12 BUG: Mark translatable string see ->SlicerLanguagePacks#12 Oct 25, 2023
jcfr
jcfr requested changes Oct 25, 2023
View reviewed changes
Copy link
Member

@jcfr jcfr left a comment

Choose a reason for hiding this comment

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

Overall the changes look good.

Before integrating, I suggest to have two commits:

  • ENH: Improve TableNode API for column title
  • ENH: Make Table module translatable
    ENH: Make SegmentStatistics measurement translatable`

Questions

  • Will the update changing SCHEMA_COLUMN_LONG_NAME to SCHEMA_COLUMN_TITLE impact loading of table saved using the older column name ?

Notes

When referencing issues outside of the Slicer project make sure to use the format Organization/Repository#IssueNumber, failure to do so is referencing issue in the "wrong" project.

See more details at https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/autolinked-references-and-urls

Libs/MRML/Core/vtkMRMLTableNode.cxx Outdated Show resolved Hide resolved
Libs/MRML/Core/vtkMRMLTableNode.cxx Outdated Show resolved Hide resolved
@jcfr jcfr changed the title BUG: Mark translatable string see ->SlicerLanguagePacks#12 BUG: Mark translatable string see -> SlicerLanguagePacks#12 Oct 25, 2023
@Papa96108 Papa96108 requested a review from jcfr October 25, 2023 15:47
@Papa96108
Copy link
Contributor Author

Hi @jcfr thanks for your suggestion should I rewrite my commit message for commit message you suggest with git rebase?

@Papa96108
Copy link
Contributor Author

for your question I share with you the issue for this commit #7217 I think that it wouln't impact the old saved table because i replace all column name by title but i will check it

@Papa96108 Papa96108 force-pushed the table-module-internationalization branch from 2aa584f to c0e10c4 Compare October 25, 2023 16:09
@Papa96108
Copy link
Contributor Author

@jcfr please run the workflow

@Papa96108
Copy link
Contributor Author

The revert commit message always causes the check commit message workflow to fail can you help me find a solution i've already used git rebase to change the commit message without success.

0d06e8d.
@lassoan
@pieper
@jcfr
@mhdiop

@mhdiop
Copy link
Contributor

mhdiop commented Oct 28, 2023

Thank you @Papa96108 for working on this issue.

We can have a Meet tonight to discuss about the difficulties you are facing.

Regards.

@Papa96108
Copy link
Contributor Author

Thank you @Papa96108 for working on this issue.

We can have a Meet tonight to discuss about the difficulties you are facing.

Regards.

Thanks you @mhdiop see you tonight

@Papa96108 Papa96108 force-pushed the table-module-internationalization branch from 846e26f to 4f32f3e Compare November 3, 2023 19:15
@jcfr jcfr added the Domain: i18n Internationalization (language translation) label Nov 11, 2023
@jcfr jcfr changed the title BUG: Mark translatable string see -> SlicerLanguagePacks#12 ENH: Make Tables module translatable (SlicerLanguagePacks#12) Nov 21, 2023
@jcfr jcfr linked an issue Nov 21, 2023 that may be closed by this pull request
Make table column title translatable #7217
Closed
@lassoan lassoan force-pushed the table-module-internationalization branch 3 times, most recently from 3d35ada to d3cd916 Compare November 29, 2023 15:24
@Papa96108 @lassoan
BUG: Make table columns translatable
dc87804 
Table column name (non-translatable) and title (displayed, translatable) can now be set separately.
This allows displaying tables in any language while allowing modules to retrieve columns of tables using language-independent string identifiers.

Updated Segment Statistics module to make all plugins and measurements translatable.

To reduce complexity of the Tables module API, column "title" has replaced "long name". Long name was barely used (if at all) and its role would have been unclear (since we have name, title, description columns already).
Improved appearance of table column tooltips (reordered, improved formatting, added display of all custom properties)., use acronym of plugin instead of (1), (2), ... for distinguishing columns that would have had the same name.

see Slicer#7217
@lassoan lassoan force-pushed the table-module-internationalization branch from d3cd916 to dc87804 Compare November 29, 2023 15:58
@lassoan
Copy link
Contributor

lassoan commented Nov 29, 2023

I've reviewed and fixed up the code, it all looks good now. Modules that provide translatable column name can enable UseColumnTitleAsColumnHeader to display that in the header (instead of displaying the column name).

  • Will the update changing SCHEMA_COLUMN_LONG_NAME to SCHEMA_COLUMN_TITLE impact loading of table saved using the older column name ?

I've added ReadLongNameAsTitle flag to allow using longName as title, but since often longName is too long for title, it is disabled by default. The only known use of longName is in Segment Statistics result tables, and there longName is too long.

@lassoan
Copy link
Contributor

lassoan commented Nov 30, 2023

@jcfr You have requested changes earlier, please have a look and approve if it looks good to you.

@lassoan lassoan merged commit d369841 into Slicer:main Dec 6, 2023
5 checks passed
@jcfr
Copy link
Member

jcfr commented Dec 6, 2023

@lassoan Would it be sensible to integrate this one into 5.6.1 ?

@lassoan
Copy link
Contributor

lassoan commented Dec 6, 2023

Yes, I think so.

@jcfr jcfr added the backport:5.x Identify pull request expected to be backported to the current 5.x release branch. label Dec 6, 2023
This was referenced Dec 11, 2023
Closed
Merged
@jcfr jcfr removed the backport:5.x Identify pull request expected to be backported to the current 5.x release branch. label Dec 11, 2023
@lassoan lassoan mentioned this pull request Feb 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lassoan lassoan lassoan approved these changes

@jcfr jcfr Awaiting requested review from jcfr

Assignees
No one assigned
Labels
Domain: i18n Internationalization (language translation)
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make table column title translatable
4 participants
@Papa96108 @mhdiop @lassoan @jcfr