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

solve internationalization problems when using ctk dicom widget with languages other than english #813

Closed
wants to merge 4 commits into from

Conversation

CodingFatman
Copy link

the modifications below aim to solve the internationalization problem when people who want to use different language to display information when using CTK DICOM Library

the details are described as below:
1、ctkDicomTableView
add a new private member QLabel to store the internal use of queryTableName, use the original QLabel just to display the table view's caption on ui.
add function setDisplayText to set the table view's caption on ui
add a function applyHeaderTextList to supply a way to use different language formating the table column title

2、ctkDICOMQueryRetrieveWidget
set the ctkDICOMQuery Parameters with QApplication::translate() result to solve the internationalization problem

3、ctkDICOMServerNodeWidget
modify addServerNode function to set the table widget column title with QApplication::translate() result to solve the internationalization problem
modify readSettings function to set the default server node parameters with QApplication::translate() result to solve the internationalization problem

4、ctkDICOMTableManager
modify init function, add function call of setDisplayText to patientsTable,studiesTable and seriesTable to format the table view caption with different language

@pieper pieper requested review from jcfr and nolden June 22, 2018 13:01
Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! I'm not a internationalization expert but we do want this code to behave well in non-English settings so let's try to get this approved and integrated.

I will add some comments on style.

Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

Another request: it would be good to add working internationalization to the demo app for ctkDICOM. So some translation files that would work with these changes would also be needed. That way people will know that this feature exists and that it doesn't break the existing functionality.

query->setHost(parameters["Address"].toString());
query->setPort(parameters["Port"].toInt());
query->setPreferCGET(parameters["CGET"].toBool());
query->setCalledAETitle(parameters[QApplication::translate("ctkDICOMServerNodeWidget", "AETitle", 0)].toString());
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this simpler with just tr("AETITLE") ?

Copy link
Author

Choose a reason for hiding this comment

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

ok,i will add the translation files to the demo app.
I use tr at first, but it seems can't work well. I will try again

Copy link
Author

Choose a reason for hiding this comment

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

after test, I have found that tr() will set the context with wrong one, it will cause the translation file can not be used. so i still use QApplication::translate

@@ -57,7 +59,10 @@ class ctkDICOMTableViewPrivate : public Ui_ctkDICOMTableView
QStringList currentSelection;
//Key = QString for columns, Values = QStringList
QHash<QString, QStringList> sqlWhereConditions;

private:
QLabel lblqueryTableName;
Copy link
Member

Choose a reason for hiding this comment

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

Better to match naming conventions, so this could be: labelQueryTableName

Copy link
Author

Choose a reason for hiding this comment

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

ok,i will modify it.

for(int i = 0; i < dicomSQLModel.columnCount(); i++)
{
QVariant fieldName = dicomSQLModel.headerData(i, Qt::Horizontal, Qt::DisplayRole);
QString headerText = QApplication::translate("ctkDICOMTableView", fieldName.toString().toStdString().c_str(), 0);
Copy link
Member

Choose a reason for hiding this comment

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

STYLE: indentation should match

Copy link
Author

Choose a reason for hiding this comment

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

ok, I use sublime text to modify the code, so the indentation is strange, I will modify it .

@@ -226,7 +250,15 @@ void ctkDICOMTableView::setDicomDataBase(ctkDICOMDatabase *dicomDatabase)
void ctkDICOMTableView::setQueryTableName(const QString &tableName)
{
Q_D(ctkDICOMTableView);
d->lblTableName->setText(tableName);
//d->lblTableName->setText(tableName);
Copy link
Member

Choose a reason for hiding this comment

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

STYLE: remove dead code

Copy link
Author

Choose a reason for hiding this comment

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

OK

@@ -146,7 +155,22 @@ void ctkDICOMTableViewPrivate::hideUIDColumns()
//----------------------------------------------------------------------------
QString ctkDICOMTableViewPrivate::queryTableName() const
{
return this->lblTableName->text();
//return this->lblTableName->text();
Copy link
Member

Choose a reason for hiding this comment

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

STYLE: remove dead code
STYLE: change to labelTableName (I see this was already there and you were being consistent but it would be good clean this up while we are working on this code)

Copy link
Author

Choose a reason for hiding this comment

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

OK

@CodingFatman
Copy link
Author

@pieper request changes have been modified

@CodingFatman
Copy link
Author

@pieper all request have been finished

@lassoan
Copy link
Member

lassoan commented Jan 11, 2023

We have received grant funding for translation of Slicer to French and Spanish, so we would like to revive and integrate this pull request. We'll set up crowdsourcing translation on Weblate.

One change that I would consider making is to generate one single .ts file for entire CTK instead of one .ts file for each component (ctkDICOM2, ctkDICOMQueryRetrieve, etc.). Having less .ts files would make managament of translations a bit easier: we would have one .ts file for each major component (Slicer, CTK, and for each extension, about a total of 100) for each language - a total of 1000-2000 files. If we had a .ts file for each component then we could end up with tens of thousands of very small .ts files.

@pieper @mhdiop @nolden

@pieper
Copy link
Member

pieper commented Jan 11, 2023

Yes, now that we have some resources more experience with the translation options we can revisit this code and get it merged.

@nolden let us know if there are considerations for internationalization of MITK we should take into account for this.

@kislinsk
Copy link
Contributor

@pieper Thanks! :) Nothing to consider regarding i18n and MITK.

@jcfr
Copy link
Member

jcfr commented Apr 13, 2023

@jcfr jcfr closed this Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants