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

modify several module's SettingDefinitionProvider to support multi-lingual #2392

Merged
merged 7 commits into from
Dec 27, 2019
Merged

modify several module's SettingDefinitionProvider to support multi-lingual #2392

merged 7 commits into from
Dec 27, 2019

Conversation

yinchang0626
Copy link
Contributor

configure embedded resources by wildcard and rebase from latest dev branch
@maliming please help to add to Milestone 1.2
#2332

@hikalkan hikalkan added this to the 1.2 milestone Dec 14, 2019
@hikalkan hikalkan requested a review from maliming December 14, 2019 12:57
@hikalkan
Copy link
Member

Thank you for your great contribution.
One problem I see that DisplayName:Abp.Localization.DefaultLanguage is added to the ValidationResource which is irrelevant.

@maliming you can merge it after reviewing, then create an LocalizationModuleResource (or find a better naming) and move this definition to there.

@hikalkan hikalkan self-requested a review December 14, 2019 13:00
@yinchang0626
Copy link
Contributor Author

@hikalkan
You are right,
i think create a new resourceName in Volo.Abp.Localization is better,
Now Volo.Abp.Localization module just have AbpValidationResource and maybe that should be move to Volo.Abp.Validation.
I can try to do it in new pull request

@maliming
Copy link
Member

@yinchang0626 You can continue to commit new code on this branch without reopening pr.

@yinchang0626
Copy link
Contributor Author

yinchang0626 commented Dec 15, 2019

@hikalkan @maliming
I am going to move AbpValidationResource from Volo.Abp.Localization to Volo.Abp.Validation Module,

namespace Volo.Abp.Localization.Resources.AbpValidation
{
    //TODO: Move to Volo.Abp.Validation!

    [LocalizationResourceName("AbpValidation")]
    public class AbpValidationResource
    {
        
    }
}

and let AbpValidationModule Depends AbpValidationModule

    [DependsOn(
        typeof(AbpLocalizationModule)
        )]
    public class AbpValidationModule : AbpModule
    {
        public override void PreConfigureServices(ServiceConfigurationContext context)
        {
            Configure<AbpVirtualFileSystemOptions>(options =>
            {
                options.FileSets.AddEmbedded<AbpValidationModule>();
            });

            Configure<AbpLocalizationOptions>(options =>
            {
                options.Resources
                    .Add<AbpValidationResource>("en")
                    .AddVirtualJson("/Volo/Abp/Validation/Localization/Resource");
            });
......

but after moved,will cause breaking change,because of all other modules and template(app,module) is depend Volo.Abp.Localization for

            Configure<AbpLocalizationOptions>(options =>
            {
                options.Resources
                    .Add<IdentityResource>("en")
                    .AddBaseTypes(
                        typeof(AbpValidationResource)
                    ).AddVirtualJson("/Volo/Abp/Identity/Localization");
            });

so exised module need to modify to
image
Could I change it like this for all modules?
or just add a LocalizationModuleResource for DisplayName:Abp.Localization.DefaultLanguage in Volo.Abp.Localization??

options.Resources
.Add<EmailingResource>("en")
.AddBaseTypes(
typeof(EmailingResource)
Copy link
Member

Choose a reason for hiding this comment

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

Why does EmailingResource inherit EmailingResource(AddBaseTypes)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thinks,I fixed it

@wakuflair
Copy link
Contributor

How about defining setting localization resources in just one resource, (e.g., "SettingResource")?
In that case, it's convenient to show the localizaed string for all the settings.
(It is what my module is doing.)

@yinchang0626
Copy link
Contributor Author

@wakuflair
of course,put all in one resource is a way and works fine,
but maybe add new module in future,then have to modify "SettingResource",I think module should not be influenced by others module.
So setting value is defined in independent module,that "DisplayName" and "Description" should be follow it.

@wakuflair
Copy link
Contributor

wakuflair commented Dec 16, 2019

No need to modify, the future module could inherit "SettingResource", or just add its own resources by using the AddVirtualJson method
It's very reasonable for the usage of settings.

@yinchang0626
Copy link
Contributor Author

@wakuflair
Any way, thanks for your module
SettingDefinition already have a Constructor which with displayName,description params.
Why not define it in first(framework) ?

    internal class EmailSettingProvider : SettingDefinitionProvider
    {
        public override void Define(ISettingDefinitionContext context)
        {
            context.Add(
                new SettingDefinition(EmailSettingNames.Smtp.Host, "127.0.0.1", L("DisplayName:Abp.Mailing.Smtp.Host"), L("Description:Abp.Mailing.Smtp.Host")),
                new SettingDefinition(EmailSettingNames.Smtp.Port, "25", L("DisplayName:Abp.Mailing.Smtp.Port"), L("Description:Abp.Mailing.Smtp.Port")),
                new SettingDefinition(EmailSettingNames.Smtp.UserName, displayName: L("DisplayName:Abp.Mailing.Smtp.UserName"), description: L("Description:Abp.Mailing.Smtp.UserName")),
                new SettingDefinition(EmailSettingNames.Smtp.Password, displayName: L("DisplayName:Abp.Mailing.Smtp.Password"), description: L("Description:Abp.Mailing.Smtp.Password"), isEncrypted: true),
                new SettingDefinition(EmailSettingNames.Smtp.Domain, displayName: L("DisplayName:Abp.Mailing.Smtp.Domain"), description: L("Description:Abp.Mailing.Smtp.Domain")),
                new SettingDefinition(EmailSettingNames.Smtp.EnableSsl, "false", L("DisplayName:Abp.Mailing.Smtp.EnableSsl"), L("Description:Abp.Mailing.Smtp.EnableSsl")),
                new SettingDefinition(EmailSettingNames.Smtp.UseDefaultCredentials, "true", L("DisplayName:Abp.Mailing.Smtp.UseDefaultCredentials"), L("Description:Abp.Mailing.Smtp.UseDefaultCredentials")),
                new SettingDefinition(EmailSettingNames.DefaultFromAddress, "[email protected]", L("DisplayName:Abp.Mailing.DefaultFromAddress"), L("Description:Abp.Mailing.DefaultFromAddress")),
                new SettingDefinition(EmailSettingNames.DefaultFromDisplayName, "ABP application", L("DisplayName:Abp.Mailing.DefaultFromDisplayName"), L("Description:Abp.Mailing.DefaultFromDisplayName"))
            );
        }
        private static LocalizableString L(string name)
        {
            return LocalizableString.Create<EmailingResource>(name);
        }
    }

and then EmailingResource is import SettingResource by AddVirtualJson or not.
I follow the Abp team's ideas

@wakuflair
Copy link
Contributor

wakuflair commented Dec 17, 2019

SettingDefinition already have a Constructor which with displayName,description params.

Yeah I prefer using displayName and description parameters. Once this PR is accepted, I will update my module to use them.

and then EmailingResource is import SettingResource by AddVirtualJson or not.

I can't see the SettingResource, it's what you plan to commit?

@yinchang0626
Copy link
Contributor Author

@wakuflair
I do not have plan to add SettingResource now.
In this PR. Just modify SettingDefinition to add define of displayName and description.
and going to do things is @hikalkan 's commit

namespace Volo.Abp.Localization.Resources.AbpValidation
{
    //TODO: Move to Volo.Abp.Validation!

    [LocalizationResourceName("AbpValidation")]
    public class AbpValidationResource
    {
        
    }
}

but I think after moved ,will cause breaking change

@yinchang0626
Copy link
Contributor Author

@hikalkan
please review this commit 3ba06fe
I move AbpValidationResource to Volo.Abp.Validation from Volo.Abp.Localization.
But still stay Volo.Abp.Localization.Resources.AbpValidation.AbpValidationResource file
for template and module still can reference it to AddBaseTypes in definition of localization resource.
And mark it as obsolete ,maybe future version will remove it,
finally,as you say ,
I put DisplayName:Abp.Localization.DefaultLanguage to new AbpLocalizationResource.
Could you check it alright?thanks

@hikalkan hikalkan merged commit ffdb8ac into abpframework:dev Dec 27, 2019
@yinchang0626 yinchang0626 deleted the feature/setting_lang branch January 21, 2020 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants