-
Notifications
You must be signed in to change notification settings - Fork 543
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
Can't go to Manage Settings with the recent changes made #3145
Comments
I am guessing this is related to #3125 - I will take a closer look |
I cannot reproduce this issue in any of my 4.0.2 installations... is there something different about the way you are invoking Module Settings in your own theme? |
Nothing special was working up until I pulled the latest changes |
@sbwalker / @leigh-pointer can you create a new theme using a template follow the steps and let me now if this I happening you ? |
@sbwalker This problem doesn't occur in 4.0.1, because I create my own ModuleTitle as described above the INotifyPropertyChanged is fired and can't resolve all my themes have their own custom ModuleTitle |
Ok so the difference between your scenario and a default install is that you created a custom module title component which you are using in a custom theme. - which explains why I could not reproduce it locally. The ability to SetModuleTitle() using the ModuleBase method has been in the framework for a long time... it just was not being utilized in many places. In 4.0.2 code was added to allow for localization of module titles - however I do not understand how this could have resulted in the behavior you are seeing. More research is needed. |
@leigh-pointer an alternative way of handling localization for ControlTitles would have been to modify the OnParametersSet() method in ModuleTitle.razor and use the SharedLocalizer (similar to how it is used for ModuleTitles). The localization values would obviously need to be included in SharedResources.resx to make this work. This is a simpler method and would provide more consistency with the ModuleTitles.
However for third party modules, they do not have the ability to add resources to SharedResources.resx - so the technique you used with SetModuleTitle() would be the only option in that case. I am trying to determine why @vnetonline is having issues in his custom theme/components. @vnetonline is it only the Module Settings UI which is throwing errors for you, or are other UIs which were modified to use this approach in 4.0.2 also throwing errors (ie. Module Import/Export, Html/Text Edit, Profile Management Add, etc...) ? |
Only module settings when I click on manage settings |
@vnetonline you have tried some of the other UIs I mentioned while using your custom theme/moduletitle component? |
Yeah I have the other modules it doesn't break because they use the built in ModuleTile this only happens when I have my own AmazingModuleTitle You can reproduce this if you create a new theme Then just copy the code from ModuleTitle code and make your own control using this code called AmazingModuleTitle .... you will see the error I am talking about |
If you use that container with AmazingModuleTitle as the default container you will see it happening on every module when you click on manage settings |
@sbwalker modules can have their own shared resource resx file at the root of the module exactly the same as Oqtane. |
@sbwalker Having read a lot about dynamic types I have found this https://heartysoft.com/ashic/blog/2010/05/anonymous-types-c-sharp-4-dynamic/ So since my Theme is an external module and because what's being inserted into the PropertyDirctory is an anonymous object |
@sbwalker To resolve the issue i have had to use good old fashioned reflection private void PropertyChanged(object sender, PropertyChangedEventArgs e)
{
if (e.PropertyName == "ModuleTitle")
{
var properties = (PropertyDictionary)SiteState.Properties;
var obj = properties["ModuleTitle"];
System.Type type = obj.GetType();
int pageModuleId = (int)type.GetProperty("PageModuleId").GetValue(obj, null);
string pageModuleTitle = (string)type.GetProperty("Title").GetValue(obj, null);
if (pageModuleId == ModuleState.PageModuleId)
{
title = pageModuleTitle;
StateHasChanged();
}
}
} But I am sure there is a better way Correct me if i am wrong ... I think the Theme and Modules are in external assemblies. I think they can't use the [Dot] approach. let me know if you think otherwise. |
@vnetonline could resolve without!
|
much more elegant solution then mine thanks @leigh-pointer |
What using statements are you using @leigh-pointer |
@leigh-pointer what is PropertyDictionaryEntry in your solution |
@vnetonline Sorry private library. Try this:
|
@leigh-pointer same binder problem |
private void PropertyChanged(object sender, PropertyChangedEventArgs e)
} |
Nope that doesn't work either |
I think we should be using ExpandoObject() instead of anonymous object because they can be shared across assemblies |
@vnetonline thank you for finding that link about anonymous types. This would explain why it is impossible to reproduce this problem except for in a very specific scenario (ie. consuming an event raised by the core framework using an anonymous type from a different razor class library). Note that this is not a problem if you create a module which raises its own events and consumes them from different components within the same razor class library. It is also not a problem if the events which are raised use public types rather than anonymous types - regardless of where they are raised or consumed. So this is truly an edge case. That being said, it is still worthwhile to provide a solution. Essentially there are 2 options:
|
I am also considering rolling back #3125 and using a different approach for localization for ModuleTitle |
@leigh-pointer I am going to explore LocalizerFactory as an option. It was added to the framework a while ago for scenarios where localization values may exist in a RESX file which does not correspond to the RAZOR file. For example, the ModuleTitle.razor component has a corresponding ModuleTitle.resx - however this is not where you would want to store the localization values, as a ModuleTitle is actually related to a specific type of module. So the LocalizerFactory allows you to specify an explicit Type ie. you can specify that the ModuleTitle needs to dynamically get its localization values from a RESX file for the current Module. This seems to be what we want in this situation - we want to localize the ModuleTitle which is assigned by the Title property in a module component - but we want the localization values to be specified in a RESX associated to the module component ie. Edit.razor and Edit.resx |
@sbwalker this sounds very interesting |
@sbwalker no worries happy to help .... I like option 2 because it is exactly what we need .... for example in my custom module template I have a main heading and a subheading using localizer factory would allow localizing both headings. Look forward to rolling back #3125 thanks for your help @leigh-pointer |
@leigh-pointer I found a much more centralized way to handle this.... in the SiteRouter I can specify:
which localizes the ControlTitle value based on the RESX file associated to the current module component. Basically this means that a module developer would not need to implement anything in their module... they could set the property Title = "xxx" and then in the RESX file for their component they would add a key of "xxx" and a value of "yyy". This eliminates the need to raise events between components - so I think it is a better approach. I am going to investigate if ModuleTitle could be handled in the same way (as opposed to relying on the framework SharedResources.resx) |
That's even cooler |
I thought I would try my idea of expando Object and it works across assemblies. I changed As you can see below This would not be a breaking change right @sbwalker ? |
@vnetonline please go ahead and submit a PR for the ExpandoObject change. |
Anonymous Object are not able to be used across assemblies however ExpandoObject is refer to oqtane#3145
@sbwalker pull request #3154 submitted, thank you to both @sbwalker & @leigh-pointer |
@leigh-pointer there are 2 possible options where module component Title can be localized using LocalizerFactory: SiteRouter or ModuleTitle component. #3155 implements it in SiteRouter but upon further consideration I believe that localization should only be performed when content is being displayed in the UI. Implementing it in SiteRouter means that it alters the value of a static internal setting... which means that if a developer had used conditional logic such as if (ModuleState.ControlTitle == "xxx") it would fail. So I am now thinking that this should be implemented in the ModuleTitle component (where the title is displayed). Note that this would not change the approach for including the localization key/value in the module component's RESX - it would only change where the value is translated. Do you agree with my logic? The impact of this is that @vnetonline would need to modify his custom ModuleTitle component to include the LocalizerFactory logic - but that is an edge case scenario so I believe its acceptable. Edit: #3156 migrates the logic from SiteRouter to ModuleTitle |
I am okay to change my logic but I not clear on how I would do it with the change you propose. |
@vnetonline see #3156 and the changes to ModuleTitle.razor - the changes are very simple |
Okay got it but what will be the resx file value and in which resx file ?? |
As an example, Oqtane.Client\Modules\Admin\Files\Edit.razor has a Title parameter specified of "Folder Management". There is a corresponding Oqtane.Client\Resources\Modules\Admin\Files\Edit.resx with an item which contains a Name of "Folder Management" (to match the Title parameter value) and a Value of "Folder Management" - obviously the Value is what would be changed if you are including a localization. This is using the standard .NET Core localization approach. Note that the Title parameter is only used on non-Index components. Index components use the user defined module title which can be specified in Module Settings - which is user defined content - not static content - so it is currently not localized. |
Yup got it thank you for the explanation @sbwalker |
This is going to be a lot better, so yes I agree. |
Note that the Title parameter is only used on non-Index components. Index components use the user defined module title which can be specified in Module Settings - which is user defined content - not static content - so it is currently not localized. The exception is Admin modules which currently store their localization keys in SharedResources.resx (which is not available to module developers). An enhancement could be made so that module titles for Index components are translated in a similar way as outlined above for Title parameters. This means that localization keys would need to be moved to the RESX corresponding to the RAZOR file (ie. moved out of SharedResources.resx) - which would be more consistent with other localization areas and would work for non-Admin modules as well (but essentially a breaking change for translators as they would need to update their RESX files). The other challenge is that if a user changed a module title dynamically, the key lookup would fail - but this would be a "known limitation". |
@sbwalker & @leigh-pointer
I am having an issue where i can't go to Manage Settings using my own theme which i have been working on I get an error
'object' does not contain a definition for 'PageModuleId'
the culprit is in ModuleTitle.razor
The text was updated successfully, but these errors were encountered: