Re: Review Request 129651: [dragon] Inhibit powersave

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129651/#review101502
-----------------------------------------------------------




src/app/mainWindow.cpp (line 821)
<https://git.reviewboard.kde.org/r/129651/#comment67944>

    QStringLiteral please.
    
    explanation of what QLatin1String does and what QStringLiteral does
    https://woboq.com/blog/qstringliteral.html
    
    Long story short: only use QLatin1String iff you have an ascii `const char[]` and need to pass it to a QL1S supporting function. e.g. `string.startsWith(...)` would be faster to call with QL1S as there is a specific override to handle QL1S http://doc.qt.io/qt-5/qstring.html#startsWith-2



src/app/mainWindow.cpp (line 822)
<https://git.reviewboard.kde.org/r/129651/#comment67946>

    Please no whitespaces surrounding arguments.



src/app/mainWindow.cpp (line 829)
<https://git.reviewboard.kde.org/r/129651/#comment67947>

    You are preventing shutdown here. As I understand it there is no if-else if you only use login1 as logind doesn't have control over screen power saving.



src/app/mainWindow.cpp (line 830)
<https://git.reviewboard.kde.org/r/129651/#comment67945>

    Seeing as this introduces a new i18n string this can't go into Application/16.12 unless you make them strings literal for the branch or get an exception from the i18n team. https://mail.kde.org/mailman/listinfo/kde-i18n-doc
    
    Either is fine since the string is probably not actually user visible anywhere.



src/app/mainWindow.cpp 
<https://git.reviewboard.kde.org/r/129651/#comment67948>

    It would be good to add a FIXME or TODO comment about the lack of screen power saving supression. I think KNotificationRestriction won't actually do that.


- Harald Sitter


On Dec. 19, 2016, 6:36 a.m., Anthony Fieroni wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129651/
> -----------------------------------------------------------
> 
> (Updated Dec. 19, 2016, 6:36 a.m.)
> 
> 
> Review request for KDE Multimedia and Harald Sitter.
> 
> 
> Repository: dragon
> 
> 
> Description
> -------
> 
> Since solid power api is not released, i use freedesktop dbus to fit other desktop power managers
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 8f84c85 
>   src/app/CMakeLists.txt 303e06e 
>   src/app/mainWindow.cpp 01a4545 
> 
> Diff: https://git.reviewboard.kde.org/r/129651/diff/
> 
> 
> Testing
> -------
> 
> *Harald master was not merged (16.04 / 16.08 to master) -> it has fresh bugreports for fixed issues.
> Last merge to 16.08
> 2016-04-18	Merge branch 'Applications/16.04'	Harald Sitter
> after that in brach 16.04
> 2016-04-26	Hide toolbar on multiscreen setup	Anthony Fieroni
> 2016-04-20	Don't try to resize maximized window	Anthony Fieroni
> 2016-04-19	MPRIS2 property 'Identity' to match desktop file entry	Anthony Fieroni
> 
> Do you want to merge them or you will ship ?
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>