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 763796 - Excessive runtime warnings from gtk_style_context_get_...()
Excessive runtime warnings from gtk_style_context_get_...()
Status: RESOLVED OBSOLETE
Product: gtk+
Classification: Platform
Component: .General
3.19.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2016-03-17 09:34 UTC by Yanko Kaneti
Modified: 2018-05-02 16:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch (885 bytes, patch)
2016-03-17 09:34 UTC, Yanko Kaneti
committed Details | Review
style context: Don't use g_warning for API misuse (1.73 KB, patch)
2016-03-18 01:55 UTC, Matthias Clasen
committed Details | Review

Description Yanko Kaneti 2016-03-17 09:34:26 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....
Comment 1 Milan Crha 2016-03-17 10:07:26 UTC
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+.
Comment 2 Matthias Clasen 2016-03-17 13:57:02 UTC
So, you are not going to apply the patch that makes evolution do the right thing?!
Comment 3 Milan Crha 2016-03-17 17:20:37 UTC
(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.
Comment 4 Matthias Clasen 2016-03-17 19:54:51 UTC
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.
Comment 5 Yanko Kaneti 2016-03-17 21:07:37 UTC
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
Comment 6 Matthias Clasen 2016-03-18 00:11:01 UTC
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.
Comment 7 Matthias Clasen 2016-03-18 01:55:00 UTC
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.
Comment 8 Matthias Clasen 2016-03-18 01:57:45 UTC
Here is what I plan to put in 3.20
Comment 9 Milan Crha 2016-03-18 07:45:24 UTC
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.
Comment 10 Matthias Clasen 2016-03-18 13:04:16 UTC
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 11 Matthias Clasen 2016-03-21 00:57:07 UTC
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
Comment 12 Milan Crha 2016-03-21 07:56:27 UTC
(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?
Comment 13 Karl Tomlinson 2016-03-23 09:18:24 UTC
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.
Comment 14 Milan Crha 2016-03-23 10:06:24 UTC
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
Comment 15 Hussam Al-Tayeb 2016-03-23 11:49:24 UTC
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
Comment 16 Matthias Clasen 2016-03-23 12:40:25 UTC
(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
Comment 17 Matthias Clasen 2016-03-23 12:42:07 UTC
oh, you mean the evolution change... I think we should make up our mind if we're talking about gtk or evolution in here.
Comment 18 Milan Crha 2016-03-23 16:35:21 UTC
(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.
Comment 19 GNOME Infrastructure Team 2018-05-02 16:58:54 UTC
-- 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.