Review Request 112076: Fix bug where monitor won't powersave even with only audio content

[James Daniel Smith]

On August 27, 2013 11:05:05 AM you wrote:
> > On Aug. 26, 2013, 9:12 a.m., Harald Sitter wrote:
> > > src/app/mainWindow.cpp, lines 866-887
> > > <http://git.reviewboard.kde.org/r/112076/diff/5/?file=184675#file184675l
> > > ine866>> > 
> > >     no unrelated format changes please
> > 
> > James Smith wrote:
> >     Should I have a separate patch then to fix some of the style issues or
> >     just leave it after the bulk of the issues have been ironed out?
> just leave it, they are isolated to functions
> 
> > On Aug. 26, 2013, 9:12 a.m., Harald Sitter wrote:
> > > src/app/mainWindow.cpp, line 864
> > > <http://git.reviewboard.kde.org/r/112076/diff/5/?file=184675#file184675l
> > > ine864>> > 
> > >     the f(bool) signature is not necessary here.
> > >     
> > >     you can simply use TheStream::hasVideo() inside the supression
> > >     function as that is what the entire logic depends on anyway.> 
> > James Smith wrote:
> >     TheStream::hasVideo() didn't work when I originally attempted a quick
> >     patch to fix the issue. Thus I needed to use stateChanged to have the
> >     bug fixed.>>> 
> >>> grep hasVideo src/app/mainWindow.cpp |wc -l
> 
> 4
> 
> I am reasonable certain it works :P
> What I just remembered though... Phonon dispatches this information as a
> signal (i.e. state changes to playing; 3 seconds later; signal for
> hasVideoChanged arrives indicating this is audio-only content), so checking
> hasVideo() at the time of state change can lead to wrong behavior (e.g.
> hasVideo=false; 3 seconds later; hasVideo=true).
> 
> So yes, likely engineHasVideoChanged() is where you want to have additional
> logic.

That didn't enable screensaver suppression as expected, so the rest of the 
changes outlayed below are also fairly obtuse in consideration. Unfortunately, 
the original patch looks like the best avenue, being cleaner and with less 
readability issues, and ultimately, functions.

Thanks!
James

> 
> Perhaps something like this:
> new function MainWindow::inhibitSleep() [example name :S; contains sleep
> logic from inhibitPowersave()] new function MainWindow::releaseSleep()
> [contains release for sleep logic from releasePowersave()[] new function
> MainWindow::inhibitScreenSleep() [contains screen power management from
> inhibitPowersave()] new function MainWindow::releaseScreenSleep() [contains
> release for screen power management from releasePowersave()] remove
> function MainWindow::inhibitPowersave() [release can be kept around as
> convenience function) MainWindow::engineStateChanged(...) calls
> MainWindow::inhibitSleep() [assumption: we always have audio, so we always
> want to prevent sleeping] MainWindow::engineHasVideoChanged(...) it calls
> MainWindow::inhibitScreenSleep() if hasVideo=true, or calls
> MainWindow::releaseScreenSleep() if hasVideo=no [overall condition: state
> must be playing] [on state change to !playing we continue to call
> releasePowersave(), which in turn calls both releseSleep() and
> releaseScreenSleep()]
> 
> ultimate result is then that we always prevent sleeping
> 
> 
> - Harald
> 
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112076/#review38595
> -----------------------------------------------------------
> 
> On Aug. 27, 2013, 1:15 a.m., James Smith wrote:
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > http://git.reviewboard.kde.org/r/112076/
> > -----------------------------------------------------------
> > 
> > (Updated Aug. 27, 2013, 1:15 a.m.)
> > 
> > 
> > Review request for KDE Multimedia and Harald Sitter.
> > 
> > 
> > Description
> > -------
> > 
> > Fixes an issue where playing audio content keeps the screen awake. Video
> > content is supposed to, but with the new play URL button, dragon should
> > allow the monitor to turn off.
> > 
> > 
> > Diffs
> > -----
> > 
> >   src/app/mainWindow.h 51ef72c
> >   src/app/mainWindow.cpp f850820
> >   src/app/stateChange.cpp 0edde72
> > 
> > Diff: http://git.reviewboard.kde.org/r/112076/diff/
> > 
> > 
> > Testing
> > -------
> > 
> > Compile, run-test
> > 
> > 
> > Thanks,
> > 
> > James Smith