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

RadzenScheduler Culture override support #229

Merged
merged 2 commits into from
Sep 27, 2021

Conversation

ThnxDaniel
Copy link
Contributor

Enables complete localization of the scheduler component.

Related issues:

@@ -8,7 +8,9 @@
{
get
{
return Scheduler.CurrentDate.ToString("MMMM yyyy");
var sb = new System.Text.StringBuilder(Scheduler.CurrentDate.ToString("MMMM yyyy", Scheduler.Culture));
sb[0] = char.ToUpper(sb[0], Scheduler.Culture);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some cultures don't treat months as names, resulting in incorrect capitalization for this usecase, sv-SE is one of them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we leave it as it is per the Culture rules then? If a culture does not capitalize its month names we shouldn't do that as well.

Copy link
Contributor Author

@ThnxDaniel ThnxDaniel Sep 27, 2021

Choose a reason for hiding this comment

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

Sure, but in this case its also the start of a heading/sentence, which is why it should be capitalized even though the months aren't treated as names.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove that capitalization from the code. If needed it can be achieved with CSS.

@akorchev
Copy link
Collaborator

Could you elaborate on the need to have a different Culture per component? As far as I know all string methods use the CurrentUICulture anyway.

@ThnxDaniel
Copy link
Contributor Author

Could you elaborate on the need to have a different Culture per component? As far as I know all string methods use the CurrentUICulture anyway.

In my case I need to override the culture, which is not possible by simply changing the value of CurrentUICulture

@akorchev
Copy link
Collaborator

Have you tried the way described in theBlazor documentation?

By uncommenting the localization code in the Radzen sample application and changing "de-DE" to "bg-BG' I was able to localize the Scheduler in Bulgarian without any other changes:

image

We believe this provides a robust way to implement localization at application level. There isn't much need to add a Culture property at component level.

@javiercampos
Copy link
Contributor

javiercampos commented Sep 27, 2021

@akorchev I'm commenting here because you closed my thread, but that way of doing localization is problematic (and it's indeed found problematic by the Blazor team, they are targeting NET 7.0 to address it). Blazor server has a shared server session context, so actions from one user (in one culture) may affect data that updates the UI on other use (which may use a different culture according to the localization set on the http connection, and not on the signalr circuit).

So even if you can reload and show the main rendering in the culture set by the middleware (which again, is not invoked on the SignalR circuit), say one user with the "de-DE" culture set as its current updates the actual data on the scheduler (to cite the example you put here), so it forces a rendering on all users showing that on screen... no matter the actual middleware-based culture for those other users, anything rendered/updated will have the "de-DE" culture (and will show with the german culture to bulgarian users): this includes date formatting or localized strings (if you use the IStringLocalizer).

Using the ASP.NET localization middleware for setting the Blazor Server culture is just a very bad idea for many reasons (note that also CurrentUICulture is not kept between async calls on the blazor synchronization context either), and until either Blazor has a more suitable method of setting the culture for either the components or the actual SignalR circuit, a workaround is needed.

I have that workaround for my own components, but I just cannot do it for third-party components in a generalized way (without having to derive from -all- components), whereas it would be "relatively easy" to do for you on the base components (as this PR shows).

Related:

@akorchev
Copy link
Collaborator

I am not sure why you think this way of doing localization is problematic as it the the official one mentioned in the Blazor documentation. We have been using it in all Radzen Blazor applications since 2019 and we haven't seen the issues you are describing. Still I do agree that if you don't want to change the culture via the official means you will encounter problems. One has to reload the application (via HTTP request) to set the current culture. This should reestablish a new SignalR connection with the right culture (AFAIK).

Radzen applications (ones generated via the Radzen IDE) change the current culture by navigating to a controller.

            var redirect = new Uri(UriHelper.Uri).GetComponents(UriComponents.PathAndQuery | UriComponents.Fragment, UriFormat.UriEscaped);

            var query = $"?culture={Uri.EscapeDataString((string)args)}&redirectUri={redirect}";

            UriHelper.NavigateTo("Culture/SetCulture" + query, forceLoad: true);

The controller itself is this:

    [Route("[controller]/[action]")]
    public partial class CultureController : Controller
    {
        public IActionResult SetCulture(string culture, string redirectUri)
        {
            if (culture != null)
            {
                HttpContext.Response.Cookies.Append(
                    CookieRequestCultureProvider.DefaultCookieName,
                    CookieRequestCultureProvider.MakeCookieValue(new RequestCulture(culture)));
            }

            return LocalRedirect(redirectUri);
        }
    }

This approach should not cause a problem where one user changes the culture of a different user. Here is a test that I made - two users browser the same application with different cultures.
localization

@javiercampos
Copy link
Contributor

javiercampos commented Sep 27, 2021

@akorchev I made a very simple (based off the default blazor template in VS and adding the localization and your controller, I added radzen just to show off, but it's not needed):

https://github.com/javiercampos/CultureProblemSample

In particular: https://github.com/javiercampos/CultureProblemSample/blob/master/Pages/Index.razor

I have Chrome (on the left) set to English and Firefox (on the right) set to Spanish (although I made the controller so you can switch via a cookie with the exact code you are using). Look what happens when I update the date in Firefox (in Spanish), to the Chrome window (or vice-versa)

BpHKDgZh8F

When updating something in the Spanish browser which raises a "StateHasChanged" (even if Invoked on the component), the CurrentUICulture on the renderer for the "English browser" is still "Spanish" (because blazor server doesn't know about browsers, and doesn't maintain the culture for their Invocation).

Notice that if I was to dump an scheduler there, the moment I invoke "StateHasChanged()" because -other user with a different culture raised that event-, the whole component would be localized wrongly.

Of course this is a contrived example with a simple event, but I personally use domain bus events (locally via events like this or distributed using rabbitmq) to update data that might need a rerender (and the invocation is done from other user's circuit, or from the rabbitmq listener thread, which do not have its CurrentUICulture set by the middleware).

The fact that it doesn't happen in your particular use-case might be ok, but it doesn't mean "it's not a problem and it should not be addressed". Again, this is OSS and I'm not forcing anyone to do anything, but it should at least be looked up into, or disregarded as "we don't want to do it", but definitely not disregarding it as "it's not a problem" (because it is, and I have many use cases where it has been, and even if the localization middleware is the "right now" official way to do localization, it doesn't work for all cases as of now -again, the Blazor team is looking into it, but it won't be till at least NET 7 that it's fixed, and you need to work around it in the meantime-)

I have some workarounds for it (and again, the Blazor team needs to flow the CurrentUICulture from different invocations and/or the blazor renderer, but they don't as of now). Implementing that workaround on Radzen components would mean deriving off all components that need it, and again, as this PR shows, it's a relatively easy thing to do if you set it on your base classes.

@akorchev
Copy link
Collaborator

Can you reproduce the problem without using a static class? I have no idea how static classes (and events) behave in ASP.NET core environment - for example how they capture the current thread etc. It seems after commenting StaticData.UpdateDate(); everything behaves as expected - both browsers remain with the same culture.

By the way should the default value of Culture be CurrentCulture instead of CurrentUICulture? According to this SO answer this seems to be the case.

@akorchev
Copy link
Collaborator

@ThnxDaniel there seems to be a problem with the format in Day view (should display only hours - not it displays the date and time):

image

@ThnxDaniel
Copy link
Contributor Author

ThnxDaniel commented Sep 27, 2021

@ThnxDaniel there seems to be a problem with the format in Day view (should display only hours - not it displays the date and time):

Had forgotten to pass it along from the RadzenDayView, but should be fixed now

@akorchev
Copy link
Collaborator

@ThnxDaniel thanks! What do you think about the default value of the Culture property? Shouldn't it be CurrentCulture as per this and this?

@ThnxDaniel
Copy link
Contributor Author

@ThnxDaniel thanks! What do you think about the default value of the Culture property? Shouldn't it be CurrentCulture as per this and this?

Reading through the documentation, it's not entirely clear as to which one would be correct for the UI portion of a web app. But I've changed it to CurrentCulture to be consistent with the rest of the codebase

@javiercampos
Copy link
Contributor

Can you reproduce the problem without using a static class? I have no idea how static classes (and events) behave in ASP.NET core environment - for example how they capture the current thread etc. It seems after commenting StaticData.UpdateDate(); everything behaves as expected - both browsers remain with the same culture.

The fact that is "other thing updating the bound values other than a direct response to the UI in that same circuit" is the culprit. I used a static class and an event to illustrate the problem, but anything that forces a re-render (updating the bound data from a background service, from other user's circuit, etc) that doesn't happen in the thread (and I mean the thread... even doing invocations in an async method where the thread -not the synchronization context, because the blazor one doesn't capture the thread-) where the middleware set the culture in, will exhibit this problem. It's only not a problem if you don't do any kind of realtime updates and only respond to the UI events on the same signalr circuit as the user.

By the way should the default value of Culture be CurrentCulture instead of CurrentUICulture? According to this SO answer this seems to be the case.

CurrentUICulture should be preferred, but that also means having to specify it -anywhere- you render values. I myself have no preference, to me, Current Culture is always the same as the UI one, but the only reason of CurrentUICulture exists is to be used on UI paints

@akorchev
Copy link
Collaborator

@ThnxDaniel Thanks! AFAIK CurrentUICulture is mostly used for resx files and satellite assemblies. Date and number formatting should probably follow CurrentCulture (according to this and my prior experience. Still as @javiercampos mentions CurreentCulture and CurrentUICulture are set to the same thing in most cases.

I will remove the capitalization code in the month view and merge the pull request.

@akorchev
Copy link
Collaborator

@ThnxDaniel to capitalize the heading with CSS you can use the following:

.rz-scheduler-nav-title {
   text-transform: capitalize;
}

@akorchev akorchev merged commit 48415a4 into radzenhq:master Sep 27, 2021
@javiercampos
Copy link
Contributor

Taken this has been merged, can we reopen #206 (since it's a problem for more components than the scheduler) and I may try to make some time to do a PR myself if noone else is interested?

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.

3 participants