After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 650298 - messageTray: improve bad-markup handling
messageTray: improve bad-markup handling
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: message-tray
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks: 649385
 
 
Reported: 2011-05-16 10:38 UTC by Dan Winship
Modified: 2011-07-27 13:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
messageTray: improve bad-markup handling (1.83 KB, patch)
2011-05-16 10:38 UTC, Dan Winship
committed Details | Review
tests: fix up typelib include paths (2.80 KB, patch)
2011-05-19 12:34 UTC, Dan Winship
needs-work Details | Review
tests: add a test for MessageTray._fixMarkup (6.17 KB, patch)
2011-05-19 12:34 UTC, Dan Winship
accepted-commit_now Details | Review
tests: fix up typelib include paths (2.84 KB, patch)
2011-07-26 13:38 UTC, Dan Winship
committed Details | Review
tests: add a test for MessageTray._fixMarkup (6.17 KB, patch)
2011-07-26 13:39 UTC, Dan Winship
committed Details | Review

Description Dan Winship 2011-05-16 10:38:43 UTC
split out from bug 649385 since this is a good fix to have in even if
the other stuff there isn't ready/isn't wanted.
Comment 1 Dan Winship 2011-05-16 10:38:45 UTC
Created attachment 187897 [details] [review]
messageTray: improve bad-markup handling

_fixMarkup() was supposed to be ensuring that the markup we passed to
clutter was correct, but it was validating the syntax incorrectly, and
wasn't checking that the markup was valid (or even well-formed). This
is bad because if you pass bad pango markup to
clutter_text_set_markup(), it will g_warn and drop the string on the
floor.

Fix by fixing up the regexps, and then calling Pango.parse_markup() on
the result, and just xml-escaping everything if parse_markup() fails.
Comment 2 Colin Walters 2011-05-16 19:23:54 UTC
Review of attachment 187897 [details] [review]:

Urgh.  Can we get some example test cases for this?  We don't really have a good way to run js unit tests right now I guess, but a few commented examples of what _fixMarkup is supposed to take as input and return as output would be *really* helpful for review and future bugfixes.
Comment 3 Dan Winship 2011-05-19 12:34:29 UTC
Created attachment 188108 [details] [review]
tests: fix up typelib include paths

The js modules have so many imports back and forth that it's pretty
much guaranteed that if you import even one of them, you'll end up
importing all of them, including ui.status.bluetooth and
ui.status.network. So fix up the typelib include paths the same way
gnome-shell-jhbuild does, so we can find everything.
Comment 4 Dan Winship 2011-05-19 12:34:32 UTC
Created attachment 188109 [details] [review]
tests: add a test for MessageTray._fixMarkup
Comment 5 Maxim Ermilov 2011-07-25 22:22:25 UTC
Review of attachment 188108 [details] [review]:

::: tests/run-test.sh.in
@@ -31,3 @@
 srcdir=`cd $srcdir && pwd`
 
-GI_TYPELIB_PATH="@MUTTER_TYPELIB_DIR@:$builddir/../src"

UPowerGlib was removed from gnome-shell.modules in favour to system one.
/usr/{lib64,lib}/girepository-1.0 should be in GI_TYPELIB_PATH.
Comment 6 Maxim Ermilov 2011-07-25 22:28:06 UTC
Review of attachment 188109 [details] [review]:

looks good

::: tests/unit/markup.js
@@ +86,3 @@
+
+// stray '&'; this is really a bug, but it's easier to do it this way
+assertConverts('<b>this</b> & <i>that</i>', '<b>this</b> &amp; <i>that</i>'); 

trailing whitespace
Comment 7 Dan Winship 2011-07-26 13:38:59 UTC
Created attachment 192667 [details] [review]
tests: fix up typelib include paths

The UPowerGLib change shouldn't affect anything; g-i looks in the
system dirs anyway. But the code was failing to use the existing value
of GI_TYPE_LIB_PATH, so fix that.
Comment 8 Dan Winship 2011-07-26 13:39:15 UTC
Created attachment 192668 [details] [review]
tests: add a test for MessageTray._fixMarkup

fix trailing whitespace
Comment 9 Dan Winship 2011-07-27 13:29:56 UTC
Attachment 187897 [details] pushed as 7542e68 - messageTray: improve bad-markup handling
Attachment 192667 [details] pushed as 0907f03 - tests: fix up typelib include paths
Attachment 192668 [details] pushed as f03793b - tests: add a test for MessageTray._fixMarkup