PDA

View Full Version : patch: Slim::Utils::Strings



mherger
2005-03-03, 13:42
Two more tweaks for the StringEditor: I renamed the variables I'm using in
the plugin to follow the guidelines ('plugin-stringeditor-varname') and
added a conditional initialization of the strings. The latter is needed
with plugins that contain their strings in the __DATA__ part of the
script. If I flush %strings they won't reload properly.

Is this ok?

Index: /home/mh/eclipse/SVN/Slim/Utils/Strings.pm
================================================== =================
--- /home/mh/eclipse/SVN/Slim/Utils/Strings.pm (revision 2300)
+++ /home/mh/eclipse/SVN/Slim/Utils/Strings.pm (working copy)
@@ -37,7 +37,7 @@
my $usr_strings;

# clear these so they can be reused for Michael's translator plugin
- %strings = ();
+ %strings = () if
(Slim::Utils::Prefs::get('plugin-stringeditor-flushstrings'));
%languages = ();

for my $dir (stringsDirs()) {
@@ -215,7 +215,7 @@

next unless $strings{$stringname}->{$tryLang};

- return $strings{$stringname}->{$tryLang} . (($tryLang ne $language) &&
Slim::Utils::Prefs::get('translatorMode') ? " {$stringname}" : '');
+ return $strings{$stringname}->{$tryLang} . (($tryLang ne $language) &&
Slim::Utils::Prefs::get('plugin-stringeditor-translatormode') ? "
{$stringname}" : '');
}


--

Michael

-----------------------------------------------------------
Help translate SlimServer by using the
SlimString Translation Helper (http://www.herger.net/slim/)

kdf
2005-03-03, 13:56
Quoting Michael Herger <slim (AT) herger (DOT) net>:

> Two more tweaks for the StringEditor: I renamed the variables I'm using in
> the plugin to follow the guidelines ('plugin-stringeditor-varname') and
> added a conditional initialization of the strings. The latter is needed
> with plugins that contain their strings in the __DATA__ part of the
> script. If I flush %strings they won't reload properly.
>
> Is this ok?
>
> Index: /home/mh/eclipse/SVN/Slim/Utils/Strings.pm
> ================================================== =================
> --- /home/mh/eclipse/SVN/Slim/Utils/Strings.pm (revision 2300)
> +++ /home/mh/eclipse/SVN/Slim/Utils/Strings.pm (working copy)
> @@ -37,7 +37,7 @@
> my $usr_strings;
>
> # clear these so they can be reused for Michael's translator plugin
> - %strings = ();
> + %strings = () if
> (Slim::Utils::Prefs::get('plugin-stringeditor-flushstrings'));
> %languages = ();
>
> for my $dir (stringsDirs()) {
> @@ -215,7 +215,7 @@
>
> next unless $strings{$stringname}->{$tryLang};
>
> - return $strings{$stringname}->{$tryLang} . (($tryLang ne $language) &&
> Slim::Utils::Prefs::get('translatorMode') ? " {$stringname}" : '');
> + return $strings{$stringname}->{$tryLang} . (($tryLang ne $language) &&
> Slim::Utils::Prefs::get('plugin-stringeditor-translatormode') ? "
> {$stringname}" : '');
> }

I know this is only a name change, and the code is already there, but I've never
been a fan of having the core server code checking on plugin prefs/params. Not
everyone is going to have the plugin enabled, and it is always a user option to
delete the plugin altogether even if it is one that gets included in the
distro. It may work fine with undef values, but to me it seems too easy to get
complicated if you have to worry about plugins being hardcoded in.

What you could do, instead, is add a couple stubs that you can call from the
plugin to create the same effect. Then if the stub is never called, everything
should work as normal.

for example:

sub flushstrings {
%strings = ();
}

the second case is a bit more tricky. I'd have to ponder that. using standard
plugin prefname formatting is always welcome in any case :)
-kdf

mherger
2005-03-03, 14:12
On Thu, 03 Mar 2005 12:56:08 -0800, kdf <slim-mail (AT) deane-freeman (DOT) com>
wrote:

[..]
> What you could do, instead, is add a couple stubs that you can call from
> the
> plugin to create the same effect. Then if the stub is never called,
> everything
> should work as normal.
>
> for example:
>
> sub flushstrings {
> %strings = ();
> }

I'd be glad to do this - but can I change that variable from within a
plugin?!? Is the following correct:

sub initStrings {
%Slim::Utils::Strings::strings = {} if
(Slim::Utils::Prefs::get('plugin-stringeditor-flushstrings'));
Slim::Utils::Strings::init();
Slim::Buttons::Plugins::read_plugins();
}

All but the first line is already there - not much more than the stub you
mentioned. I tried it and it seems to work.

That's the nice thing about open-source: somebody tells you how to do
something better. I already thought about that kind of solution, but did
not try hard enough (always trying something like
Slim::Utils::Strings::%strings...). Thanks for this!

> the second case is a bit more tricky. I'd have to ponder that. using
> standard plugin prefname formatting is always welcome in any case :)

--

Michael

-----------------------------------------------------------
Help translate SlimServer by using the
SlimString Translation Helper (http://www.herger.net/slim/)

kdf
2005-03-03, 14:25
Quoting Michael Herger <slim (AT) herger (DOT) net>:

> On Thu, 03 Mar 2005 12:56:08 -0800, kdf <slim-mail (AT) deane-freeman (DOT) com>
> wrote:
>
> [..]
> > What you could do, instead, is add a couple stubs that you can call from
> > the
> > plugin to create the same effect. Then if the stub is never called,
> > everything
> > should work as normal.
> >
> > for example:
> >
> > sub flushstrings {
> > %strings = ();
> > }
>
> I'd be glad to do this - but can I change that variable from within a
> plugin?!? Is the following correct:
>
> sub initStrings {
> %Slim::Utils::Strings::strings = {} if
> (Slim::Utils::Prefs::get('plugin-stringeditor-flushstrings'));
> Slim::Utils::Strings::init();
> Slim::Buttons::Plugins::read_plugins();
> }
>
> All but the first line is already there - not much more than the stub you
> mentioned. I tried it and it seems to work.

I dont think you can explicitly set anything on a hash from another module.
That's why there are lots of modules that simply return a local hash.

how about:
sub initStrings {
Slim::Utils::Strings::flushStrings() if
Slim::Utils::Prefs::get('plugin-stringeditor-flushstrings');
Slim::Utils::Strings::init();
Slim::Buttons::Plugins::read_plugins();
}

then add this to Utils::Strings:

sub flushStrings {
%strings = ();
}


I dont know if thsi way is necessarily innately better than using the prefs
directly as you originally suggested. It just happens to be a prefernce of
mine to keep plugins self-contained as much as possible :) You only have to
look at the importer plugins to see that it isn't always possible.

-kdf

mherger
2005-03-03, 14:42
On Thu, 03 Mar 2005 13:25:09 -0800, kdf <slim-mail (AT) deane-freeman (DOT) com>
wrote:

[..]
>> sub initStrings {
>> %Slim::Utils::Strings::strings = {} if
>> (Slim::Utils::Prefs::get('plugin-stringeditor-flushstrings'));
>> Slim::Utils::Strings::init();
>> Slim::Buttons::Plugins::read_plugins();
>> }
>>
>> All but the first line is already there - not much more than the stub
>> you
>> mentioned. I tried it and it seems to work.
>
> I dont think you can explicitly set anything on a hash from another
> module.

That's what I thought.

> That's why there are lots of modules that simply return a local hash.

Hmm... it seems to be working, but I get warnings: "Reference found where
even-sized list expected at
/home/mh/eclipse/SVN/Plugins/StringEditor/Plugin.pm line 573."

> how about:
> sub initStrings {
> Slim::Utils::Strings::flushStrings() if
> Slim::Utils::Prefs::get('plugin-stringeditor-flushstrings');
> Slim::Utils::Strings::init();
> Slim::Buttons::Plugins::read_plugins();
> }
>
> then add this to Utils::Strings:
>
> sub flushStrings {
> %strings = ();
> }

No problem for me. But it's not within my power to do this. I can send a
patch, though :-).

> I dont know if thsi way is necessarily innately better than using the
> prefs
> directly as you originally suggested. It just happens to be a prefernce
> of
> mine to keep plugins self-contained as much as possible :) You only
> have to
> look at the importer plugins to see that it isn't always possible.

--

Michael

-----------------------------------------------------------
Help translate SlimServer by using the
SlimString Translation Helper (http://www.herger.net/slim/)

mherger
2005-03-03, 15:02
On Thu, 03 Mar 2005 22:42:31 +0100, Michael Herger <slim (AT) herger (DOT) net> wrote:

[..]
>>> %Slim::Utils::Strings::strings = {} if

Make this non curly brackets (%strings = ()) and it seems to be working
just fine... But if some perl guru can convince me that it's not possible,
maybe I'll believe him :-)

--

Michael

-----------------------------------------------------------
Help translate SlimServer by using the
SlimString Translation Helper (http://www.herger.net/slim/)

mherger
2005-03-03, 15:23
On Thu, 03 Mar 2005 22:12:34 +0100, Michael Herger <slim (AT) herger (DOT) net> wrote:

[..]
>> What you could do, instead, is add a couple stubs that you can call
>> from the
>> plugin to create the same effect. Then if the stub is never called,
>> everything
>> should work as normal.
>>
>> for example:
>>
>> sub flushstrings {
>> %strings = ();
>> }

Ok, here's what I have now:

sub initStrings {
%Slim::Utils::Strings::strings = () if
(Slim::Utils::Prefs::get('plugin-stringeditor-flushstrings'));
%Slim::Utils::Strings::languages = ();
Slim::Utils::Strings::init();
Slim::Buttons::Plugins::read_plugins();
}

This seems to be working: no error, no warning, expected effects. If
somebody can confirm me that the %Slim::Utils::Strings::strings
construction is expected to work, I'd be glad to remove the plugin
specific two lines from Slim::Utils::Strings::init(). Any comment?

Regards,

--

Michael

-----------------------------------------------------------
Help translate SlimServer by using the
SlimString Translation Helper (http://www.herger.net/slim/)

kdf
2005-03-03, 16:06
Quoting Michael Herger <slim (AT) herger (DOT) net>:

>
> No problem for me. But it's not within my power to do this. I can send a
> patch, though :-).

it probably should be, but granting that power isn't within My power ;)
as always, patches are absolutely welcome!

I wouldn't fuss too much if you are having trouble with changes. your original
idea is simple enough too. As long as it works, that's what counts.

-kdf

mherger
2005-03-04, 13:00
On Thu, 03 Mar 2005 15:06:01 -0800, kdf <slim-mail (AT) deane-freeman (DOT) com>
wrote:

> Quoting Michael Herger <slim (AT) herger (DOT) net>:
>
>>
>> No problem for me. But it's not within my power to do this. I can send a
>> patch, though :-).
>
> it probably should be, but granting that power isn't within My power ;)
> as always, patches are absolutely welcome!
>
> I wouldn't fuss too much if you are having trouble with changes. your
> original
> idea is simple enough too. As long as it works, that's what counts.

Thanks. I'll do some more testing on the plugin's code before I submit a
new patch. But renaming the variable would be a good thing - at least you
knew whom to ask whether it's still needed or not. With its current name
somebody will ask "what the h... is this for?" and it's quickly removed.

--

Michael

-----------------------------------------------------------
Help translate SlimServer by using the
SlimString Translation Helper (http://www.herger.net/slim/)