RNA reference mapping: generate manual prefix based on current lang #104491

Damien Picard · 2023-02-09T00:21:37+01:00

Damien Picard commented

2023-02-09 00:21:37 +01:00

The manual url prefix was generated statically, using the current
language from the preferences. This led to the rna_manual_reference
module having to be imported and deleted every time.

Instead, use a function to get the proper prefix each time, depending
on current language, even if it has changed.

This will allow us to use the proper manual language from the help
menu in a future commit.

The review was started in https://archive.blender.org/developer/D16263.

The manual url prefix was generated statically, using the current language from the preferences. This led to the `rna_manual_reference` module having to be imported and deleted every time. Instead, use a function to get the proper prefix each time, depending on current language, even if it has changed. This will allow us to use the proper manual language from the help menu in a future commit. ----- The review was started in https://archive.blender.org/developer/D16263.

Damien Picard reviewed 2023-02-09 00:30:44 +01:00

release/scripts/modules/bpy/utils/__init__.py

						
				@ -1007,3 +1006,1 @@

				    # avoid storing in memory

				    del _sys.modules["rna_manual_reference"]

				    return ret

				    return (ref_mod.get_url_manual_prefix(), ref_mod.url_manual_mapping)

Damien Picard commented

2023-02-09 00:30:44 +01:00

In the original review, Campbell wrote:

This change doesn't seem necessary, can't this be left as-is? (replacing ref_mod.url_manual_prefix with ref_mod.url_manual_prefix()). Why is it necessary to keep the module loaded?

I replied by quoting what Bastien wrote in D15766:

I do not understand that, makes no sense to me, it basically defeats the point of modules caching in python, by forcing re-importing the module on every call?

Unless the point here is that this module has to be reloaded every time, to regenerate the rna_manual_reference.LANG value?

I wanted to use the url_manual_prefix in another file for !104492, but Bastien requested this change, so I figured I might as well change the existing use at the same time. But it may well be that I misunderstood the intent behind unloading the module on each call.

In [the original review](https://archive.blender.org/developer/D16263), Campbell wrote: > This change doesn't seem necessary, can't this be left as-is? (replacing `ref_mod.url_manual_prefix` with `ref_mod.url_manual_prefix()`). Why is it necessary to keep the module loaded? I replied by quoting what Bastien wrote in [D15766](https://archive.blender.org/developer/differential/0015/0015766/index.html): > I do not understand that, makes no sense to me, it basically defeats the point of modules caching in python, by forcing re-importing the module on every call? > > Unless the point here is that this module has to be reloaded every time, to regenerate the rna_manual_reference.LANG value? I wanted to use the `url_manual_prefix` in another file for !104492, but Bastien requested this change, so I figured I might as well change the existing use at the same time. But it may well be that I misunderstood the intent behind unloading the module on each call.

Campbell Barton commented

2023-02-09 00:45:36 +01:00

In general avoid mixing in unrelated changes, it complicates review and means the reviewer needs to guess at why a change was made that isn't related to the patch description.

The intent was to avoid keeping a large mapping in memory unnecessarily.
Since this only runs when the user clicks on a manual link, I didn't see the overhead of loading the module each time to be a problem (it's cached as a *.pyc file so loading is fast). But it would be better to discuss this outside of this patches review.

Please remove these changes from the patch.

In general avoid mixing in unrelated changes, it complicates review and means the reviewer needs to guess at why a change was made that isn't related to the patch description. The intent was to avoid keeping a large mapping in memory unnecessarily. Since this only runs when the user clicks on a manual link, I didn't see the overhead of loading the module each time to be a problem (it's cached as a `*.pyc` file so loading is fast). But it would be better to discuss this outside of this patches review. Please remove these changes from the patch.

Damien Picard commented

2023-02-09 01:29:16 +01:00

Thank you for the explanation :)

Campbell Barton commented

2023-02-09 01:37:00 +01:00

This kind of information should be included in comments, also - importing then deleting from sys.modules is not great, committed 9fd71d470e to improve the situation, also double checked performance isn't likely to be a problem.

This kind of information should be included in comments, also - importing then deleting from `sys.modules` is not great, committed 9fd71d470e5bebf12029acb770f3ab7620b418d3 to improve the situation, also double checked performance isn't likely to be a problem.

pioverfour marked this conversation as resolved

Damien Picard requested review from Campbell Barton 2023-02-09 00:31:18 +01:00

Damien Picard requested review from Aaron Carlisle 2023-02-09 00:31:18 +01:00

Damien Picard referenced this pull request

2023-02-09 00:42:04 +01:00

WIP: I18n: use proper language code when opening the manual from menu #104492

Campbell Barton requested changes 2023-02-09 00:47:51 +01:00

release/scripts/modules/rna_manual_reference.py

						
				@ -36,0 +34,4 @@

				    if language == 'DEFAULT':

				        import os

				        language = os.getenv('LANG', '').split('.')[0]

				    return language_codes.get(language, 'en')

Campbell Barton commented

2023-02-09 00:40:34 +01:00

Double quotes for non-enums.

pioverfour marked this conversation as resolved

Damien Picard force-pushed dp_dynamic_manual_lang_mapping from 4ed638e55d to 6e480b9e17

2023-02-09 01:42:29 +01:00

Compare

Damien Picard requested review from Campbell Barton 2023-02-09 01:42:48 +01:00

Brecht Van Lommel added a new dependency 2023-02-09 01:47:53 +01:00

#104492 WIP: I18n: use proper language code when opening the manual from menu

Campbell Barton approved these changes 2023-02-09 03:56:50 +01:00

Campbell Barton requested changes 2023-02-09 04:03:10 +01:00

Campbell Barton left a comment

This patch in it's current form doesn't seem to do anything useful.

url_manual_prefix() needs to be made into a function, currently any script that imports this will still have a static variable.

This patch in it's current form doesn't seem to do anything useful. url_manual_prefix() needs to be made into a function, currently any script that imports this will still have a static variable.

Campbell Barton referenced this pull request

2023-02-09 05:25:08 +01:00

PyAPI: add bpy.utils.manual_locale_code(), use for "Help" menu #104494

Campbell Barton commented

2023-02-09 05:27:36 +01:00

After checking on D15766 & D16263, I'd propose an alternative change to this, !104494.