> On Oct. 24, 2012, 11:09 a.m., Oliver Henshaw wrote: > > powerdevil/daemon/powerdevilactionpool.cpp, lines 102-109 > > <http://git.reviewboard.kde.org/r/106863/diff/3/?file=90668#file90668line102> > > > > Is isSupported checked everywhere it should be? Remember that this > > should work when DPMS is configured in but the display doesn't support DPMS > > (e.g. qxl or vmware displays in a VM) > > Kai Uwe Broulik wrote: > I ifdef'd out all the DPMS-related stuff but it would probably also be > good to add a > if (action) { > if (action->isSupported()) { > action->onProfileLoad(); > } > } else { … > to powerdevilcore so it doesn't load an action if it is not supported. > > Oliver Henshaw wrote: > Guarding onProfileLoad won't be enough if/when PowerDevilDPMS action is > patched to use KIdleTime timeouts rather than DPMS timeouts. Plus this > complicates PowerDevilAction semantics, at least in my view. > > Sorry I don't have anything more constructive to offer. It would be > easier if there were a Solid::Display class to query in the profilegenerator, > as in my original suggestion (or maybe the extremely new libkscreen might be > more appropriate). Maybe this is more long term work though. > > Oliver Henshaw wrote: > Hmm, how about your addition as well as (uncompiled, untested, > uncommented): > > diff --git a/powerdevil/daemon/powerdevilactionpool.cpp > b/powerdevil/daemon/powerdevilactionpool.cpp > index a9950f1..fae35ee 100644 > --- a/powerdevil/daemon/powerdevilactionpool.cpp > +++ b/powerdevil/daemon/powerdevilactionpool.cpp > @@ -125,7 +125,7 @@ Action* ActionPool::loadAction(const QString& > actionId, const KConfigGroup& grou > if (m_actionPool.contains(actionId)) { > Action *retaction = m_actionPool[actionId]; > > - if (group.isValid()) { > + if (group.isValid() && retaction->isSupported()) { > > if (m_activeActions.contains(actionId)) { > // We are reloading the action: let's unload it first > then. > > > Which should be right as long as isSupported never changes at run-time > and as long as there's not some gotcha I've not seen yet. > > Oliver Henshaw wrote: > Forgot to mention that that diff relies on > https://git.reviewboard.kde.org/r/106692/ > > Kai Uwe Broulik wrote: > I think isChanged _can_ change at runtime, is does a query to X11 when > you call it. Don't know if _that_ can change though. Don't know if it > does/can when you attach another monitor. > > Dario Freddi wrote: > I agree with the concerns Oliver has. I need to think a bit more over > this, but indeed this has to be fixed somehow.
I added this to the kde 4.10 feature plan so we have some additional time to think of something :) - Kai Uwe ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106863/#review20781 ----------------------------------------------------------- On Oct. 15, 2012, 4:17 p.m., Kai Uwe Broulik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106863/ > ----------------------------------------------------------- > > (Updated Oct. 15, 2012, 4:17 p.m.) > > > Review request for Solid. > > > Description > ------- > > That message usually appears when starting before the Desktop is up, causing > an ugly 1990's passivepopup dialog on the screen, and its contents are not > really novice-user-resolvable. > On my machine it always claims "The profile Battery tried to activate > DPMSControl which is a non-existent action.", which is when I compile > powerdevil myself that DPMS stuff is not compiled (DPMS build requirements > not met here) and so the action floats around in the config but cannot be > triggered anyways. (Imho this is a really infamous message, have seen it > quite often on other machines *duck*). All the other actions seem to be > installed anyways, so this missing action poses no threat. I guess a kWarning > would be sufficient for this. > > > Diffs > ----- > > powerdevil/daemon/actions/CMakeLists.txt db9ca47 > powerdevil/daemon/actions/dpms/powerdevildpmsaction.cpp a16bf7e > powerdevil/daemon/powerdevilactionpool.cpp 484c271 > > Diff: http://git.reviewboard.kde.org/r/106863/diff/ > > > Testing > ------- > > Compiles. > The previous passivepopup does not appear anymore. Did not test whether the > kwarning is triggered, though. (Dunno how to get powerdevil debug console > output) > > > Thanks, > > Kai Uwe Broulik > >
_______________________________________________ Kde-hardware-devel mailing list Kde-hardware-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-hardware-devel
