search

mate-desktop / mate-notification-daemon

Daemon to display passive pop-up notifications
https://mate-desktop.org
GNU General Public License v2.0
30 stars 26 forks source link

Fix: escape text #106 #107

Closed kopr12 closed 8 years ago

monsta commented 8 years ago

Thanks, merged.

raveit65 commented 8 years ago

@kajzersoze @monsta I'm sorry to say but this breaks text in notifications. With this PR bildschirmfoto zu 2016-09-09_08-15-32 without it bildschirmfoto zu 2016-09-09_13-22-10

You can see that with normal github notifications from mate-dev too.

kopr12 commented 8 years ago

True, but it's better to have it like that than without any text at all. Now, it depends on application how that's going to work, some are dealing with that, some don't (they replace those characters in their code).

Without this PR you would see empty text for both summary and body when you execute this in terminal : $ notify-send "Test & Summary" "Test & Body"

with this PR it shows like it's supposed to be. Give it a try.

It's not ideal but I would say it's better than without it.

kopr12 commented 8 years ago

Or actually .. it would show correctly for summary but body would be empty, can't remember anymore, it was theme dependant, I think two themes out of 4 were showing summary correctly, but body was missing in all 4.

Edit : Yeah, just checked the commit, standard and slider themes already had g_markup_escape_text for summary, but not for the body.

raveit65 commented 8 years ago

I don't think, i will attach a screenshot from same kind of notification if i build next time for Fedora. And as you see in screenshot the different is. raveit65's

and 

raveit65's

means the apos is broken. With other notifications is see also probs with urls.

raveit65 commented 8 years ago

Btw. i never saw problems with body message here.

kopr12 commented 8 years ago

Like I said, some apps are dealing with that in their own code, you just probably didn't use those, first time I've noticed that was some radio app, some notifications were empty, I was wondering why and that's how I found this issue. btw if you try that notify-send example I posted above you would see empty text for body for sure. 100%

raveit65 commented 8 years ago

This is with xchat and it was working before :) If it works with notify-send is nice but in practice i use xchat.

raveit65 commented 8 years ago

Btw. with your example file gnotify i see the Body message.

kopr12 commented 8 years ago

Well, I don't know what to tell you, I just looked at gnome notification daemon and they do it like that as well , take a look : https://github.com/GNOME/notification-daemon/blob/5a2648549b4fb5a6816044d54f1d63b07ceceea8/src/nd-notification-box.c#L206-L230

btw you will see empty body if it has char & in it (without this PR)

I'll try to see what exactly is the problem and hopefully find a solution for it. I was busy to deal with that before, now I can play with it. But yes, you're right, I get it like that too, in Hexchat and Corebird (it was fine without this PR) , but some apps (clementine, gradio) and notify-send were showing empty text. It was getting on my nerves at first but somehow I got used to it, because it's better to see it like that than to see nothing ;)

kopr12 commented 8 years ago

Okay, I figured it out, will make a PR shortly.

kopr12 commented 8 years ago

@raveit65, I just made a PR which fixes this, please do try it, should be all good now.