Re: Kup in KDE Review
On 2020-04-07 05:09, Adriaan de Groot wrote: On Monday, 6 April 2020 12:32:54 CEST Simon Persson wrote: Please help to review kup. - It's probably worthwhile looking at REUSE licensing compliance (see reuse.software, or ask on IRC #kde-devel) so that the license is machine- readable and checkable. I have changed license headers and such now. reuse lint now only complains about missing license on various small files, like gitignore and such. - Although you find_package(LibGit2) you were linking "old style" instead of using the imported target LibGit2::LibGit2. I pushed a build fix, now it builds on FreeBSD as well. Thanks! - Uses a handful of deprecated methods; depending on what exactly you want to be compatible with, you might chase those. Yes, I'm aware. I have reduced the number by quite a lot. The ones left are either complicated to port or too recent. I would have preferred waiting to change anything actually but all these deprecation warnings were causing a risk to drown out warnings about actual problems so I went and fixed them. Takes so much time to figure out what distros are shipping so I just went with some hunch on what versions are OK to depend on now. Thank you! Simon
Re: Kup in KDE Review
On 2020-04-08 16:53, Christophe Giboudeaux wrote: 'COPYING' only covers the GPL-2.0-only part of the code. You also need the GPL-3.0-only and the LicenseRef-KDE-Accepted-GPL reference. Note that we're slowly moving from the monolithic license blocks to SPDX statements. See https://community.kde.org/Policies/Licensing_Policy#License_Statements for details. Christophe Thanks for the link! I was looking for it after I read Adrians email but never found that page, even though I knew it existed. I have pushed changes today, think it's all settled now. Simon
Re: Kup in KDE Review
Hello! On 2020-04-07 06:01, Nicolas Fella wrote: Hi, I briefly skimmed trough the codebase. Looks all sane to me. A few minor observations: - You may want to look into KConfigXT. It should be able to generate the classes from settings/ from an XML description. I think that I looked at it when I started Kup, about 10 years ago... don't remember exactly but I think the issue was about dynamic configuration entries. Kup allows user to create as many backup plans as they want and each will have some settings. - You are using KInit for the daemon executable. We are looking into killing that in the KF6 transition (https://phabricator.kde.org/T12140 ) so consider it deprecated (although it is not offically yet). If you do not care about using kup outside of Plasma you might also consider implementing the daemon part as a kded module. Ok, thanks for the info. And yes, moving it to kded might be possible. Again, I think I looked at doing that in the beginning but don't remember the reason that I went for a standalone executable instead. - In https://invent.kde.org/kde/kup/-/blob/master/filedigger/mergedvfsmodel.cpp#L60 please check if instead of calling loadMimeTypeIcon simply returning the iconname is enough, or return QIcon::fromTheme(name). loadMimeTypeIcon looks like a likely candidate for deprecation/removal to me. That would also get rid of the KIconThemes dependency. Excellent finding! I have pushed a commit now to remove this dependency. Simon
Re: Kup in KDE Review
On 2020-04-07 21:28, Jonathan Riddell wrote: This looks great. I think the other comments have covered the main issues so I'll just make a cheeky feature request and suggest it gets the ability to upload to cloud storage since I would guess that's the main way to do backups these days. Talking to Nextcloud or gdrive or AWS directly would make it much more useful to have. Jonathan Thanks for the kind words. Yep, there are probably some people who want to save backups to some server. It is possible with kup since the beginning, by using a fuse mount. Which you need to figure out yourself. Then just point kup to the filesystem path and kup will from then on monitor mounts and unmounts to see if that path is available. So just make sure to mount the storage at the same point every time. But... in order to do something a bit smoother for the user to configure I have not done much. I have looked at SMB but never figured out a way that I could let the user browse for network shares and configure fuse mounts easily. So nothing ever came out of that effort. Then I imagined other protocols will be similar so never looked at anything else. If somebody has an idea how the current situation can be improved I would be interested to hear, even though working on it would be very low priority for me. For myself I have never used anything cloudish. Except email and github, I guess... :) Simon
Re: Kup in KDE Review
On 2020-04-07 06:02, Albert Astals Cid wrote: When in the kcm i go to add new plan i get the "Versioned backup" disabled because bup is not installed *BUT* it is still the selected radio button, i guess in that case it would make more sense if the "Synchronized backup" was the selected one, no? kup-filedigger stars off really small here (less than 300x300). Not critical but it seems like it would benefit for having some minimum/recommended size set. Cheers, Albert Good findings! I pushed fixes for both just now. Thank you! (I didn't see the small default size because of my kwin rule to maximize all windows upon creation, such things that make me love plasma ) Simon
Kup in KDE Review
Hello! Please help to review kup. It is a backup scheduler tightly integrated with plasma (has system setting kcm, systray plasmoid, kioslave). It supports saving backups either with bup or with rsync. It has been developed outside of KDE for many years and only now is being incubated. I am unsure if it should end up in extragear or some release service bundle. Perhaps people have an opinion on this? https://invent.kde.org/kde/kup Thanks, Simon
Re: Review Request 110662: Add dbus signal to ksmserver, used for requesting session saving from services.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110662/ --- (Updated Oct. 29, 2013, 5:53 p.m.) Review request for kde-workspace, Aaron J. Seigo, Ivan Čukić, and Thomas Lübking. Repository: kde-workspace Description --- Support session saving in the activity manager service, which can be started before ksmserver and therefore it could potentially not have contact with ksmserver over ICE. Do this by emitting a dbus signal when the current session should be saved. Only support for save state or local, which is used for saving current window positions and such, not the commit state or global which is used to save files the user has opened. A service like the activity manager should only need the former since it is not an application. Separate review for adding support in kamd. Diffs - ksmserver/org.kde.KSMServerInterface.xml 9dad130 ksmserver/server.h 3b993c5 ksmserver/shutdown.cpp d1db3ff Diff: http://git.reviewboard.kde.org/r/110662/diff/ Testing --- Tested by running a full kde session compiled from master, logging of the signal with qbusviewer. With restore previous session set: signal emitted at all logouts. save session button is not available. With restore manually saved session set: signal emitted on manual save session activation but not on log out. Thanks, Simon Persson
Re: Review Request 110663: Don't save state of running activities unless asked to by the session manager
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110663/ --- (Updated May 29, 2013, 10:02 a.m.) Review request for kde-workspace and Ivan Čukić. Changes --- Check the ksmserver config file for which session should be restored. Manually saved session gets saved in separate config file entries. I was also trying to make kamd crash-safe when ksmserver is set to restore manually saved session, I tried adding a cleanShutdown entry to the config file, it would get set to false after restoring a session and to true in the Activities destructor. I did some changes to main() so that the destructors would get called. Unfortunately it was still not enough as it seems kamd is not asked nicely to quit on logout, but rather killed. Decided to keep the changes so that destructors are called at least in the case of a nice shutdown. Description --- Only save state of currently running session when asked to by the session manager. This will support activities in the restore manually saved session function of the session manager. Relies on review https://git.reviewboard.kde.org/r/110662/ Diffs (updated) - src/service/Activities.cpp 9e09c9f src/service/Activities_p.h 5f3304d src/service/Application.h 2e84b27 src/service/Application.cpp 54cbf8a src/service/jobs/ksmserver/KSMServer.h 7e42e56 src/service/jobs/ksmserver/KSMServer.cpp 888df1f Diff: http://git.reviewboard.kde.org/r/110663/diff/ Testing --- Tested by running a full kde session compiled from master. Saw that with restore previous session set there was no regression (only change would be that a power failure or crash of ksmserver would now result in the current state of running activities not getting saved, hardly critical I would say). Also saw that restore manually saved session now restores activities as they were when the session was saved. Thanks, Simon Persson
Review Request 110662: Add dbus signal to ksmserver, used for requesting session saving from services.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110662/ --- Review request for kde-workspace, Aaron J. Seigo, Kevin Ottens, Ivan Čukić, and Thomas Lübking. Description --- Support session saving in the activity manager service, which can be started before ksmserver and therefore it could potentially not have contact with ksmserver over ICE. Do this by emitting a dbus signal when the current session should be saved. Only support for save state or local, which is used for saving current window positions and such, not the commit state or global which is used to save files the user has opened. A service like the activity manager should only need the former since it is not an application. Separate review for adding support in kamd. Diffs - ksmserver/org.kde.KSMServerInterface.xml 9dad130 ksmserver/server.h 3b993c5 ksmserver/shutdown.cpp d1db3ff Diff: http://git.reviewboard.kde.org/r/110662/diff/ Testing --- Tested by running a full kde session compiled from master, logging of the signal with qbusviewer. With restore previous session set: signal emitted at all logouts. save session button is not available. With restore manually saved session set: signal emitted on manual save session activation but not on log out. Thanks, Simon Persson
Review Request 110663: Don't save state of running activities unless asked to by the session manager
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110663/ --- Review request for kde-workspace and Ivan Čukić. Description --- Only save state of currently running session when asked to by the session manager. This will support activities in the restore manually saved session function of the session manager. Relies on review https://git.reviewboard.kde.org/r/110662/ Diffs - src/service/Activities.cpp 9e09c9f src/service/Activities_p.h 5f3304d src/service/jobs/ksmserver/KSMServer.h 7e42e56 src/service/jobs/ksmserver/KSMServer.cpp 888df1f Diff: http://git.reviewboard.kde.org/r/110663/diff/ Testing --- Tested by running a full kde session compiled from master. Saw that with restore previous session set there was no regression (only change would be that a power failure or crash of ksmserver would now result in the current state of running activities not getting saved, hardly critical I would say). Also saw that restore manually saved session now restores activities as they were when the session was saved. Thanks, Simon Persson
Re: Review Request: Fix global shortcuts that needs shift key, like for example ctrl+% (ctrl+shift+5 on many keyboards)
On June 7, 2011, 4:28 p.m., Michael Jansen wrote: Why don't you fix KKeyServer to return the correct results instead of fixing the wrong ones here? Yes, my first thought was to fix this by changing KKeyServer::keyQtToModX to add Shift to the modifiers if it is needed to get the symbol. This could be done using the (currently unused) function getModsRequired(uint sym) in kkeyserver_x11.cpp. But this is public API and I don't know what other users there are and what the expected behavior is, changing it to do add modifiers that were not there based on the behavior of some widget for entering shortcuts could be considered unexpected:) Same goes for KKeyServer::xEventToQt(), that would need to be changed to strip Shift modifier in the cases where KKeySequenceWidget has removed it. The positive thing about doing it this way would be that isShiftAsModifierAllowed() would not need to be exported, it could be in a private header as Aaron suggested in the other review request. - Simon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101520/#review3751 --- On June 6, 2011, 11:02 a.m., Simon Persson wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101520/ --- (Updated June 6, 2011, 11:02 a.m.) Review request for KDE Runtime and Michael Jansen. Summary --- Second patch to fix this bug, depends on patch in review request 101515. KKeySequenceWidget used to enter shortcuts removes shift from the recorded shortcut if the symbol produced from that physical key is different when shift is used (upper/lowercase letters doesn't count). kglobalaccel needs to include shift in the grab in order to be triggered on this class of shortcuts, and then in the keypress event handler it also needs to strip the shift again before checking which shortcut was just triggered. This addresses bugs 179504, 197548 and 215030. http://bugs.kde.org/show_bug.cgi?id=179504 http://bugs.kde.org/show_bug.cgi?id=197548 http://bugs.kde.org/show_bug.cgi?id=215030 Diffs - kglobalaccel/kglobalaccel_x11.cpp 2576d2e Diff: http://git.reviewboard.kde.org/r/101520/diff Testing --- Tested using the master branch, running in a Xephyr session using gb,us and se keyboard layouts. The tested layout needs to be the first entry in the list of layouts in the keyboard layout switcher, otherwise it will not work. Works for all the !£$%^*()_+ etc, no regression on shortcuts with only a number, only a letter or shift+letter. Also no regression on alt+shift+tab or ctrl+shift+esc. Thanks, Simon
Review Request: Fix global shortcuts that needs shift key, like for example ctrl+% (ctrl+shift+5 on many keyboards)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101520/ --- Review request for KDE Runtime and Michael Jansen. Summary --- Second patch to fix this bug, depends on patch in review request 101515. KKeySequenceWidget used to enter shortcuts removes shift from the recorded shortcut if the symbol produced from that physical key is different when shift is used (upper/lowercase letters doesn't count). kglobalaccel needs to include shift in the grab in order to be triggered on this class of shortcuts, and then in the keypress event handler it also needs to strip the shift again before checking which shortcut was just triggered. This addresses bugs 179504, 197548 and 215030. http://bugs.kde.org/show_bug.cgi?id=179504 http://bugs.kde.org/show_bug.cgi?id=197548 http://bugs.kde.org/show_bug.cgi?id=215030 Diffs - kglobalaccel/kglobalaccel_x11.cpp 2576d2e Diff: http://git.reviewboard.kde.org/r/101520/diff Testing --- Tested using the master branch, running in a Xephyr session using gb,us and se keyboard layouts. The tested layout needs to be the first entry in the list of layouts in the keyboard layout switcher, otherwise it will not work. Works for all the !£$%^*()_+ etc, no regression on shortcuts with only a number, only a letter or shift+letter. Also no regression on alt+shift+tab or ctrl+shift+esc. Thanks, Simon
