PDA

View Full Version : [Slim-Checkins] r3353 - trunk/server/Slim/Hardware



Robert Moser
2005-06-07, 20:57
kdf (AT) svn (DOT) slimdevices.com blurted out:
> Author: kdf
> Date: 2005-06-07 17:22:28 -0700 (Tue, 07 Jun 2005)
> New Revision: 3353
>
> Modified:
> trunk/server/Slim/Hardware/IR.pm
> Log:
> Bug: 527
>
> Description: irset and irmap prefs should use filename only instead of
> an absolute path. This allows version upgrades and multiple servers
> in different paths that can better share prefs.

That's fine (at least with me, Dean will probably disagree), but these
two changes need to go. We really don't want to have to touch the file
system twice every time we have an IR event. Any particular reason for
these?


> @@ -287,6 +307,7 @@
> for (Slim::Utils::Prefs::clientGetArray($client,'disab ledirsets')) {delete $enabled{$_}};
>
> for my $irset (keys %enabled) {
> + $irset = -r $irset ? $irset : IRPath($irset);
> if (defined $irCodes{$irset}{$code}) {
> $::d_ir && msg("found button $irCodes{$irset}{$code} for $code\n");
> $code = $irCodes{$irset}{$code};
> @@ -312,7 +333,10 @@
> my $mode = shift;
>
> $mode = Slim::Buttons::Common::mode($client) unless defined($mode);
> +
> my $map = Slim::Utils::Prefs::clientGet($client,'irmap');
> + $map = -r $map ? $map : IRPath($map);
> +
> assert($client);
> assert($map);
> assert($mode);

dean
2005-06-07, 21:09
On Jun 7, 2005, at 8:57 PM, Robert Moser wrote:

> kdf (AT) svn (DOT) slimdevices.com blurted out:
>
>> Author: kdf
>> Date: 2005-06-07 17:22:28 -0700 (Tue, 07 Jun 2005)
>> New Revision: 3353
>> Modified:
>> trunk/server/Slim/Hardware/IR.pm
>> Log:
>> Bug: 527
>> Description: irset and irmap prefs should use filename only
>> instead of
>> an absolute path. This allows version upgrades and multiple servers
>> in different paths that can better share prefs.
>>
>
> That's fine (at least with me, Dean will probably disagree), but
> these two changes need to go.
I'm not following. What will I disagree about?

> We really don't want to have to touch the file system twice every
> time we have an IR event. Any particular reason for these?
Agreed.

kdf
2005-06-07, 21:18
On 7-Jun-05, at 8:57 PM, Robert Moser wrote:

> kdf (AT) svn (DOT) slimdevices.com blurted out:
>> Author: kdf
>> Date: 2005-06-07 17:22:28 -0700 (Tue, 07 Jun 2005)
>> New Revision: 3353
>> Modified:
>> trunk/server/Slim/Hardware/IR.pm
>> Log:
>> Bug: 527
>> Description: irset and irmap prefs should use filename only instead of
>> an absolute path. This allows version upgrades and multiple servers
>> in different paths that can better share prefs.
>
> That's fine (at least with me, Dean will probably disagree),

Dean requested the all clear in the bug report. As it was, this was the
only pref that was hanging on to full paths.

> but these two changes need to go. We really don't want to have to
> touch the file system twice every time we have an IR event. Any
> particular reason for these?

Well, if a user sets up a custom ir set and custom map, then moves to
another version that doesnt happen to have that custom ir or map file,
things fall apart. Those lines are meant to trap that and grab a fresh
valid file. I'm open to better methods.

-kdf

dean
2005-06-07, 21:39
On Jun 7, 2005, at 9:18 PM, kdf wrote:
>> but these two changes need to go. We really don't want to have to
>> touch the file system twice every time we have an IR event. Any
>> particular reason for these?
>>
>
> Well, if a user sets up a custom ir set and custom map, then moves
> to another version that doesnt happen to have that custom ir or map
> file, things fall apart. Those lines are meant to trap that and
> grab a fresh valid file. I'm open to better methods.

I don't think it's unreasonable to suggest that a server would need
to be restarted for an irmap to be reloaded.

kdf
2005-06-07, 21:55
On 7-Jun-05, at 9:39 PM, dean blackketter wrote:

>
> On Jun 7, 2005, at 9:18 PM, kdf wrote:
>>> but these two changes need to go. We really don't want to have to
>>> touch the file system twice every time we have an IR event. Any
>>> particular reason for these?
>>>
>>
>> Well, if a user sets up a custom ir set and custom map, then moves to
>> another version that doesnt happen to have that custom ir or map
>> file, things fall apart. Those lines are meant to trap that and grab
>> a fresh valid file. I'm open to better methods.
>
> I don't think it's unreasonable to suggest that a server would need to
> be restarted for an irmap to be reloaded.
>
sure, then we'll also need an upgrade script to strip the old pref and
replace it with the filename instead of full path. otherwise, the code
would be adding the path twice.
-k

kdf
2005-06-07, 22:12
Here is perhaps one way to fix this. This requires restarts, but will
create the map and irset hashes based on filename, adding the full path
for loading during init only. The it will use the memory resident
hashes from then on.

-kdf

Robert Moser
2005-06-07, 23:46
kdf blurted out:
> Here is perhaps one way to fix this. This requires restarts, but will
> create the map and irset hashes based on filename, adding the full path
> for loading during init only. The it will use the memory resident
> hashes from then on.
>
> -kdf

Looks good to me. The one in lookup() wasn't really doing much to help
anyway, since the keys of the %enabled hash weren't coming from the
disabledirsets preference anyway. A bad value for irmap won't cause too
much trouble, it will just use $defaultmapfile instead. Speaking of
which, defaultMapFile() will need to be changed to return just the
filename, since we use $defaultMapFile to do the hash lookup, so it must
match a key in the %irMap hash.

The reason why I anticipated Dean not liking the change was just because
he originally made the change from just the filename to the full path.
I of course spouted off without looking at the bug.

Looking at the lookup function closely, it seems that we were touching
the filesystem each time anyway, due to the irfiles() call. It would
probably be better in the lookup() function to use a copy of the keys of
the %irCodes hash rather than the return value from irfiles().

kdf
2005-06-08, 00:04
Quoting Robert Moser <rlmoser (AT) comcast (DOT) net>:
Speaking of
> which, defaultMapFile() will need to be changed to return just the
> filename, since we use $defaultMapFile to do the hash lookup, so it must
> match a key in the %irMap hash.

right, I'll have to do that one in the morning. My eyes are just about shut
from jetlag.
>
> The reason why I anticipated Dean not liking the change was just because
> he originally made the change from just the filename to the full path.
> I of course spouted off without looking at the bug.

np. I never went back into the history, just assumed it was always like that.

>
> Looking at the lookup function closely, it seems that we were touching
> the filesystem each time anyway, due to the irfiles() call. It would
> probably be better in the lookup() function to use a copy of the keys of
> the %irCodes hash rather than the return value from irfiles().
>
I did a quick switch to that idea and committed it. I'll be looking at it more
anyway since bug 494 also affects this section of code and will not patch
directly from the latest attachment there.

thanks!

-kdf