GNOME Bugzilla – Bug 763796
Excessive runtime warnings from gtk_style_context_get_...()
Last modified: 2018-05-02 16:58:54 UTC
Created attachment 324157 [details] [review] Proposed patch evolution-3.19.92-1.fc25.x86_64 My journal on rawhide is full (thousands for a day) with .... Mar 17 11:15:24 d2 evolution.desktop[9415]: (evolution:9415): Gtk-WARNING **: State 0 for EMailShellSidebar 0x55d04ef9ec50 doesn't match state 192 set via gtk_style_context_set_state () .... The attached patch maybe ? There seem to be a number of other places where gtk_style_context_get_padding is called with flags 0....
Thanks for a bug report. This is wrong on the gtk+ side. The issues (of gtk+) are, from my point of view: a) such warnings are useless on end-user machines, they are supposed to be shown for developers only; gtk+ should simply stop filling end-user logs with nonsense (and breaking their SSD-s) b) such issue doesn't qualify for a crash (fatal-warnings), but I do not know exact rationale why it was added there c) the API of gtk_style_context_get_padding() is broken by design due to this warning, because if the caller is not meant to change the state flags, then the state flags are not supposed to be there at the first place; d) if the API is right (and the caller can override the state flags), then the warning is misleading e) if I recall correctly, I was told that the right way to avoid this runtime warning is to do a round trip similar to: - save context state - change context state flags - call gtk_style_context_get_padding() with desired flags - restore context state Why would each single application do this roundtrip, when it can be done transparently by gtk+ inside the call of gtk_style_context_get_padding(), instead of filling user logs with nonsense? f) the change suggested in the above patch supports the c) point I always thought that the libraries are here to help, to avoid code duplication, but this particular decision makes me think of the opposite in the case of the "latest" gtk+.
So, you are not going to apply the patch that makes evolution do the right thing?!
(In reply to Matthias Clasen from comment #2) > So, you are not going to apply the patch that makes evolution do the right > thing?! Definitely not right now (the hard code freeze) and even I do workarounds myself, I'd prefer to have this properly fixed, on the gtk+ side, rather than have each single application which uses this API write their own workarounds. If I can avoid useless code, I prefer to avoid it.
Applications use our apis in wild and wonderful ways, and sometimes we can't preserve all the ways in which they are used. Sorry if that inconveniences you.
This probably sounds stupid and hacky but couldn't there be a GTK_STATE_CURRENT = sizeof(GtkStateFlags) all 1's, to be used in the various gtk_style_context_get_X apis for this
there is two possible ways to 'correct' a call like gtk_style_context_get_foo (context, some_state, &foo); either, what is wanted is gtk_style_context_get_foo (context, gtk_style_context_get_state (context), &foo); or, it could also be gtk_style_context_save (context); gtk_style_context_set_state (context, some_state); gtk_style_context_get_foo (context, gtk_style_context_get_state (context), &foo); gtk_style_context_restore (context); We don't know which one is the correct fixup, so doing it for you is not really possible. > This probably sounds stupid and hacky but couldn't there be a > GTK_STATE_CURRENT = sizeof(GtkStateFlags) all 1's, to be used in the various > gtk_style_context_get_X apis for this That would be a bit too hacky and limiting for future development of the GtkStateFlags enum, I think. We have talked about doing a magic interpretation for 0 ( == GTK_STATE_FLAG_NORMAL), since that is not a state that any widget ever has, since GTK+ 3.10 introduced states for text direction.
Created attachment 324226 [details] [review] style context: Don't use g_warning for API misuse We've changed our API here; what these applications are doing used to be fine. Don't make users suffer for this by spamming their logs in a stable release. We'll keep the warning in master.
Here is what I plan to put in 3.20
I do not understand why you call it an API misuse. Look at: https://developer.gnome.org/gtk3/stable/GtkStyleContext.html#gtk-style-context-get-padding > state ... state to retrieve the padding for How can I misuse it? That's a valid request to ask for the value of the context for a state which is not current, for example just to pre-cache it. Of course, if I use some incorrect value, then it's a different story and the issue is on my side (even, if they are flags, then all can be sometimes set and sometimes not, right?). (In reply to Matthias Clasen from comment #7) > We'll keep the warning in master. Thus it'll be a regression for the next stable? What about incorporating these sort of things under one umbrella, like G_ENABLE_DIAGNOSTIC? I'm not sure whether it makes sense for you, but for me the signal of the current API is that I am allowed to ask for style values for any style combination, not only for the one that is current for the style context. Not being it true, the 'state' parameter wouldn't be there. The runtime warning says the opposite, which is (kindly said) confusing. Even your example of the "solution" from comment #6 shows that the 'state' parameter is useless. The right thing, from my point of view, would be to change those gtk_style_context_get_foo() functions in a way: restore_state = FALSE; if (state != gtk_style_context_get_state (context)) { restore_state = TRUE; gtk_style_context_save (context); gtk_style_context_set_state (context, some_state); } ... if (restore_state) { gtk_style_context_restore (context); } Or break the API and remove the useless 'state' argument, thus it'll be clear what the code is supposed to look like. Changing the devel-doc to > state ... always use gtk_style_context_get_state (context) just highlights the uselessness of the argument in the API.
There is no need to insist on the point that the state argument is useless in the API now. It used to make sense, but now it doesn't.
Comment on attachment 324226 [details] [review] style context: Don't use g_warning for API misuse Attachment 324226 [details] pushed as 5c63ab2 - style context: Don't use g_warning for API misuse
(In reply to Matthias Clasen from comment #10) > There is no need to insist on the point that the state argument is useless > in the API now. It used to make sense, but now it doesn't. True. It seemed to me that we are not on the same page, thus I repeated myself. The devel-doc is also inaccurate, though I'd prefer if you could keep the argument usable, just for cases where people could prefer/like to use it. The change suggested in comment #9 doesn't seem that hard. Maybe?
https://git.gnome.org/browse/gtk+/commit/?id=a9814fea7d169400b033bc28f3b7570b3423bc15 says "this warning has become rather important for detecting bugs." Previously, it was only "performance issues". https://git.gnome.org/browse/gtk+/commit/?id=eba317228fd36c67513e2a46f5d1b57328db7033 Is this just about occasional unnecessary invalidations, or worse? I guess if this is happens during a draw signal on the widget, then there is a continual invalidation/draw stream. I don't know whether that alone would warrant making this a warning for future versions or not. There are probably other ways to do that, but perhaps modifying a widget's style context is less obvious. I'm likely missing some understanding.
I extended Yanko's patch from comment #0 for the evolution and committed it to sources. I still consider the API of those gtk_style_context_get_... (style_context, state, ....) broken and a good candidate to be fixed on the gtk+ side soon. Created commit_01f5ae2 in evo master (3.21.1+) [1] Created commit_58919ef in evo gnome-3-20 (3.20.1+) [1] https://git.gnome.org/browse/evolution/commit/?id=01f5ae2
This fix is causing a visual glitch. I thought I would post here but I can open a new bug as well if needed. http://i.imgur.com/Bs0YLf4.png
(In reply to Hussam Al-Tayeb from comment #15) > This fix is causing a visual glitch. > I thought I would post here but I can open a new bug as well if needed. > > http://i.imgur.com/Bs0YLf4.png this is unrelated. there was no 'fix' here anyway. I just took out the warning
oh, you mean the evolution change... I think we should make up our mind if we're talking about gtk or evolution in here.
(In reply to Hussam Al-Tayeb from comment #15) > This fix is causing a visual glitch. That's broken with due to newer gtk (3.20.0). It looks properly with, for example, gtk 3.18.6.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/601.