Skip to content

Add ignore_dbusclose settings#732

Merged
tsipinakis merged 5 commits intodunst-project:masterfrom
matclab:731-ignore-dbus-close-setting
Jul 17, 2020
Merged

Add ignore_dbusclose settings#732
tsipinakis merged 5 commits intodunst-project:masterfrom
matclab:731-ignore-dbus-close-setting

Conversation

@matclab
Copy link

@matclab matclab commented Jul 10, 2020

closes #731

However I was not able to test it because of X BadAtom error:

./dunst -print -verbosity debug
{
	appname: 'dunst'
	summary: 'startup'
	body: 'dunst is up and running'
	icon: 'dialog-information'
	raw_icon set: false
	icon_id: 'dialog-information'
	desktop_entry: ''
	category: 
	timeout: 10000
	urgency: LOW
	transient: 0
	formatted: '<b>startup</b>
dunst is up and running'
	fg: #888888
	bg: #222222
	frame: #aaaaaa
	fullscreen: pushback
	progress: -1
	stack_tag: 
	id: 2
	actions: {}
	script: (null)
}
DEBUG: RUN
DEBUG: XEvent: Ignoring '65'
DEBUG: XEvent: processing 'CreateNotify'
DEBUG: XEvent: Checking for active screen changes
DEBUG: XEvent: Checking for active screen changes
DEBUG: XEvent: Checking for active screen changes
DEBUG: XEvent: Checking for active screen changes
DEBUG: XEvent: Ignoring '65'
DEBUG: XEvent: Checking for active screen changes
DEBUG: XEvent: Checking for active screen changes
DEBUG: XEvent: Ignoring '19'
DEBUG: XEvent: Ignoring '19'
DEBUG: XEvent: Ignoring '15'
DEBUG: XEvent: processing 'Expose'
X Error of failed request:  BadAtom (invalid Atom parameter)
  Major opcode of failed request:  23 (X_GetSelectionOwner)
  Atom id in failed request:  0x0
  Serial number of failed request:  218
  Current serial number in output stream:  218

@tsipinakis
Copy link
Member

Some points: The notification spec has no concept of disallowing closing notifications, so to make this 'compliant' I'd choose to lie to the clients that the notification is actually closed, but keep it open.
To do this, we need to:

  • Send a NotificationClosed signal back (see signal_notification_closed)
  • Invalidate all client-based actions as they most likely won't work anymore (see notification_invalidate_action)

Additionally please update the example dunstrc and the man page with some documentation about the new option.

@tsipinakis
Copy link
Member

Oh, I almost forgot the elephant in the room:

However I was not able to test it because of X BadAtom error:

Can you please expand more into this? Does it happen on master without this PR? If so, that's an important bug, please file an issue about it!

@matclab
Copy link
Author

matclab commented Jul 12, 2020

Oh, I almost forgot the elephant in the room:

However I was not able to test it because of X BadAtom error:

Can you please expand more into this? Does it happen on master without this PR? If so, that's an important bug, please file an issue about it!

I just tried with master and cannot reproduce the problem, and switching back on this branch I cannot reproduce it anymore. And then I remember that I had an external screen connected when the problem occurred. Checking it again with an external screen and the problem happens also with master. I'll open a bug report.

@matclab
Copy link
Author

matclab commented Jul 12, 2020

Note that I didn't call notification_invalidate_action which is already call by signal_notification_closed.

@matclab matclab force-pushed the 731-ignore-dbus-close-setting branch from 3c85de8 to 0761240 Compare July 12, 2020 12:17
@matclab
Copy link
Author

matclab commented Jul 12, 2020

The CI fails on codecov timeout. Nothing I can do on my side.

@matclab matclab force-pushed the 731-ignore-dbus-close-setting branch from 0761240 to 19a42e3 Compare July 12, 2020 20:44
@tsipinakis
Copy link
Member

The changes look good. One thing left is that when a notification is closed, a warning is printed WARNING: Closing notification 'Test' not supported. Notification already closed. from signal_notification_closed. I guess the easiest way to solve this is to remove that line entirely.

As for the docs, Useful to enforce _the_ timeout set by dunst configuration, just missing a the but otherwise it looks good to me.

@matclab matclab force-pushed the 731-ignore-dbus-close-setting branch from 19a42e3 to de33228 Compare July 14, 2020 15:55
@matclab
Copy link
Author

matclab commented Jul 14, 2020

commits updated to edit the english according to review and remove the warning.

Copy link
Member

@tsipinakis tsipinakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/queues.c Outdated


/* see queues.h */
struct notification* queues_get_received_by_id(int id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does received mean in this context?
I'd say queues_get_by_id would be a shorter and more descriptive name.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm I was trying to say that id was not looked for in all the queues…
queues_get_by_id would be a bit misleading as history queues is not used.

The best would probably to name the function queues_get_by_id and search in all queues. In our use case it won't change the looking time as we expect the id to be in one of the first two queues.
Would this be ok ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine by me. Default history max is ~20 items so performance impact is negligible.

matclab added 4 commits July 16, 2020 18:28
…ance

The notification spec has no concept of disallowing closing notifications,
so to make the ignore_dbusclose 'compliant' we choose to lie to the clients
that the notification is actually closed, but keep it open.
To do this, we send a NotificationClosed signal back (via signal_notification_closed
which also call notification_invalidate_action)
This message has no real added value and is emitted every time a
notification is closed when `ignore_dbuclose` is set.
@matclab matclab force-pushed the 731-ignore-dbus-close-setting branch from bf90495 to 842a524 Compare July 16, 2020 16:35
@tsipinakis tsipinakis merged commit eb253a4 into dunst-project:master Jul 17, 2020
@tsipinakis
Copy link
Member

Thanks for the PR! And sorry for the delay in merging this.

@matclab
Copy link
Author

matclab commented Jul 18, 2020

No problem. Thanks for your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a configuration to ignore dbus closeNotification event

2 participants