GNOME Bugzilla – Bug 769485
Eliminate use of g_test_expect_message()
Last modified: 2016-08-06 21:56:03 UTC
As discussed with mclasen on IRC, it would be good if we could deprecate g_test_expect_message() when G_LOG_USE_STRUCTURED is enabled, since unit tests can now set a custom log writer function to check expected log messages if they want. In these two cases (the only two calls to g_test_expect_message() in GTK+) the fixes are simpler than adding a custom writer function. Once/If these patches are pushed, commit df02e8f47cac4d2c550491c9de633233218c7415 in GLib can be reverted, and the documentation for g_test_expect_message() updated to mention it’s deprecated with structured logging.
Created attachment 332678 [details] [review] gtktreestore: Remove unnecessary g_warning() Calling gtk_tree_store_reorder() on a parent which has no children should be a no-op, and doesn’t need to emit a warning about it.
Created attachment 332679 [details] [review] gtkicontheme: Don’t emit warning about fallback theme when in unit tests In order to eliminate g_test_expect_message() (which doesn’t work with G_LOG_USE_STRUCTURED), make the warning about the fallback theme not existing be conditional on the icon theme search path containing a system path. Any application code which modifies the search path does so through appends and prepends, so this should not affect whether the warning is emitted in production.
Review of attachment 332678 [details] [review]: ::: gtk/gtktreestore.c @@ +2290,1 @@ return; I don't think turning this into a debug message is right - either this is a programming error, in which case the warning is right. Or we tacitly ignore your attempt to sort nothing.
Review of attachment 332679 [details] [review]: ::: gtk/gtkicontheme.c @@ +1883,3 @@ + system_paths_in_search_path = + g_str_has_prefix (priv->search_path[i], xdg_data_dirs[j]); + } Would be nicer to only do this extra work in the !found case.
Review of attachment 332679 [details] [review]: The idea seems good
Review of attachment 332678 [details] [review]: Thinking some more about this; I think the warning should stay; we could just push a trivial null log writer around the call to ignore the message.
Created attachment 332705 [details] [review] gtktreestore: Remove unnecessary g_warning() Calling gtk_tree_store_reorder() on a parent which has no children should be a no-op, and doesn’t need to emit a warning about it.
Created attachment 332706 [details] [review] gtkicontheme: Don’t emit warning about fallback theme when in unit tests In order to eliminate g_test_expect_message() (which doesn’t work with G_LOG_USE_STRUCTURED), make the warning about the fallback theme not existing be conditional on the icon theme search path containing a system path. Any application code which modifies the search path does so through appends and prepends, so this should not affect whether the warning is emitted in production.
Review of attachment 332705 [details] [review]: I'm a bit dubious about this. You can't call reorder without having first constructed a new order, so you need to know fairly well about the existing children. So if you hit an empty tree all of a sudden, something has gone seriously wrong. I do think that this deserves a warning.
Review of attachment 332706 [details] [review]: arr, maybe you misinterpreted my followup comment from yesterday ? I meant that the approach you took for the icon theme was ok, while the treeview warning should be silenced with a null log writer...
Attachment 332706 [details] pushed as 76721e7 - gtkicontheme: Don’t emit warning about fallback theme when in unit tests
For reference, Mattias pushed commit 76721e736e9605b3a7ec76d3676d8cb5f4172490 as an alternative to attachment #332705 [details]. Thanks for fixing up my mess; I guess I misread which comment applied to which patch. :-(