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

Allow to override global proxy settings #581

Merged
merged 2 commits into from
Dec 4, 2021
Merged

Allow to override global proxy settings #581

merged 2 commits into from
Dec 4, 2021

Conversation

oliv3r
Copy link
Contributor

@oliv3r oliv3r commented Nov 23, 2021

There are various use-cases where we want to configure proxy settings within trakt. E.g. Kodi/the host may be setup to connect to a Socks5 proxy, which script.trakt currently does not support. Additionally the user may want to use a different proxy (or just for trakt) then the Kodi/host configuration has set up. As this is feature for expert users, this should be hidden behind the expert settings level.

To accomplish the above, a second commit exists in this merge request, to migrate the settings.xml file to the newer kodi format, which allows to hide settings at different levels.

Copy link
Collaborator

@razzeee razzeee left a comment

Choose a reason for hiding this comment

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

LGTM

@oliv3r
Copy link
Contributor Author

oliv3r commented Nov 29, 2021

@razzeee what are the next steps to get this merged now? Anything else I need to do?

@razzeee
Copy link
Collaborator

razzeee commented Nov 29, 2021

I need to find time to do a final settings test (or run time tests) I only had a look at the code above.

@razzeee
Copy link
Collaborator

razzeee commented Nov 29, 2021

The scrobbling and general categories seem to have problems.

  1. scrobbling, changed order and a seemingly arbitrary grouping (draws a line in the middle)
  2. general, seems like it shows the hidden settings on expert level? there are a bunch of settings, that are just empty with no title.

@oliv3r
Copy link
Contributor Author

oliv3r commented Nov 30, 2021

@razzeee It seems to be working here, but I've not extensively tested each and every setting.

  1. As for scrobbeling; 8d975cc#diff-8e5952d4ad33bfc8e7b5b8739c8f898dd3f950d0f2548bc09f901489f50f6339R417 defines the group and indeed 8d975cc#diff-8e5952d4ad33bfc8e7b5b8739c8f898dd3f950d0f2548bc09f901489f50f6339R434 is the second group defined. Could be copy paste error (or unconscious :) It might be as it seemed logical to group the scrobbeling types together? I don't remember, I'll remove the group and double check the order again. Come to think of it, this was one of the sections I was playing with and maybe I intended to offer this (later) as an settings improvement. Anyway, it's fixed :)

The blank line is because there's no name (label) for this section/group (yet).

  1. This is 'just how kodi's setting system works' I guess /I don't think you can define the level on a category/group level, just on an individual setting level. My kodi won't show sections (latest libreelec) that have no settings. So it looks/feels quite natural. But the default levels are up to you I suppose what you want. I think the current 'makes sense' where the basic only has connectivity settings, and 'standard' (i think the kodi default) will show up most 'flags'.

@razzeee
Copy link
Collaborator

razzeee commented Nov 30, 2021

Scrobbling seems fine

image

General still needs work

@oliv3r
Copy link
Contributor Author

oliv3r commented Dec 2, 2021

Scrobbling seems fine

General still needs work

Hey @razzeee, I only did the language file for en_US; not sure what was needed. I figured once merged transifex would add them. What is the process normally here? Should I add all translations entries? (I head tail -n 10 en_US >> $file would work) ...

@razzeee
Copy link
Collaborator

razzeee commented Dec 2, 2021

I don't think that's the problem. I would think those are the hidden config fields, that are now configured to show up (by mistake I assume)

@oliv3r
Copy link
Contributor Author

oliv3r commented Dec 2, 2021

I don't think that's the problem. I would think those are the hidden config fields, that are now configured to show up (by mistake I assume)

@razzeee but they look perfectly like the proxy stuff, e.g. URL (blank string) port (0) etc. I put the translations in my en_GB file as well for testing, and i see the actual text, which is as expected either way, as it is supposed to show up in 'expert' mode?

I get the same as your screenshot; but when I do tail -n 40 resource.language.en_US/strings.po >> resource.language.en_GB/strings.po it looks fine?

@razzeee
Copy link
Collaborator

razzeee commented Dec 3, 2021

Your right, I didn't realize you did US instead of UK. As far as I know we need to change that, to have translations working.

@oliv3r
Copy link
Contributor Author

oliv3r commented Dec 3, 2021

Your right, I didn't realize you did US instead of UK. As far as I know we need to change that, to have translations working.

Ok, i'll do a quick shell script and do all langs; done @razzeee on the upside, it made the review easier :)

for future googlers, what I did was

for _dir in *; do tail -n 40 resource.language.en_US/strings.po >> ${_dir}/strings.po; done
git restore resource.language.en_US/strings.po
git add */strings.po

@razzeee
Copy link
Collaborator

razzeee commented Dec 4, 2021

@gade01 can we add the translations, as proposed or would that mess up the translation flow, as it expects to do it on it's own?

@oliv3r
Copy link
Contributor Author

oliv3r commented Dec 4, 2021

@gade01 can we add the translations, as proposed or would that mess up the translation flow, as it expects to do it on it's own?

I didn't add the translations themselves, just the untranslated entries, which should mean, if untranslated, they show up as english, once a translating service (transifex at all) drops by, they list them as untranslated strings until someone fills it in I suppose.

@gade01
Copy link
Contributor

gade01 commented Dec 4, 2021

@razzeee @oliv3r
Thanks for reaching out :)
New translations should be added only to en_gb file.
Weblate will add them the correct way to all other language files.

Right now they are added after the commented out lines for some languages,

Also, please add a line break before new strings in en_gb file:
https://github.com/trakt/script.trakt/pull/581/files#diff-12c1b153e8d2d6703d5a8cb60ea98f77680652daf82ae49903767a212d12bfe3R740

@gade01
Copy link
Contributor

gade01 commented Dec 4, 2021

Please merge any open Weblate PR before merging this one, thanks.

#583

Otherwise it will cause merge coonflicts at Weblate.

The current way of settings for trakt is deprecated and should migrate
to the new format (see [0]). This commit performs the migration to the
new settings format.

Everything is kept as close as possible on how it was, with all standard
configuration settings added to the 'standard' level, auth settings to
the 'basic' level (as otherwise the addon is not that useful) and some
more complex things (debug for example) to higher levels.

The 'internal settings' that are used to store some tokens etc, are at
the 'never show in GUI' level 4.

[0]: https://kodi.wiki/view/Add-on_settings

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
There are various use-cases where we want to configure proxy settings
within trakt. E.g. Kodi/the host may be setup to connect to a Socks5
proxy, which script.trakt currently does not support. Additionally the
user may want to use a different proxy (or just for trakt) then the
Kodi/host configuration has set up.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
@oliv3r
Copy link
Contributor Author

oliv3r commented Dec 4, 2021

@gade01 Done, I removed the other language and only left the en_GB one (after the newline fix; turns out, there was no line-ending at all :( There is now again.

@razzeee as #583 is now merged, I have also rebased as well, so should be ready to go :)

Copy link
Contributor

@gade01 gade01 left a comment

Choose a reason for hiding this comment

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

@oliv3r Thanks a lot.
Language changes look great to me.

@razzeee
Copy link
Collaborator

razzeee commented Dec 4, 2021

Thanks for sticking with us :)

@razzeee razzeee merged commit dee9e8a into trakt:main Dec 4, 2021
@oliv3r
Copy link
Contributor Author

oliv3r commented Dec 5, 2021

No problem, I hope you value the contribution :)

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