Re: Review Request 125802: Split generic parts from X11Locker into AbstractLocker

[Bhushan Shah]

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

(Updated Oct. 26, 2015, 1:02 p.m.)


Review request for Plasma and Martin Gräßlin.


Changes
---

Fix issues

- Add previous copyrights
- Don't change position of initialize()

I hope this revision is readable, done with different options.


Repository: plasma-workspace


Description
---

Advantage of this split is some parts such as showLockWindow and hideLockWindow 
needs platform specific code. So instead of doing complex if/else branching we 
can have seperate X11Locker and WaylandLocker.


Diffs (updated)
-

  ksmserver/screenlocker/abstractlocker.cpp PRE-CREATION 
  ksmserver/screenlocker/CMakeLists.txt 24a89f6 
  ksmserver/screenlocker/abstractlocker.h PRE-CREATION 
  ksmserver/screenlocker/autotests/CMakeLists.txt ab901ca 
  ksmserver/screenlocker/autotests/lockwindowtest.cpp 444c44f 
  ksmserver/screenlocker/autotests/x11lockertest.cpp PRE-CREATION 
  ksmserver/screenlocker/ksldapp.h b281ab6 
  ksmserver/screenlocker/ksldapp.cpp 68ebfc2 
  ksmserver/screenlocker/lockwindow.h c4013be 
  ksmserver/screenlocker/lockwindow.cpp e46733e 
  ksmserver/screenlocker/x11locker.h PRE-CREATION 
  ksmserver/screenlocker/x11locker.cpp PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/125802/diff/


Testing
---

builds, installs, autotests and kscreenlocker_test passes, normal lockscreen 
also works.


Thanks,

Bhushan Shah

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 125802: Split generic parts from X11Locker into AbstractLocker

[Martin Gräßlin]

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


+1, looks good to me. Let's give it some time for other devs to also have a 
look at it.

- Martin Gräßlin


On Oct. 26, 2015, 9:11 a.m., Bhushan Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125802/
> ---
> 
> (Updated Oct. 26, 2015, 9:11 a.m.)
> 
> 
> Review request for Plasma and Martin Gräßlin.
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> Advantage of this split is some parts such as showLockWindow and 
> hideLockWindow needs platform specific code. So instead of doing complex 
> if/else branching we can have seperate X11Locker and WaylandLocker.
> 
> 
> Diffs
> -
> 
>   ksmserver/screenlocker/CMakeLists.txt 24a89f6 
>   ksmserver/screenlocker/abstractlocker.h PRE-CREATION 
>   ksmserver/screenlocker/abstractlocker.cpp PRE-CREATION 
>   ksmserver/screenlocker/autotests/CMakeLists.txt ab901ca 
>   ksmserver/screenlocker/autotests/lockwindowtest.cpp 444c44f 
>   ksmserver/screenlocker/autotests/x11lockertest.cpp PRE-CREATION 
>   ksmserver/screenlocker/ksldapp.h b281ab6 
>   ksmserver/screenlocker/ksldapp.cpp 68ebfc2 
>   ksmserver/screenlocker/lockwindow.h c4013be 
>   ksmserver/screenlocker/lockwindow.cpp e46733e 
> 
> Diff: https://git.reviewboard.kde.org/r/125802/diff/
> 
> 
> Testing
> ---
> 
> builds, installs, autotests and kscreenlocker_test passes, normal lockscreen 
> also works.
> 
> 
> Thanks,
> 
> Bhushan Shah
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 125802: Split generic parts from X11Locker into AbstractLocker

[Martin Gräßlin]

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


Difficult to read on reviewboard. Overall it looks good to me, but will need 
another look at it.


ksmserver/screenlocker/lockwindow.h (line 5)


I would keep the previous copyrights. In my opinion they all still have 
copyright on it given that it's based on their work.



ksmserver/screenlocker/lockwindow.h (lines 58 - 59)


why did you switch the position of initialize()?


- Martin Gräßlin


On Oct. 26, 2015, 8:17 a.m., Bhushan Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125802/
> ---
> 
> (Updated Oct. 26, 2015, 8:17 a.m.)
> 
> 
> Review request for Plasma and Martin Gräßlin.
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> Advantage of this split is some parts such as showLockWindow and 
> hideLockWindow needs platform specific code. So instead of doing complex 
> if/else branching we can have seperate X11Locker and WaylandLocker.
> 
> 
> Diffs
> -
> 
>   ksmserver/screenlocker/autotests/CMakeLists.txt ab901ca 
>   ksmserver/screenlocker/abstractlocker.cpp PRE-CREATION 
>   ksmserver/screenlocker/CMakeLists.txt 24a89f6 
>   ksmserver/screenlocker/autotests/lockwindowtest.cpp 444c44f 
>   ksmserver/screenlocker/ksldapp.h b281ab6 
>   ksmserver/screenlocker/ksldapp.cpp 68ebfc2 
>   ksmserver/screenlocker/lockwindow.h c4013be 
>   ksmserver/screenlocker/lockwindow.h c4013be 
>   ksmserver/screenlocker/lockwindow.cpp e46733e 
> 
> Diff: https://git.reviewboard.kde.org/r/125802/diff/
> 
> 
> Testing
> ---
> 
> builds, installs, autotests and kscreenlocker_test passes, normal lockscreen 
> also works.
> 
> 
> Thanks,
> 
> Bhushan Shah
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 125802: Split generic parts from X11Locker into AbstractLocker

[Bhushan Shah]

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

(Updated Oct. 26, 2015, 1:41 p.m.)


Review request for Plasma and Martin Gräßlin.


Changes
---

Fix issues, as well as revert move of lockwindow.{h,cpp} -> x11locker.{h,cpp} 
for ease in reviewing.


Repository: plasma-workspace


Description
---

Advantage of this split is some parts such as showLockWindow and hideLockWindow 
needs platform specific code. So instead of doing complex if/else branching we 
can have seperate X11Locker and WaylandLocker.


Diffs (updated)
-

  ksmserver/screenlocker/CMakeLists.txt 24a89f6 
  ksmserver/screenlocker/abstractlocker.h PRE-CREATION 
  ksmserver/screenlocker/abstractlocker.cpp PRE-CREATION 
  ksmserver/screenlocker/autotests/CMakeLists.txt ab901ca 
  ksmserver/screenlocker/autotests/lockwindowtest.cpp 444c44f 
  ksmserver/screenlocker/autotests/x11lockertest.cpp PRE-CREATION 
  ksmserver/screenlocker/ksldapp.h b281ab6 
  ksmserver/screenlocker/ksldapp.cpp 68ebfc2 
  ksmserver/screenlocker/lockwindow.h c4013be 
  ksmserver/screenlocker/lockwindow.cpp e46733e 

Diff: https://git.reviewboard.kde.org/r/125802/diff/


Testing
---

builds, installs, autotests and kscreenlocker_test passes, normal lockscreen 
also works.


Thanks,

Bhushan Shah

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 125802: Split generic parts from X11Locker into AbstractLocker

[Martin Gräßlin]

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



ksmserver/screenlocker/abstractlocker.h (line 66)


stayOnTop is public in the Abstract class, but private in the implementing 
class. Is it supposed to be public? If not I suggest to move to protected.



ksmserver/screenlocker/x11locker.h (lines 47 - 48)


add "override" to the overriden methods


- Martin Gräßlin


On Oct. 26, 2015, 8:32 a.m., Bhushan Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125802/
> ---
> 
> (Updated Oct. 26, 2015, 8:32 a.m.)
> 
> 
> Review request for Plasma and Martin Gräßlin.
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> Advantage of this split is some parts such as showLockWindow and 
> hideLockWindow needs platform specific code. So instead of doing complex 
> if/else branching we can have seperate X11Locker and WaylandLocker.
> 
> 
> Diffs
> -
> 
>   ksmserver/screenlocker/abstractlocker.cpp PRE-CREATION 
>   ksmserver/screenlocker/CMakeLists.txt 24a89f6 
>   ksmserver/screenlocker/abstractlocker.h PRE-CREATION 
>   ksmserver/screenlocker/autotests/CMakeLists.txt ab901ca 
>   ksmserver/screenlocker/autotests/lockwindowtest.cpp 444c44f 
>   ksmserver/screenlocker/autotests/x11lockertest.cpp PRE-CREATION 
>   ksmserver/screenlocker/ksldapp.h b281ab6 
>   ksmserver/screenlocker/ksldapp.cpp 68ebfc2 
>   ksmserver/screenlocker/lockwindow.h c4013be 
>   ksmserver/screenlocker/lockwindow.cpp e46733e 
>   ksmserver/screenlocker/x11locker.h PRE-CREATION 
>   ksmserver/screenlocker/x11locker.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125802/diff/
> 
> 
> Testing
> ---
> 
> builds, installs, autotests and kscreenlocker_test passes, normal lockscreen 
> also works.
> 
> 
> Thanks,
> 
> Bhushan Shah
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 125802: Split generic parts from X11Locker into AbstractLocker

[David Edmundson]

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

Ship it!


ship it!

I include a grumble about something that existed before this patch, so you can 
consier it later (or ignore it)


ksmserver/screenlocker/abstractlocker.h (line 68)


I don't like this set.

It means if you use globalAccel() from X11Locker's constructor, you crash.

We currently don't, but it's leaving the safety off for the next person who 
edits this file.

I would either:
 - make a public accessor in KSLApp (which owns this object) and port code 
to  KSLDApp::self()->globalAccel(), getting rid m_globalAccel here.

 - pass it in the constructor to AbstractLocker


- David Edmundson


On Oct. 26, 2015, 11:37 a.m., Bhushan Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125802/
> ---
> 
> (Updated Oct. 26, 2015, 11:37 a.m.)
> 
> 
> Review request for Plasma, David Edmundson and Martin Gräßlin.
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> Advantage of this split is some parts such as showLockWindow and 
> hideLockWindow needs platform specific code. So instead of doing complex 
> if/else branching we can have seperate X11Locker and WaylandLocker.
> 
> 
> Diffs
> -
> 
>   ksmserver/screenlocker/CMakeLists.txt 24a89f6 
>   ksmserver/screenlocker/abstractlocker.h PRE-CREATION 
>   ksmserver/screenlocker/abstractlocker.cpp PRE-CREATION 
>   ksmserver/screenlocker/autotests/CMakeLists.txt ab901ca 
>   ksmserver/screenlocker/autotests/lockwindowtest.cpp 444c44f 
>   ksmserver/screenlocker/autotests/x11lockertest.cpp PRE-CREATION 
>   ksmserver/screenlocker/ksldapp.h b281ab6 
>   ksmserver/screenlocker/ksldapp.cpp 68ebfc2 
>   ksmserver/screenlocker/lockwindow.h c4013be 
>   ksmserver/screenlocker/lockwindow.cpp e46733e 
> 
> Diff: https://git.reviewboard.kde.org/r/125802/diff/
> 
> 
> Testing
> ---
> 
> builds, installs, autotests and kscreenlocker_test passes, normal lockscreen 
> also works.
> 
> 
> Thanks,
> 
> Bhushan Shah
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 125802: Split generic parts from X11Locker into AbstractLocker

[Bhushan Shah]

> On Oct. 26, 2015, 5:32 p.m., David Edmundson wrote:
> > ksmserver/screenlocker/abstractlocker.h, line 68
> > 
> >
> > I don't like this set.
> > 
> > It means if you use globalAccel() from X11Locker's constructor, you 
> > crash.
> > 
> > We currently don't, but it's leaving the safety off for the next person 
> > who edits this file.
> > 
> > 
> > I would either:
> >  - make a public accessor in KSLApp (which owns this object) and port 
> > code to  KSLDApp::self()->globalAccel(), getting rid m_globalAccel here.
> > 
> >  - pass it in the constructor to AbstractLocker

I will do it later in seperate review request.. currently I've 2-3 other 
patches depending upon this one and rebasing them would be mess I fear..


- Bhushan


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


On Oct. 26, 2015, 5:07 p.m., Bhushan Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125802/
> ---
> 
> (Updated Oct. 26, 2015, 5:07 p.m.)
> 
> 
> Review request for Plasma, David Edmundson and Martin Gräßlin.
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> Advantage of this split is some parts such as showLockWindow and 
> hideLockWindow needs platform specific code. So instead of doing complex 
> if/else branching we can have seperate X11Locker and WaylandLocker.
> 
> 
> Diffs
> -
> 
>   ksmserver/screenlocker/CMakeLists.txt 24a89f6 
>   ksmserver/screenlocker/abstractlocker.h PRE-CREATION 
>   ksmserver/screenlocker/abstractlocker.cpp PRE-CREATION 
>   ksmserver/screenlocker/autotests/CMakeLists.txt ab901ca 
>   ksmserver/screenlocker/autotests/lockwindowtest.cpp 444c44f 
>   ksmserver/screenlocker/autotests/x11lockertest.cpp PRE-CREATION 
>   ksmserver/screenlocker/ksldapp.h b281ab6 
>   ksmserver/screenlocker/ksldapp.cpp 68ebfc2 
>   ksmserver/screenlocker/lockwindow.h c4013be 
>   ksmserver/screenlocker/lockwindow.cpp e46733e 
> 
> Diff: https://git.reviewboard.kde.org/r/125802/diff/
> 
> 
> Testing
> ---
> 
> builds, installs, autotests and kscreenlocker_test passes, normal lockscreen 
> also works.
> 
> 
> Thanks,
> 
> Bhushan Shah
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 125802: Split generic parts from X11Locker into AbstractLocker

[Bhushan Shah]

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

(Updated Oct. 26, 2015, 1:36 p.m.)


Status
--

This change has been marked as submitted.


Review request for Plasma, David Edmundson and Martin Gräßlin.


Changes
---

Submitted with commit 5a0ab38613763ab50a0e1315615163a48fdb753b by Bhushan Shah 
to branch master.


Repository: plasma-workspace


Description
---

Advantage of this split is some parts such as showLockWindow and hideLockWindow 
needs platform specific code. So instead of doing complex if/else branching we 
can have seperate X11Locker and WaylandLocker.


Diffs
-

  ksmserver/screenlocker/CMakeLists.txt 24a89f6 
  ksmserver/screenlocker/abstractlocker.h PRE-CREATION 
  ksmserver/screenlocker/abstractlocker.cpp PRE-CREATION 
  ksmserver/screenlocker/autotests/CMakeLists.txt ab901ca 
  ksmserver/screenlocker/autotests/lockwindowtest.cpp 444c44f 
  ksmserver/screenlocker/autotests/x11lockertest.cpp PRE-CREATION 
  ksmserver/screenlocker/ksldapp.h b281ab6 
  ksmserver/screenlocker/ksldapp.cpp 68ebfc2 
  ksmserver/screenlocker/lockwindow.h c4013be 
  ksmserver/screenlocker/lockwindow.cpp e46733e 

Diff: https://git.reviewboard.kde.org/r/125802/diff/


Testing
---

builds, installs, autotests and kscreenlocker_test passes, normal lockscreen 
also works.


Thanks,

Bhushan Shah

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel