Home of the Squeezebox™ & Transporter® network music players.
Results 1 to 7 of 7
  1. #1

    Detecting screen saver with now playing 'screen saver'

    Hi,

    For my brightess patch, I use the service offered by the screen saver applet to detect if the screen saver is running (appletManager:callService("isScreensaverActive")) . This works okay, except for the now playig screen saver. Apparently, this is not considered a real screen saver, so the return is always false.

    I look into the actual screen saver applet code (function _activate(the_screensaver)), and even for the now playing screen, it goes through all the code of an actual screen saver, but just doesn't start a screen saver. It would be quite trivial to add an extra variable to this code, set it to true when finishing this function, and set it to false on any event. isScreensaverActive could then simply return this value.

    Is this considered a bug or a featured? If I would submit a bug report for it, what would be the chance that it would be fixed. Of course I could patch it myself as part of the brightness patch, but I'd like to try not to modify existing code too much.
    My patch: Reduce brightness when screen saver is active for Touch (and maybe Radio)

  2. #2
    Senior Member erland's Avatar
    Join Date
    Dec 2005
    Location
    Sweden
    Posts
    10,840
    Quote Originally Posted by 505 View Post
    Hi,

    For my brightess patch, I use the service offered by the screen saver applet to detect if the screen saver is running (appletManager:callService("isScreensaverActive")) . This works okay, except for the now playig screen saver. Apparently, this is not considered a real screen saver, so the return is always false.

    I look into the actual screen saver applet code (function _activate(the_screensaver)), and even for the now playing screen, it goes through all the code of an actual screen saver, but just doesn't start a screen saver. It would be quite trivial to add an extra variable to this code, set it to true when finishing this function, and set it to false on any event. isScreensaverActive could then simply return this value.

    Is this considered a bug or a featured? If I would submit a bug report for it, what would be the chance that it would be fixed. Of course I could patch it myself as part of the brightness patch, but I'd like to try not to modify existing code too much.
    If you know how to patch it, I would suggest that you start by posting a patch in this thread so someone can take a look at it because currently I'm not 100% sure I even understand exactly what you like to change.

    If you register a bug, the chance of it to be included is a lot better if you also attach a tested patch to the bug report.
    Erland Isaksson (My homepage)
    (Developer of many plugins/applets (both free and commercial).
    If you like to encourage future presence on this forum and/or third party plugin/applet development, consider purchasing some plugins)

    Interested in the future of music streaming ? ickStream - A world of music at your fingertips.

  3. #3
    Thanks. I submitted a bug, and attached a patch to this bug. Any input on the proposed patch would be appreciated.

    http://bugs.slimdevices.com/show_bug.cgi?id=17916
    My patch: Reduce brightness when screen saver is active for Touch (and maybe Radio)

  4. #4
    Senior Member erland's Avatar
    Join Date
    Dec 2005
    Location
    Sweden
    Posts
    10,840
    Based on the NowPlayingApplet.lua and its openScreensaver function I wonder if the following bug was the cause of NowPlayingApplet not being handled as a normal screen saver:
    http://bugs.slimdevices.com/show_bug.cgi?id=12002

    I wonder if there is any risk the states get inconsistent with your patch ?
    Since you basically are duplicating the self.active hash states with a new boolean state.
    Erland Isaksson (My homepage)
    (Developer of many plugins/applets (both free and commercial).
    If you like to encourage future presence on this forum and/or third party plugin/applet development, consider purchasing some plugins)

    Interested in the future of music streaming ? ickStream - A world of music at your fingertips.

  5. #5
    Quote Originally Posted by erland View Post
    I wonder if there is any risk the states get inconsistent with your patch ?
    Since you basically are duplicating the self.active hash states with a new boolean state.
    Thanks for your comment. I went over the code again, and there is indeed a case where the flag is not cleared correctly. The patch is pretty conservative in how it sets the flag: set to true at the end of the activate function, set to false at user input.
    I now see that the function deactivateScreensaver() is also exposed as a service, and several applets call this service. So, at the beginning of this function, the flag should be set to false too.

    In terms of consequences, these might not be that big. The goal is to have a proper implementation of IsScreenSaverActive(), and this is only used in two places:
    - _activate() in ScreenSaverApplet, to prevent the back light from going on on the blankscreen screen saver. The flag is set after this check, so it should be safe.
    - showNowPlaying() in NowPlayingApplet, to test a rare condition where a screen saver is already active. This one is then deactivated.
    My patch: Reduce brightness when screen saver is active for Touch (and maybe Radio)

  6. #6
    Senior Member erland's Avatar
    Join Date
    Dec 2005
    Location
    Sweden
    Posts
    10,840
    Quote Originally Posted by 505 View Post
    Thanks for your comment. I went over the code again, and there is indeed a case where the flag is not cleared correctly. The patch is pretty conservative in how it sets the flag: set to true at the end of the activate function, set to false at user input.
    I now see that the function deactivateScreensaver() is also exposed as a service, and several applets call this service. So, at the beginning of this function, the flag should be set to false too.

    In terms of consequences, these might not be that big. The goal is to have a proper implementation of IsScreenSaverActive(), and this is only used in two places:
    - _activate() in ScreenSaverApplet, to prevent the back light from going on on the blankscreen screen saver. The flag is set after this check, so it should be safe.
    - showNowPlaying() in NowPlayingApplet, to test a rare condition where a screen saver is already active. This one is then deactivated.
    For me it feels like the main obstacle to make it safe to commit your patch is that we need to understand why NowPlayingApplet isn't considered to be an active screen saver. If it was considered to be an active screen saver, we wouldn't have any problem, because then the original code would work.

    When I read bug #12002 I get the impression that the reason is that NowPlayingApplet needs to allow user interaction without deactivating the screen saver, basically the user should be able to switch to VU meters without deactivating the screen saver. For all other screen savers any user activity results in that the screen saver is closed.

    My personal feeling is that isScreenSaverActive service should return "true" independent of the screen saver currently active, and as I understand this is exactly what your patch corrects. If you haven't tried how it works when switching VU meter mode, on a Squeezebox Touch, it might be a good idea to verify this as it seems like that was the reason to make NowPlayingApplet behave differently than all other screen savers.

    After doing that it feels like you have done your part and we can just wait for someone from Logitech to take a look at it and decide if they want to incorporate your patch or not.
    Erland Isaksson (My homepage)
    (Developer of many plugins/applets (both free and commercial).
    If you like to encourage future presence on this forum and/or third party plugin/applet development, consider purchasing some plugins)

    Interested in the future of music streaming ? ickStream - A world of music at your fingertips.

  7. #7
    Quote Originally Posted by erland View Post
    If you haven't tried how it works when switching VU meter mode, on a Squeezebox Touch, it might be a good idea to verify this as it seems like that was the reason to make NowPlayingApplet behave differently than all other screen savers.
    This still works as intended. You can still change to the VU meter mode, with just a single press, even when isScreenSaverActive=true. As soon as you touch the screen (or use the remote), isScreenSaverActive becomes false. I think this is also intended behaviour, since I'm at that point interacting with the system, and this should stop the screen saver.

    Only minor point could be openScreensaver() in NowPlayingApplet. This does a deactivatescreensaver(), settings isScreenSaverActive to false, then restarts the screen saver timer, which does an _activate(), setting isScreenSaverActive to true.
    In all test cases, the false was set before the true, so the end result is the expected true. If things don't happen parallel, this is safe. Or, the deactivatescreensaver() could be removed from the function. Comments next to it state that it is not needed at the moment.
    My patch: Reduce brightness when screen saver is active for Touch (and maybe Radio)

Tags for this Thread

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •