D18739: Sync QML module padding to reflect system setting pages

[Valerio Pilo]

vpilo added a comment.


  In D18739#407649 , @broulik wrote:
  
  > If you open multiple KCMs switching between QML- and Widget-ones leads to 
awkward outside margin changes with this patch.
  >
  >   kcmshell5 icons style
  >
  >
  > Switch between the two and observe how the sidebar dances (this is likely 
the same issue as when doing the same in System Settings in Icons mode rather 
than Sidebar)
  
  
  I didn't realize you could open multiple KCMs. With that, it's much easier 
for me to see that there's multiple differences between QML- and Widget- based 
KCMs. Disabling window decoration borders, it's even more visible.
  
  | F6600583: qml.png  | F6600582: 
widget.png  |  |

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D18739

To: davidedmundson, #plasma
Cc: broulik, vpilo, kde-frameworks-devel, michaelh, ngraham, bruns

D18739: Sync QML module padding to reflect system setting pages

[Valerio Pilo]

vpilo added a comment.


  As mentioned in D18458 , a KCM not based 
on KCM.GridViewKCM & friends (e.g. a KCM based on Kirigami.Page) will now get 
extra padding (on Breeze, no scaling, it's of 6px), both on KCMShell and 
SystemSettings.

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D18739

To: davidedmundson, #plasma
Cc: vpilo, kde-frameworks-devel, michaelh, ngraham, bruns

D4799: Delay notifications until desktop session has loaded

[Valerio Pilo]

This revision was automatically updated to reflect the committed changes.
Closed by commit R289:2d40672b0c85: Do not remove queued notifications when the 
fd.o service starts. Also start the… (authored by vpilo).

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4799?vs=12377&id=12712

REVISION DETAIL
  https://phabricator.kde.org/D4799

AFFECTED FILES
  src/notifybypopup.cpp

To: vpilo, #plasma, #plasma_workspaces, davidedmundson, dfaure, broulik, 
graesslin, mck182
Cc: plasma-devel, davidedmundson, dfaure, broulik, graesslin, mck182, 
#frameworks, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol

D4799: Delay notifications until desktop session has loaded

[Valerio Pilo]

vpilo added a comment.


  Ping
  
  [my excuses if it's not good practice to do so]

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D4799

To: vpilo, #plasma, #plasma_workspaces, davidedmundson, dfaure, broulik, 
graesslin, mck182
Cc: plasma-devel, davidedmundson, dfaure, broulik, graesslin, mck182, 
#frameworks, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol

D4799: Delay notifications until desktop session has loaded

[Valerio Pilo]

vpilo added a comment.


  I pushed https://phabricator.kde.org/D5012; this rev is now testable.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D4799

To: vpilo, #plasma, #plasma_workspaces, davidedmundson, dfaure, broulik, 
graesslin, mck182
Cc: plasma-devel, davidedmundson, dfaure, broulik, graesslin, mck182, 
#frameworks, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol

D4799: Delay notifications until desktop session has loaded

[Valerio Pilo]

vpilo added reviewers: davidedmundson, dfaure, broulik, graesslin, mck182.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D4799

To: vpilo, #plasma, #plasma_workspaces, davidedmundson, dfaure, broulik, 
graesslin, mck182
Cc: plasma-devel, davidedmundson, dfaure, broulik, graesslin, mck182, 
#frameworks, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol

D4799: Delay notifications until desktop session has loaded

[Valerio Pilo]

vpilo updated this revision to Diff 12377.
vpilo marked an inline comment as done.
vpilo added reviewers: Plasma, Plasma: Workspaces.
vpilo added a comment.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.


  New patchset.
  I moved the waiting program to plasma-workspace, as per the suggestions 
(thanks). I also didn't like it much there, but didn't know where it would have 
fitted better.
  
  The other diff is https://phabricator.kde.org/D5012

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4799?vs=12310&id=12377

REVISION DETAIL
  https://phabricator.kde.org/D4799

AFFECTED FILES
  src/notifybypopup.cpp

To: vpilo, #plasma, #plasma_workspaces
Cc: plasma-devel, davidedmundson, dfaure, broulik, graesslin, mck182, 
#frameworks, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol

D4799: Delay notifications until desktop session has loaded

[Valerio Pilo]

vpilo added a dependent revision: D5012: Delay notifications until desktop 
session has loaded.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D4799

To: vpilo
Cc: davidedmundson, dfaure, broulik, graesslin, mck182, #frameworks

D4799: Delay notifications until desktop session has loaded

[Valerio Pilo]

vpilo updated this revision to Diff 12310.
vpilo added a comment.


  New version, using the idea suggested by @davidedmundson .
  
  The KNotifications manager was clearing queued notifications also when the 
fd.o Notifications service initially registers; and while I get that it's done 
on unregistering to cleanup, I can't see why it should do it on register as 
well. Without that clearing, notifications queue up and get delivered naturally 
when the service gets restarted.
  
  Also, I had to take out the KDE_FULL_SESSION check to make this work. Is 
there any drawback to this?

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4799?vs=11831&id=12310

REVISION DETAIL
  https://phabricator.kde.org/D4799

AFFECTED FILES
  src/CMakeLists.txt
  src/notifybypopup.cpp
  src/waitforname/CMakeLists.txt
  src/waitforname/main.cpp
  src/waitforname/org.freedesktop.Notifications.service.in

To: vpilo
Cc: davidedmundson, dfaure, broulik, graesslin, mck182, #frameworks

D4799: Delay notifications until desktop session has loaded

[Valerio Pilo]

vpilo added a comment.
View Revision
In D4799#91246, @davidedmundson wrote:
There is another solution that would work without any changes to KNotification.

DBus has a solution to buffer messages and wait for a name to become available, it happens in DBus activation. If plasmashell was DBus activated on org.freedesktop.Notifications we wouldn't have this problem at all.

Making plasmashell do that is probably a bit weird, but there is a hack we can do that I've seen done in Telepathy. Instead of activating plasmashell we have a small binary that gets DBus activated but simply idles waits for org.freedesktop.Notifications to become available then quits.

DBus-daemon thinks it's launched something and happily waits for the name to become available queuing the messages. Knotification can just fire and forget as normal.
 The only time there's any performance penalty is if you do send a notification pre-plasmashell starting up, and it's quite minor.

I've tested this using, https://paste.kde.org/pwdqvevvo
 running "notify-send foobar"
 then waiting 10 seconds and starting plasmashell

Notification appeared perfectly.

We'd need to re-implement mc-wait-for-name in Plasma code (or just use Exec=sleep 30)


This sounds like a very good alternative to me. I can implement it in KSplash.REPOSITORYR289 KNotificationsREVISION DETAILhttps://phabricator.kde.org/D4799EMAIL PREFERENCEShttps://phabricator.kde.org/settings/panel/emailpreferences/To: vpiloCc: davidedmundson, dfaure, broulik, graesslin, mck182, Frameworks

[Differential] [Closed] D4801: KNotifications cleanup

[Valerio Pilo]

This revision was automatically updated to reflect the committed changes.
Closed by commit R289:76ca9bc01c6f: Removed some dead code. (authored by vpilo).

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D4801?vs=11835&id=12031

REVISION DETAIL
  https://phabricator.kde.org/D4801

AFFECTED FILES
  src/knotificationmanager.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: vpilo, apol
Cc: apol, #frameworks

[Differential] [Commented On] D4799: Delay notifications until desktop session has loaded

[Valerio Pilo]

vpilo added a comment.


  In https://phabricator.kde.org/D4799#91109, @dfaure wrote:
  
  > Yes interface->isServiceRegistered(dbusServiceName) is technically 
blocking, but it can't block for a long time, since it's only talking to the 
dbus daemon. The reply comes in rather quickly, unlike a blocking call to 
another KDE process which could be busy or where the call itself could take a 
long time to be processed.
  >  In summary, I don't think an existing call to isServiceRegistered can be 
used as an argument for making (potentially much much longer) blocking calls.
  
  
  Not trying to :) I was just concerned about those pre-existing calls. If 
those are fine, I'll just go on with my plan.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D4799

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: vpilo
Cc: dfaure, broulik, graesslin, mck182, #frameworks

[Differential] [Commented On] D4799: Delay notifications until desktop session has loaded

[Valerio Pilo]

vpilo added a comment.


  @mck182 I didn't notice before, but KNotifications is already making blocking 
calls on creation:
  
src/notifybypopup.cpp@182:
QDBusConnectionInterface *interface = 
QDBusConnection::sessionBus().interface();
d->dbusServiceExists = interface && 
interface->isServiceRegistered(dbusServiceName);
  
  And there's more.
  I can make the KSplash notification relay; but I wouldn't know how to get rid 
of those..

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D4799

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: vpilo
Cc: dfaure, broulik, graesslin, mck182, #frameworks

[Differential] [Updated] D4801: KNotifications cleanup

[Valerio Pilo]

vpilo edited the summary of this revision.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D4801

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: vpilo
Cc: #frameworks

[Differential] [Commented On] D4799: Delay notifications until desktop session has loaded

[Valerio Pilo]

vpilo added a comment.


  In https://phabricator.kde.org/D4799#90169, @dfaure wrote:
  
  > About the code in kded that calls ksplash: that code is obsolete and 
currently only kept for compatibility reasons, see 
https://git.reviewboard.kde.org/r/129010/
  >  IOW you can ignore that code completely.
  
  
  OK, thanks :)
  
  In https://phabricator.kde.org/D4799#90165, @broulik wrote:
  
  > Most are not, however. PowerDevil, for instance, actually delays its 
notifications (e.g. you startup with a low battery) until a notification 
service registers itself to avoid the embarrassing popup ontop of KSplash while 
still showing the notification soon after logging in. Almost anything else 
("You are now connected to network X") is pointless, and I hate this Yakuake 
notification when logging in.
  
  
  All notifications can be disabled if the user finds them useless and/or 
annoying - not a reason to prevent them from showing at all by means of 
frameworks, though, IMHO. And we can't expect the applications to all do what 
PowerDevil does. Unless we make this change happen (with another trigger to 
decide whether the splash is being shown).

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D4799

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: vpilo
Cc: dfaure, broulik, graesslin, mck182, #frameworks

[Differential] [Commented On] D4799: Delay notifications until desktop session has loaded

[Valerio Pilo]

vpilo added a comment.


  @graesslin I disagree. Some popups might be useful, some are very useful; we 
should aim to never drop any. Just as an example, the popup with the Yakuake 
console toggle key is pretty fundamental to be reminded of, the first few times 
you start it up.
  
  It's all about saving users from annoyances. Papercuts, in Canonical terms.
  
  @mck182 - I think your approach of taking them in, and replaying them at the 
end, is the best option I can see from my inexperienced POV.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D4799

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: vpilo
Cc: graesslin, mck182, #frameworks

[Differential] [Commented On] D4799: Delay notifications until desktop session has loaded

[Valerio Pilo]

vpilo added a comment.


  In https://phabricator.kde.org/D4799#89931, @mck182 wrote:
  
  > Thanks for the patch! I wanted to do exactly this a long
  >  time ago. However this solution brings a burden to all
  >  apps using KNotification in form of a blocking dbus call
  >  which is further only relevant when used in Plasma.
  >
  > That's a no-no I'm afraid, we'd have to find a better solution.
  
  
  I understand. I'm out of the KDE development loop since a few years, so I 
wouldn't know what alternatives might there be that will be acceptable in the 
frameworks.
  Straight away I am thinking about:
  
  1. A quick check if KSplashQML is found in the processes list (afaics, 
there's no alternatives to ksplash)
  2. KSplash could listen for registration of a new dbus instance of 
KNotifications and emit a message to it (most probably too slow)
  3. KNotifications could send an async call to KSplash with a very quick 
timeout and start deciding on its queued notifications if/after the answer 
arrives
  4. Any other way to check whether the start-up process is still running 
without relying on KSplash?
  
  On a sidenote, I saw a couple comments about a 'new kde start system', but 
nothing more informative. Got any info?
  e.g. kded/src/kded.cpp@770, before calling on dbus KSplash.setStage():  
//NOTE: We are going to change how KDE starts and this certanly doesn't fit on 
the new design.
  
  > I was thinking that maybe KSplash should have the notifications
  >  dbus interface instead and handle it somehow on its own,
  >  either just collecting the notifications and then reemitting them
  >  once it's done splashing, or just displaying them directly, I dunno.
  
  That would add a lot of overhead & complexity to KSplash itself for a small 
case, I wouldn't like it.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D4799

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: vpilo
Cc: mck182, #frameworks

[Differential] [Request, 16 lines] D4801: KNotifications cleanup

[Valerio Pilo]

vpilo created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  Removed some dead code

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D4801

AFFECTED FILES
  src/knotificationmanager.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: vpilo
Cc: #frameworks

[Differential] [Request, 35 lines] D4799: Delay notifications until desktop session has loaded

[Valerio Pilo]

vpilo created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  The KNotifications library is immediately available for programs to send out 
notifications from the beginning of a desktop session. But if a startup program 
(or the frameworks themselves) sends out notifications before the desktop 
notifications service is ready, users will get old-school passive pop-ups, on 
top of the splash screen. In case other kinds of notifications were setup (e.g. 
sounds), those also gets in the way.
  
  With this change, all notifications will be blocked until KSplash goes away.
  
  NOTE: In case the splash screen is off, the change does nothing. You still 
get the passive popup. Anything that would make for a better "session is still 
loading" trigger than the D-Bus service of KSplash?

TEST PLAN
  Testing with application:
  
  - put into list of Startup/Shutdown in SystemSettings a program that shows a 
KNotification on start (e.g. Yakuake from distro's package, to test binary 
compatibility).
  - from the program's settings, set a valid sound and ensure a popup is shown 
on the event occurrence
  - logout and login again
  - the notification(s) should appear as regular Plasma notifications only when 
the splash is over, along with the sound
  
  Testing with system event:
  
  - On a laptop or a device with touchpad, set "Disable touchpad when mouse is 
plugged in" from Settings > Input Devices > Touchpad > Enable/Disable Touchpad 
tab. Ensure you login with a mouse plugged in
  - from rom Settings > Touchpad, set a valid sound and ensure a popup is shown 
on the event occurrence
  - logout and login again
  - the "touchpad disabled" notification should appear as regular Plasma 
notifications only when the splash is over, along with the sound

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D4799

AFFECTED FILES
  src/knotificationmanager.cpp
  src/knotificationmanager_p.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: vpilo
Cc: #frameworks