Re: Review Request 121915: Add lidClosedChanged signal to org.kde.Solid.PowerManagement
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/ --- (Updated Sept. 2, 2015, 11:38 a.m.) Status -- This change has been marked as submitted. Review request for Plasma and Solid. Changes --- Submitted with commit b5a1e1bec3052460ff8c87764a065cc61cdeba47 by Dan Vrátil to branch master. Repository: powerdevil Description --- There's no way to detect when lid has been closed other than listening to `changed` signal org.freedesktop.UPower and then polling the Powerdevil for new values. This patch adds a new signal to the PowerDevil interface to notify about the change and provide new value right away. Makes it much easier to use. Diffs - daemon/org.kde.Solid.PowerManagement.xml 53f77e5 daemon/powerdevilbackendinterface.h 702b66b daemon/powerdevilbackendinterface.cpp 6dd8c71 daemon/powerdevilcore.h e255703 daemon/powerdevilcore.cpp d51ab19 Diff: https://git.reviewboard.kde.org/r/121915/diff/ Testing --- Tested with qdbus-monitor, signal is emitted when laptop lid is closed/opened. Thanks, Daniel Vrátil ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121915: Add lidClosedChanged signal to org.kde.Solid.PowerManagement
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/#review74374 --- This has been submitted, right? - Kai Uwe Broulik On Jan. 8, 2015, 12:04 nachm., Daniel Vrátil wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/ --- (Updated Jan. 8, 2015, 12:04 nachm.) Review request for Plasma and Solid. Repository: powerdevil Description --- There's no way to detect when lid has been closed other than listening to `changed` signal org.freedesktop.UPower and then polling the Powerdevil for new values. This patch adds a new signal to the PowerDevil interface to notify about the change and provide new value right away. Makes it much easier to use. Diffs - daemon/org.kde.Solid.PowerManagement.xml 53f77e5 daemon/powerdevilbackendinterface.h 702b66b daemon/powerdevilbackendinterface.cpp 6dd8c71 daemon/powerdevilcore.h e255703 daemon/powerdevilcore.cpp d51ab19 Diff: https://git.reviewboard.kde.org/r/121915/diff/ Testing --- Tested with qdbus-monitor, signal is emitted when laptop lid is closed/opened. Thanks, Daniel Vrátil ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121915: Add lidClosedChanged signal to org.kde.Solid.PowerManagement
On gen. 9, 2015, 11:25 a.m., Àlex Fiestas wrote: Where do you plan to use this? We are not maintaining compatibility (so far) in our dbus apis, so why add this at all? Daniel Vrátil wrote: KScreen. For now we are listening to UPower, which IMO sucks and we should use PowerDevil instead (as it provides abstraction for alternative backends). This makes it just easier to monitor changes. Àlex Fiestas wrote: This has a few problems: 1-It couples KScreen more to PLasma by adding a runtime dependency (which is ok if you decide to do so) 2-It might create a deadlock since kscreen is a kded module asking to another module (powerdevil) for things. 3-We will depend on Powerdevil if we use kscreen in other places (sddm desperatly needs something like kscreen, or kscreen itself) 4-Adds a dependency to an api that is not stable (so far it has not been), we have changed it many times and I am sure we will change it again My recommendation for this will be to merge the new power management api in Solid and make kscreen depend on it. Solid already offers backends (so we don't have to implement this in kscreen) and we don't have to expose powerdevil internals. So this is a -1 from my side (if the only reason to add this is KSCreen). Martin Klapetek wrote: 2-It might create a deadlock since kscreen is a kded module asking to another module (powerdevil) for things. As dfaure explained in https://git.reviewboard.kde.org/r/121885/ QtDbus is really smart and in-process DBus calls are made into direct methods and should never deadlock. Which is cool :) Oh wow! One less problem to worry about then. We still need to be careful not to deadlock by doing sync calls between the same kded modules (happened already). - Àlex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/#review73569 --- On gen. 8, 2015, 12:04 p.m., Daniel Vrátil wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/ --- (Updated gen. 8, 2015, 12:04 p.m.) Review request for Plasma and Solid. Repository: powerdevil Description --- There's no way to detect when lid has been closed other than listening to `changed` signal org.freedesktop.UPower and then polling the Powerdevil for new values. This patch adds a new signal to the PowerDevil interface to notify about the change and provide new value right away. Makes it much easier to use. Diffs - daemon/org.kde.Solid.PowerManagement.xml 53f77e5 daemon/powerdevilbackendinterface.h 702b66b daemon/powerdevilbackendinterface.cpp 6dd8c71 daemon/powerdevilcore.h e255703 daemon/powerdevilcore.cpp d51ab19 Diff: https://git.reviewboard.kde.org/r/121915/diff/ Testing --- Tested with qdbus-monitor, signal is emitted when laptop lid is closed/opened. Thanks, Daniel Vrátil ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121915: Add lidClosedChanged signal to org.kde.Solid.PowerManagement
On gen. 9, 2015, 11:25 a.m., Àlex Fiestas wrote: Where do you plan to use this? We are not maintaining compatibility (so far) in our dbus apis, so why add this at all? Daniel Vrátil wrote: KScreen. For now we are listening to UPower, which IMO sucks and we should use PowerDevil instead (as it provides abstraction for alternative backends). This makes it just easier to monitor changes. This has a few problems: 1-It couples KScreen more to PLasma by adding a runtime dependency (which is ok if you decide to do so) 2-It might create a deadlock since kscreen is a kded module asking to another module (powerdevil) for things. 3-We will depend on Powerdevil if we use kscreen in other places (sddm desperatly needs something like kscreen, or kscreen itself) 4-Adds a dependency to an api that is not stable (so far it has not been), we have changed it many times and I am sure we will change it again My recommendation for this will be to merge the new power management api in Solid and make kscreen depend on it. Solid already offers backends (so we don't have to implement this in kscreen) and we don't have to expose powerdevil internals. So this is a -1 from my side (if the only reason to add this is KSCreen). - Àlex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/#review73569 --- On gen. 8, 2015, 12:04 p.m., Daniel Vrátil wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/ --- (Updated gen. 8, 2015, 12:04 p.m.) Review request for Plasma and Solid. Repository: powerdevil Description --- There's no way to detect when lid has been closed other than listening to `changed` signal org.freedesktop.UPower and then polling the Powerdevil for new values. This patch adds a new signal to the PowerDevil interface to notify about the change and provide new value right away. Makes it much easier to use. Diffs - daemon/org.kde.Solid.PowerManagement.xml 53f77e5 daemon/powerdevilbackendinterface.h 702b66b daemon/powerdevilbackendinterface.cpp 6dd8c71 daemon/powerdevilcore.h e255703 daemon/powerdevilcore.cpp d51ab19 Diff: https://git.reviewboard.kde.org/r/121915/diff/ Testing --- Tested with qdbus-monitor, signal is emitted when laptop lid is closed/opened. Thanks, Daniel Vrátil ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121915: Add lidClosedChanged signal to org.kde.Solid.PowerManagement
On Jan. 9, 2015, 12:25 p.m., Àlex Fiestas wrote: Where do you plan to use this? We are not maintaining compatibility (so far) in our dbus apis, so why add this at all? Daniel Vrátil wrote: KScreen. For now we are listening to UPower, which IMO sucks and we should use PowerDevil instead (as it provides abstraction for alternative backends). This makes it just easier to monitor changes. Àlex Fiestas wrote: This has a few problems: 1-It couples KScreen more to PLasma by adding a runtime dependency (which is ok if you decide to do so) 2-It might create a deadlock since kscreen is a kded module asking to another module (powerdevil) for things. 3-We will depend on Powerdevil if we use kscreen in other places (sddm desperatly needs something like kscreen, or kscreen itself) 4-Adds a dependency to an api that is not stable (so far it has not been), we have changed it many times and I am sure we will change it again My recommendation for this will be to merge the new power management api in Solid and make kscreen depend on it. Solid already offers backends (so we don't have to implement this in kscreen) and we don't have to expose powerdevil internals. So this is a -1 from my side (if the only reason to add this is KSCreen). 2-It might create a deadlock since kscreen is a kded module asking to another module (powerdevil) for things. As dfaure explained in https://git.reviewboard.kde.org/r/121885/ QtDbus is really smart and in-process DBus calls are made into direct methods and should never deadlock. Which is cool :) - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/#review73569 --- On Jan. 8, 2015, 1:04 p.m., Daniel Vrátil wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/ --- (Updated Jan. 8, 2015, 1:04 p.m.) Review request for Plasma and Solid. Repository: powerdevil Description --- There's no way to detect when lid has been closed other than listening to `changed` signal org.freedesktop.UPower and then polling the Powerdevil for new values. This patch adds a new signal to the PowerDevil interface to notify about the change and provide new value right away. Makes it much easier to use. Diffs - daemon/org.kde.Solid.PowerManagement.xml 53f77e5 daemon/powerdevilbackendinterface.h 702b66b daemon/powerdevilbackendinterface.cpp 6dd8c71 daemon/powerdevilcore.h e255703 daemon/powerdevilcore.cpp d51ab19 Diff: https://git.reviewboard.kde.org/r/121915/diff/ Testing --- Tested with qdbus-monitor, signal is emitted when laptop lid is closed/opened. Thanks, Daniel Vrátil ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121915: Add lidClosedChanged signal to org.kde.Solid.PowerManagement
On Jan. 9, 2015, 12:25 p.m., Àlex Fiestas wrote: Where do you plan to use this? We are not maintaining compatibility (so far) in our dbus apis, so why add this at all? KScreen. For now we are listening to UPower, which IMO sucks and we should use PowerDevil instead (as it provides abstraction for alternative backends). This makes it just easier to monitor changes. - Daniel --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/#review73569 --- On Jan. 8, 2015, 1:04 p.m., Daniel Vrátil wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/ --- (Updated Jan. 8, 2015, 1:04 p.m.) Review request for Plasma and Solid. Repository: powerdevil Description --- There's no way to detect when lid has been closed other than listening to `changed` signal org.freedesktop.UPower and then polling the Powerdevil for new values. This patch adds a new signal to the PowerDevil interface to notify about the change and provide new value right away. Makes it much easier to use. Diffs - daemon/org.kde.Solid.PowerManagement.xml 53f77e5 daemon/powerdevilbackendinterface.h 702b66b daemon/powerdevilbackendinterface.cpp 6dd8c71 daemon/powerdevilcore.h e255703 daemon/powerdevilcore.cpp d51ab19 Diff: https://git.reviewboard.kde.org/r/121915/diff/ Testing --- Tested with qdbus-monitor, signal is emitted when laptop lid is closed/opened. Thanks, Daniel Vrátil ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121915: Add lidClosedChanged signal to org.kde.Solid.PowerManagement
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/#review73584 --- Ship it! +1 from me - Lukáš Tinkl On Led. 8, 2015, 1:04 odp., Daniel Vrátil wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/ --- (Updated Led. 8, 2015, 1:04 odp.) Review request for Plasma and Solid. Repository: powerdevil Description --- There's no way to detect when lid has been closed other than listening to `changed` signal org.freedesktop.UPower and then polling the Powerdevil for new values. This patch adds a new signal to the PowerDevil interface to notify about the change and provide new value right away. Makes it much easier to use. Diffs - daemon/org.kde.Solid.PowerManagement.xml 53f77e5 daemon/powerdevilbackendinterface.h 702b66b daemon/powerdevilbackendinterface.cpp 6dd8c71 daemon/powerdevilcore.h e255703 daemon/powerdevilcore.cpp d51ab19 Diff: https://git.reviewboard.kde.org/r/121915/diff/ Testing --- Tested with qdbus-monitor, signal is emitted when laptop lid is closed/opened. Thanks, Daniel Vrátil ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121915: Add lidClosedChanged signal to org.kde.Solid.PowerManagement
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/#review73569 --- Where do you plan to use this? We are not maintaining compatibility (so far) in our dbus apis, so why add this at all? - Àlex Fiestas On gen. 8, 2015, 12:04 p.m., Daniel Vrátil wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/ --- (Updated gen. 8, 2015, 12:04 p.m.) Review request for Plasma and Solid. Repository: powerdevil Description --- There's no way to detect when lid has been closed other than listening to `changed` signal org.freedesktop.UPower and then polling the Powerdevil for new values. This patch adds a new signal to the PowerDevil interface to notify about the change and provide new value right away. Makes it much easier to use. Diffs - daemon/org.kde.Solid.PowerManagement.xml 53f77e5 daemon/powerdevilbackendinterface.h 702b66b daemon/powerdevilbackendinterface.cpp 6dd8c71 daemon/powerdevilcore.h e255703 daemon/powerdevilcore.cpp d51ab19 Diff: https://git.reviewboard.kde.org/r/121915/diff/ Testing --- Tested with qdbus-monitor, signal is emitted when laptop lid is closed/opened. Thanks, Daniel Vrátil ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121915: Add lidClosedChanged signal to org.kde.Solid.PowerManagement
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/#review73478 --- daemon/powerdevilbackendinterface.cpp https://git.reviewboard.kde.org/r/121915/#comment51141 emit lidClosedChanged(d-isLidClosed); ? Also you might want to check for changes in the property, so you don't emit too much. - Aleix Pol Gonzalez On Jan. 8, 2015, 11 a.m., Daniel Vrátil wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/ --- (Updated Jan. 8, 2015, 11 a.m.) Review request for Plasma. Repository: powerdevil Description --- There's no way to detect when lid has been closed other than listening to `changed` signal org.freedesktop.UPower and then polling the Powerdevil for new values. This patch adds a new signal to the PowerDevil interface to notify about the change and provide new value right away. Makes it much easier to use. Diffs - daemon/org.kde.Solid.PowerManagement.xml 53f77e5 daemon/powerdevilbackendinterface.h 702b66b daemon/powerdevilbackendinterface.cpp 6dd8c71 daemon/powerdevilcore.h e255703 daemon/powerdevilcore.cpp d51ab19 Diff: https://git.reviewboard.kde.org/r/121915/diff/ Testing --- Tested with qdbus-monitor, signal is emitted when laptop lid is closed/opened. Thanks, Daniel Vrátil ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121915: Add lidClosedChanged signal to org.kde.Solid.PowerManagement
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/ --- (Updated Jan. 8, 2015, 11:58 a.m.) Review request for Plasma and Solid. Repository: powerdevil Description --- There's no way to detect when lid has been closed other than listening to `changed` signal org.freedesktop.UPower and then polling the Powerdevil for new values. This patch adds a new signal to the PowerDevil interface to notify about the change and provide new value right away. Makes it much easier to use. Diffs - daemon/org.kde.Solid.PowerManagement.xml 53f77e5 daemon/powerdevilbackendinterface.h 702b66b daemon/powerdevilbackendinterface.cpp 6dd8c71 daemon/powerdevilcore.h e255703 daemon/powerdevilcore.cpp d51ab19 Diff: https://git.reviewboard.kde.org/r/121915/diff/ Testing --- Tested with qdbus-monitor, signal is emitted when laptop lid is closed/opened. Thanks, Daniel Vrátil ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121915: Add lidClosedChanged signal to org.kde.Solid.PowerManagement
On Jan. 8, 2015, 12:58 p.m., Aleix Pol Gonzalez wrote: daemon/powerdevilbackendinterface.cpp, line 219 https://git.reviewboard.kde.org/r/121915/diff/1/?file=339090#file339090line219 emit lidClosedChanged(d-isLidClosed); ? Also you might want to check for changes in the property, so you don't emit too much. setButtonPressed is generic and might be invoked in different situations, but we only want to emit lidClosedChanged when the button event is LidClosed or LidOpened, that's why the code is the way it is. I'll just add the change guards. - Daniel --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/#review73478 --- On Jan. 8, 2015, 12:58 p.m., Daniel Vrátil wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/ --- (Updated Jan. 8, 2015, 12:58 p.m.) Review request for Plasma and Solid. Repository: powerdevil Description --- There's no way to detect when lid has been closed other than listening to `changed` signal org.freedesktop.UPower and then polling the Powerdevil for new values. This patch adds a new signal to the PowerDevil interface to notify about the change and provide new value right away. Makes it much easier to use. Diffs - daemon/org.kde.Solid.PowerManagement.xml 53f77e5 daemon/powerdevilbackendinterface.h 702b66b daemon/powerdevilbackendinterface.cpp 6dd8c71 daemon/powerdevilcore.h e255703 daemon/powerdevilcore.cpp d51ab19 Diff: https://git.reviewboard.kde.org/r/121915/diff/ Testing --- Tested with qdbus-monitor, signal is emitted when laptop lid is closed/opened. Thanks, Daniel Vrátil ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121915: Add lidClosedChanged signal to org.kde.Solid.PowerManagement
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/ --- (Updated Jan. 8, 2015, 1:04 p.m.) Review request for Plasma and Solid. Repository: powerdevil Description --- There's no way to detect when lid has been closed other than listening to `changed` signal org.freedesktop.UPower and then polling the Powerdevil for new values. This patch adds a new signal to the PowerDevil interface to notify about the change and provide new value right away. Makes it much easier to use. Diffs (updated) - daemon/org.kde.Solid.PowerManagement.xml 53f77e5 daemon/powerdevilbackendinterface.h 702b66b daemon/powerdevilbackendinterface.cpp 6dd8c71 daemon/powerdevilcore.h e255703 daemon/powerdevilcore.cpp d51ab19 Diff: https://git.reviewboard.kde.org/r/121915/diff/ Testing --- Tested with qdbus-monitor, signal is emitted when laptop lid is closed/opened. Thanks, Daniel Vrátil ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request 121915: Add lidClosedChanged signal to org.kde.Solid.PowerManagement
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121915/ --- Review request for Plasma. Repository: powerdevil Description --- There's no way to detect when lid has been closed other than listening to `changed` signal org.freedesktop.UPower and then polling the Powerdevil for new values. This patch adds a new signal to the PowerDevil interface to notify about the change and provide new value right away. Makes it much easier to use. Diffs - daemon/org.kde.Solid.PowerManagement.xml 53f77e5 daemon/powerdevilbackendinterface.h 702b66b daemon/powerdevilbackendinterface.cpp 6dd8c71 daemon/powerdevilcore.h e255703 daemon/powerdevilcore.cpp d51ab19 Diff: https://git.reviewboard.kde.org/r/121915/diff/ Testing --- Tested with qdbus-monitor, signal is emitted when laptop lid is closed/opened. Thanks, Daniel Vrátil ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
