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

Screensaver inhibitor. #1229

Merged
merged 19 commits into from
Apr 9, 2017
Merged

Conversation

JosepMaJAZ
Copy link
Contributor

@JosepMaJAZ JosepMaJAZ commented Mar 25, 2017

This PR allows Mixxx to prevent the screensaver from starting while Mixxx is running.
https://bugs.launchpad.net/mixxx/+bug/1222098

Tested working on Windows and Linux with Gnome.
Needs testing on Mac (if it compiles... will wait for travis to verify that).
It has also additional code for the "screensaver extension of the X server" (libxss), but it is completely untested.

Currently, it is always on. Further refinements should include allowing the user to choose if it has to be always enabled, just when playing, or disable this feature.

  • Working on Windows
  • Working on Linux
  • Working on Mac
  • User configurable setting
  • inhibit only on play (possibility to inhibit when it is playing only)
  • inhibit on controller usage

@Be-ing
Copy link
Contributor

Be-ing commented Mar 25, 2017

Thanks, this works for me with KDE (X11). Build is working on all platforms. @esbrandt want to test on Mac?

I think the user configurable option should default to being enabled.

@esbrandt
Copy link
Contributor

Tested, and works for me with macos 10.11.6


IOReturn success;
// FIXME (JosepMaJAZ): I have no idea how to access the NSappKitVersionNumber.
/* work-around a bug in 10.7.4 and 10.7.5, so check for 10.7.x < 10.7.4 and 10.8 */
Copy link
Contributor

Choose a reason for hiding this comment

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

We require at least 10.8, see 2313e72

@MK-42
Copy link
Contributor

MK-42 commented Mar 26, 2017

What about a CO to set that state? Then skin-artists could introduce a button for it? In AutoDJ mode for example the Screensaver could be desired, and when starting the mixxx one could easily deactivate it?

@JosepMaJAZ
Copy link
Contributor Author

Ok, user configurable option implemented.
Now I need help on the option "prevent on play" and some discussion/guidance about the possible control object for this setting.

Prevent on play is slightly complicated because I'm not sure what would be the correct option. On one side, the EngineDeck::isActive() looks like a good candicate to check, but adding a timer to check this regularly doesn't look like the best option.
Another alternative would be to listen to the control objects "play" and "stop" of each deck and samplers, but would also require additional checking, since a deck stopping does not mean that another is not playing. (Bug https://bugs.launchpad.net/mixxx/+bug/1639495 is also missing a way to detect "playback stopped".)

@daschuer
Copy link
Member

Thank you for adopting this :-)

What means uninhibit? For me "Inhibit sreensaver" means "no screensaver".

What is the use-case for uninhibit on play?

You can hook this feature into

updateCurrentPlayingDeck();

The original use-case for me was that mouse and keyboard actions are inhibiting the screensaver, but not the midi controller. So it can happen that the screensaver or power save mode is activated even though the user is controlling Mixxx via midi controller.

To solve this, you could hook somewhere into the controller mapping code to inhibit the screen-saver.

@JosepMaJAZ
Copy link
Contributor Author

Oups!!! sorry.. obviously i meant the opposite. inhibit on play. Just fixed it in the description :D

About inhibiting it only on controller usage is not something that is needed right now with this solution. Basically, the current action on all systems is to prevent the screensaver while the application is running. The original bug and what you say is about sending events to the system if there is action from the controller.

@Be-ing
Copy link
Contributor

Be-ing commented Mar 28, 2017

What about a CO to set that state? Then skin-artists could introduce a button for it? In AutoDJ mode for example the Screensaver could be desired, and when starting the mixxx one could easily deactivate it?

Are there any other use cases in which it would be helpful to change whether the screensaver is inhibited while running Mixxx? If not, I think another preference option to uninhibit the screensaver prevention while AutoDJ is enabled would be a better solution than adding a button to skins.

@@ -11,6 +11,13 @@ enum class TooltipsPreference {
TOOLTIPS_ONLY_IN_LIBRARY = 2,
};

// Settings to enable or disable the prevention to run the screensaver.
enum class ScreenSaverPreference {
PREVENT_OFF = 0,
Copy link
Member

Choose a reason for hiding this comment

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

INHIBIT_OFF sounds more natural to me, though I am not a native speaker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, to me it's the word inhibit that isn't natural. I'm using it because that's the name used in the linux part of the solution and I kept that name.
http://dictionary.cambridge.org/dictionary/english/prevent
http://dictionary.cambridge.org/dictionary/english/prevent

Copy link
Member

Choose a reason for hiding this comment

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

I have checked google and you are probably right:
"prevent screensaver" 4550 Hits
"inhibit screensaver" 1020 Hits

src/mixxx.cpp Outdated
// Save the current window state (position, maximized, etc)
UserSettingsPointer pConfig = m_pSettingsManager->settings();
int inhibit = pConfig->getValue<int>(ConfigKey("[Config]","InhibitScreensaver"));
mixxx::ScreenSaverPreference inhiPref = mixxx::ScreenSaverPreference(inhibit);
Copy link
Member

Choose a reason for hiding this comment

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

This casing form is not easy recognizable as cast.
Can we switch to
auto inhiPref = static_castmixxx::ScreenSaverPreference(inhibit);
?

Copy link
Member

Choose a reason for hiding this comment

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

In this case we could also avoid casting to enum and cast to int below.
The line is not aware the new PREVENT_ON_PLAY feature.

So it may look finally like that:
if (inhiPref != static_cast<int>(mixxx::ScreenSaverPreference::PREVENT_OFF)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

use of "auto" and explicit cast sounds interesting. use of static_cast sounds in contradiction with the use of "enum class".
I would preffer the oppinion of someone else before changing that from what it is now.

Copy link
Member

Choose a reason for hiding this comment

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

You can keep it, if you like.

I have just looked in
https://google.github.io/styleguide/cppguide.html#Casting
In this case your enum class has no constructor so I think static_cast<> it the best choice here.
We only user auto rarely, except if it helps not to repeat ourselves like in this case.

src/mixxx.cpp Outdated
@@ -1079,6 +1105,15 @@ void MixxxMainWindow::slotNoMicrophoneInputConfigured() {
m_pPrefDlg->showSoundHardwarePage();
}

void MixxxMainWindow::slotChangedPlayingDeck(int deck) {
UserSettingsPointer pConfig = m_pSettingsManager->settings();
int inhibit = pConfig->getValue<int>(ConfigKey("[Config]","InhibitScreensaver"));
Copy link
Member

Choose a reason for hiding this comment

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

We should not parse the preferences over and over again.
Please save inhibit as member variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I will need a slot to call when I change the setting from the preferences.

Copy link
Member

Choose a reason for hiding this comment

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

This can be a direct call, because the DlgPrefControls holds a pointer to MixxxMainWindow.


namespace mixxx {

bool ScreenSaverHelper::enabled = false;
Copy link
Member

Choose a reason for hiding this comment

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

prefix with "s_"

if (inhiOld == mixxx::ScreenSaverPreference::PREVENT_ON) {
mixxx::ScreenSaverHelper::uninhibit();
} else if (inhiOld == mixxx::ScreenSaverPreference::PREVENT_ON_PLAY) {
mixxx::ScreenSaverHelper::inhibitOnCondition(false);
Copy link
Member

Choose a reason for hiding this comment

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

can't we call a plain uninhibit in this case as well?
Maybe we need to move the check if it is already uninhibited inside the mixxx::ScreenSaverHelper::uninhibit();
and simplify this code here much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conceptually yes, but in practice no.
Some of the implementations require that you call the pair inhibit as many times as uninhibit. (as in, calling inhibit twice and unhinibit once would not uninhibit. So just in case I try to keep that).

I probably need to find a better name for this method. What it really does is either inhibit or uninhibit, depending if the parameter is the same or different than the current status. like this:

Current status: inhibit on
inhibitOnCondition(true) -> does nothing
inhibitOnCondition(false) -> unhinibit.
Current status: inhibit off
inhibitOnCondition(true) -> inhibit
inhibitOnCondition(false) -> does nothing

The end result is always that true leaves it enabled and false leaves it disabled, but internally it decides if it has to change anything or not.

But now that I think about it, I'm not sure why i needed it as separate method. Will check and comment later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I ended incorporating the logic inside inhibit and uninhibit.

@daschuer
Copy link
Member

What about a CO to set that state? Then skin-artists could introduce a button for it? In AutoDJ mode for example the Screensaver could be desired, and when starting the mixxx one could easily deactivate it?

Are there any other use cases in which it would be helpful to change whether the screensaver is inhibited while running Mixxx? If not, I think another preference option to uninhibit the screensaver prevention while AutoDJ is enabled would be a better solution than adding a button to skins.

The only use case i can think of is to protecting Mixxx from curious eyes while you are at the restrooms.
This can be done better by immediately activate the OS screen lock. Arn't the OS controls sufficient for this?

@daschuer
Copy link
Member

@JosepMaJAZ: do you have fun and time to add the midi inhibit feature as well? All these config options are IMHO only a bandaid for it. If not, no problem than we can do it in an other PR.

Idea:

We may add a
PlayerInfo::setUserControlled()
which is called from the Controller subsystem
and PlayerInfo::getUserControlled() to simulate user actions for the screen-saver.
The midi controlled flag can be reset by a timer interval.
PlayerInfo::setUserControlled() can be called in:
MidiController::processInputMapping

What do you think?

@Be-ing
Copy link
Contributor

Be-ing commented Mar 28, 2017

The only use case i can think of is to protecting Mixxx from curious eyes while you are at the restrooms.
This can be done better by immediately activate the OS screen lock. Arn't the OS controls sufficient for this?

Yes, the OS controls are sufficient for cases like that.

do you have fun and time to add the midi inhibit feature as well? All these config options are IMHO only a bandaid for it. If not, no problem than we can do it in an other PR.

I think that would be redundant. When would Mixxx be receiving MIDI signals and you would want to inhibit the screensaver but no decks would be playing?

@daschuer
Copy link
Member

do you have fun and time to add the midi inhibit feature as well? All these config options are IMHO only a bandaid for it. If not, no problem than we can do it in an other PR.

I think that would be redundant. When would Mixxx be receiving MIDI signals and you would want to inhibit the screensaver but no decks would be playing?

It is the question when the screen-saver should kick in. With the current preferences there is no chance to have the screensaver active when the deck is playing, but inhibit it during controller actions.

It is "normal" that the screensaver is activated the specified time after the last user action. If this would work for use action via a controller in the same way as for actions via keyboard and mouse, there is IMHO no strong requirement for tweaking the additional preference options, it will "just work".

@JosepMaJAZ
Copy link
Contributor Author

@daschuer : As @Be-ing said, the controller part is not really needed, and if allowed, it's just an option to finegrain a bit more when the screensaver can activate.

If I have the screensaver at some relatively high time, say 20 minutes, it is to be expected that I would use the controller at least once during that time.
If I have the screensaver at some relatively low time, say 5 minutes, it can happen with a long track that the screensaver activates even when using the controller, but the prevent on play mode would prevent it.

In itself, the option to inform the OS about actions when the controller is being used is good, but the implementation on the side of the screensaverhelper also needs to be changed ( not using continuous in windows, using the "SimulateUserActivity" on gnome... or the plain dumb method of simulating actions, which not all OSs interpret as actions":
https://forum.qt.io/topic/52771/solved-screensaver-disable-screensaver-on-windows
)

@JosepMaJAZ
Copy link
Contributor Author

Ok, I finally added code to the ScreenSaverHelper to allow to trigger user activity and implemented it on Controller::receive and on MidiController::receive() . This is called always independently of the screensaver setting.
Said that, the prevent on play might not be working 100% (i have to check why).

This requires testing on Linux and Mac again

@daschuer
Copy link
Member

Great, Thank you!

if (userActivityInhibitTimer.elapsed() > 1000) {
mixxx::ScreenSaverHelper::triggerUserActivity();
userActivityInhibitTimer.start();
}
Copy link
Member

Choose a reason for hiding this comment

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

MidiController inherits from Controller. So this code should be moved to a Controller function and called from here.

void ScreenSaverHelper::uninhibitInternal()
{
qError("Screensaver suspending not implemented");
}
Copy link
Member

Choose a reason for hiding this comment

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

is triggerUserActivity() missing in this #else branch

#else
void ScreenSaverHelper::inhibitInternal()
{
qError("Screensaver suspending not implemented");
Copy link
Member

Choose a reason for hiding this comment

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

This and below should be a DEBUG_ASSERT(!"Screensaver suspending not implemented") to avoid log file spam in a release version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmmm.... qError seemed ok thinking that the feature had to be enabled. But the default is precisely enabled, and in case of triggerAction it is always enabled.
Maybe I could have a variable to print it just once.
This case will happen on unusual versions ( Unix, special versions of linux, maybe raspberry...).
The end result is that the feature will not be available, but what is the most correct thing to do? I was thinking about disabling the setting on the preferences page, and maybe force its value on startup to allow screensaver.

Copy link
Member

Choose a reason for hiding this comment

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

My idea was, that it is an error, but only one the developers should bother, that`s why I have proposed a DEBUG_ASSERT.
An
#error not implemented
will work even better.
I am nor sure if it is worth to spend extra work on this unlikely case.
Fixing the log spam issue by a boolean will work for me as well.

@JosepMaJAZ JosepMaJAZ changed the title [WIP] Screensaver inhibitor. Screensaver inhibitor. Mar 29, 2017
@daschuer
Copy link
Member

daschuer commented Apr 5, 2017

The preferences option work well using Ubuntu Trusty + Cinnamon.

Unfortunately the contoller actvity does not work:

Warning [Controller]: Call to inhibit for  org.freedesktop.ScreenSaver  failed:  "This method is not implemented" 
Warning [Controller]: Call to inhibit for  org.gnome.ScreenSaver  failed:  "Unexpected reply signature: got "no signature", expected "u" (uint)" 
Warning [Controller]: Could not send activity using the registered DBus methods.  Will not try again until the program is restarted.   

@daschuer
Copy link
Member

daschuer commented Apr 6, 2017

Cinnamon has its own org.cinnamon.ScreenSaver

But I do not think it is a good idea to probe for all desktop environments to have this feature.
It should work if we rely on freedesktop.

I have read a bit and it looks like the freedesktop api was stripped down intentionally.
https://people.freedesktop.org/~hadess/idle-inhibition-spec/
https://lists.freedesktop.org/archives/xdg/2012-November/012577.html

But there is no word what to do instead.

Ins an inhibit uninhibit cycle a good idea? Or should we better fake a key-press?

@JosepMaJAZ
Copy link
Contributor Author

JosepMaJAZ commented Apr 6, 2017 via email

@daschuer
Copy link
Member

daschuer commented Apr 6, 2017

This is an interesting bug:
https://bugzilla.gnome.org/show_bug.cgi?id=579430

It looks like we can
XResetScreenSaver

or, if that does not work:
XTestFakeKeyEvent

Example:
https://github.com/sebastiandeutsch/snes-sdk/blob/aea27e607a4ccaad9e5da1c35e4046ba1289349c/bsnes/src/ui_qt/platform/platform_x.cpp

@JosepMaJAZ
Copy link
Contributor Author

I fixed the error that the D-bus call gave to you, so it should work with dbus for you now.
I tried the fakekey method and didn't even get it to compile.
Said that, the dbus method didn't work for me and I ended adding another workaround with xlib.

@daschuer
Copy link
Member

daschuer commented Apr 6, 2017

I fixed the error that the D-bus call gave to you, so it should work with dbus for you now.
I tried the fakekey method and didn't even get it to compile.
Said that, the dbus method didn't work for me and I ended adding another workaround with xlib.

Thanks!

I use cinnamon on unbuntu and the gnome screensaver is installed as well.
In my case it probes freedesktop, which will never work, because it is deprecated.
Then it calls the "wrong" screensaver gnome, which finally calls XResetScreenSaver internally.

As reported in https://bugzilla.gnome.org/show_bug.cgi?id=579430 this call is required, but it is missing in
the cinnamon version.

If we have good results that just XResetScreenSaver works, we should only call this.
I hope I will find time soon to test it.

@JosepMaJAZ
Copy link
Contributor Author

The failed checks are an unrelated test ( midi controller presets) and the mac os build that was canceled or had some problem starting.

@daschuer
Copy link
Member

daschuer commented Apr 7, 2017

Here the results:

  • The current implementation uses the wrong screen saver in the cinnamon case. It inhibits the scrrensaver well, but when I to disable the screensaver via the controller, the cinnamon bars are gone.
  • XResetScreenSaver works to inhibit the screensaver/power saving as well. It is also able to switch on a screen after power saving without destroying cinnamon. Unfortunately it is not able to activate the log-in dialog in case the screen is locked.
  • If I swap gnome and cinnamon in USERACTIVITY it works for cinnamon, but not for Unity.

I see two options to solve it:

  1. Only use XResetScreenSaver. For unlocking a locked screen, you need to use the keyboard anyway, so just enabling the screen can be sufficient.
  2. check for the active desktop environment and use the right screensaver.

In any case, we should not try to use freedesktop, since we already know that it will not work.

from xdg-screensaver:

detectDE()
{
    if [ x"$KDE_FULL_SESSION" = x"true" ]; then DE=kde;
    elif [ x"$GNOME_DESKTOP_SESSION_ID" != x"" ]; then DE=gnome;
    elif `dbus-send --print-reply --dest=org.freedesktop.DBus /org/freedesktop/DBus org.freedesktop.DBus.GetNameOwner string:org.gnome.SessionManager > /dev/null 2>&1` ; then DE=gnome;
    elif xprop -root _DT_SAVE_MODE 2> /dev/null | grep ' = \"xfce4\"$' >/dev/null 2>&1; then DE=xfce;
    elif [ x"$DESKTOP_SESSION" = x"LXDE" ]; then DE=lxde;
    else DE=""
    fi
}

@JosepMaJAZ
Copy link
Contributor Author

Ok, commited. Now only XResetScreensaver.
I also tested it with xfce. It works for the power settings but not for the screensaver (the dbus method probably doesn't work for that either).

@radusuciu
Copy link
Contributor

radusuciu commented Apr 8, 2017

Thanks for working on this! I've had the screensaver mess me up a few times when I forget to change my power settings.

@daschuer
Copy link
Member

daschuer commented Apr 9, 2017

Strange: ControllerPresetValidationTest.MidiPresetsValid fails on Windows, but there is no entry why it fails.
I assume that is is ether a AppVeyor issue or a merge issue, and not related to your latest changes.

Else LGTM.

Thank you for this nice complete solution.

@daschuer daschuer merged commit d9f3683 into mixxxdj:master Apr 9, 2017
@rryan
Copy link
Member

rryan commented Apr 16, 2017

Sorry for not making time to send comments about this PR before it was merged! And apologies if you already discussed some of these. I skimmed the thread.

Blocking?

None of these APIs seem to say anything about whether they block or not -- is it wise to be calling them in the receive loop of the controller? Maybe we should proxy the message from the ControllerManager thread to the main thread.

Thread Safety

Since ScreenSaverHelper is called from multiple threads, do we need to worry about thread safety of these libraries we're using to interact with system? It looks like we might present the same cookie/assertion ID to the system multiple times for two concurrent calls to ScreenSaverHelper? Can anything else bad happen? I'm assuming these system functions are re-entrant, but is that a safe assumption?

Cleanups

What happens if Mixxx crashes? The OS presumably automatically releases the wakelocks we took?

@JosepMaJAZ
Copy link
Contributor Author

JosepMaJAZ commented Apr 16, 2017 via email

@JosepMaJAZ JosepMaJAZ deleted the prevent-screensaver branch June 2, 2017 19:52
@esbrandt esbrandt mentioned this pull request Jun 24, 2017
37 tasks
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.

7 participants