tomasklaen / uosc

Feature-rich minimalist proximity-based UI for MPV player.
GNU Lesser General Public License v2.1
1.86k stars 68 forks source link

feat: add char_conv option #947

Closed bennyyip closed 3 months ago

bennyyip commented 3 months ago

Currently char_conv doesn't work if the first language in languages is en. This PR adds an option to enable it.

It is disabled by default and BREAKS the old behavior. The old users of this feature need to add following line to their usoc.conf:

char_conv=yes
dyphire commented 3 months ago

The char_conv is designed to be used only for localized language romanization, and when the languages option's language priority is headed by en it means that the user is an English-speaking user who doesn't need localized language processing. Not enforcing char_conv is expected, and there is no reason to add an option to force it and break the usefulness of the languages option.

Edit: even if there is a good reason to always localize, all you need is to apply a patch like the one below to intl.lua and char_conv.lua instead of adding an option to break the existing behavior.

---
 src/uosc/lib/char_conv.lua | 6 +-----
 src/uosc/lib/intl.lua      | 2 --
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/src/uosc/lib/char_conv.lua b/src/uosc/lib/char_conv.lua
index 8e5183c..101a629 100644
--- a/src/uosc/lib/char_conv.lua
+++ b/src/uosc/lib/char_conv.lua
@@ -6,11 +6,7 @@ local data = {}
 local languages = get_languages()
 for i = #languages, 1, -1 do
    lang = languages[i]
-   if (lang == 'en') then
-       data = {}
-   else
-       table_assign(data, get_locale_from_json(char_dir .. lang:lower() .. '.json'))
-   end
+   table_assign(data, get_locale_from_json(char_dir .. lang:lower() .. '.json'))
 end

 local romanization = {}
diff --git a/src/uosc/lib/intl.lua b/src/uosc/lib/intl.lua
index 79a7c64..955de23 100644
--- a/src/uosc/lib/intl.lua
+++ b/src/uosc/lib/intl.lua
@@ -58,8 +58,6 @@ for i = #languages, 1, -1 do
    lang = languages[i]
    if (lang:match('.json$')) then
        table_assign(locale, get_locale_from_json(lang))
-   elseif (lang == 'en') then
-       locale = {}
    else
        table_assign(locale, get_locale_from_json(intl_dir .. lang:lower() .. '.json'))
    end
-- 
bennyyip commented 3 months ago

it means that the user is an English-speaking user who doesn't need localized language processing

No. I use English for UI but I still prefer to search chapter/playlist without switching IME. It doesn't make sense to skip romanization when the user is using English.

all you need is to apply a patch like the one below

Adding a new option is better than applying an extra patch.

instead of adding an option to break the existing behavior.

Yes, it's bad to break existing behavior. Maybe we can detect the languages options to decide whether it's enable to keep backward compatibility. But it's more complicated than my current implementation.

I doubt only few users know this feature because it's not documented. It should not be big problem to break an undocumented behavior.

dyphire commented 3 months ago

I see what you need now, but this can be accomplished by simply applying the following patch without adding the option, and it won't cause any damage. This was a consideration I was missing when I added the Romanization feature, thanks for pointing this out.

---
 src/uosc/lib/char_conv.lua | 6 +-----
 1 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/uosc/lib/char_conv.lua b/src/uosc/lib/char_conv.lua
index 8e5183c..101a629 100644
--- a/src/uosc/lib/char_conv.lua
+++ b/src/uosc/lib/char_conv.lua
@@ -6,11 +6,7 @@ local data = {}
 local languages = get_languages()
 for i = #languages, 1, -1 do
    lang = languages[i]
-   if (lang == 'en') then
-       data = {}
-   else
-       table_assign(data, get_locale_from_json(char_dir .. lang:lower() .. '.json'))
-   end
+   table_assign(data, get_locale_from_json(char_dir .. lang:lower() .. '.json'))
 end

 local romanization = {}
-- 

By the way, at the moment uosc only has Chinese romanization files, I'm not sure if you're using this feature correctly.

bennyyip commented 3 months ago

I see what you need now, but this can be accomplished by simply applying the following patch without adding the option, and it won't cause any damage. This was a consideration I was missing when I added the Romanization feature, thanks for pointing this out.

---
 src/uosc/lib/char_conv.lua | 6 +-----
 1 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/uosc/lib/char_conv.lua b/src/uosc/lib/char_conv.lua
index 8e5183c..101a629 100644
--- a/src/uosc/lib/char_conv.lua
+++ b/src/uosc/lib/char_conv.lua
@@ -6,11 +6,7 @@ local data = {}
 local languages = get_languages()
 for i = #languages, 1, -1 do
  lang = languages[i]
- if (lang == 'en') then
-     data = {}
- else
-     table_assign(data, get_locale_from_json(char_dir .. lang:lower() .. '.json'))
- end
+ table_assign(data, get_locale_from_json(char_dir .. lang:lower() .. '.json'))
 end

 local romanization = {}
-- 

By the way, at the moment uosc only has Chinese romanization files, I'm not sure if you're using this feature correctly.

This patch is better. I added a new option because I don't want extra overhead for people don't need it. With this patch, there's still 0 overhead when zh is not in languages.

I speak Chinese too. And I know this feature because of your mpv config.

@dyphire Could you make a new PR with this patch?

dyphire commented 3 months ago

Could you make a new PR with this patch?

948