search

mate-desktop / mate-settings-daemon

MATE settings daemon
https://mate-desktop.org
GNU General Public License v2.0
43 stars 47 forks source link

msd-xrdb-manager.c: avoid deprecated 'gtk_widget_ensure_style' #248

Closed sc0w closed 5 years ago

sc0w commented 5 years ago

Seems gtk_widget_ensure_style is useless, according to the documentation:

https://developer.gnome.org/gtk3/stable/GtkWidget.html#gtk-widget-ensure-style

Not a very useful function; most of the time, if you want the style, the widget is realized, and realized widgets are guaranteed to have a style already.

muktupavels commented 5 years ago

Does it work? Your quoted info does not match with what happens in code... From what I see window is created, but it is never shown or presented and that means that widget is not realized!

monsta commented 5 years ago

Hmm... where is that manager->priv->widget displayed actually?

monsta commented 5 years ago

Ok, looks like that widget is only used to get GtkStyle and GdkColor from it, convert them into string representations and then pass these to xrdb tool. I don't know why it's done. Does it apply current theme settings to bare X apps, or something?

Upstream plugin had the same code before it was completely removed in https://github.com/GNOME/gnome-settings-daemon/commit/cc982caf2d02eeebf85ed763ddd4ca0819d72435.

sc0w commented 5 years ago

Honestly, I have no idea how to test this PR, but I did the same in https://github.com/mate-desktop/mate-control-center/pull/388 and https://github.com/mate-desktop/mate-calc/pull/66, and the widgets affected worked as expected.

lukefromdc commented 5 years ago

What happens if we remove this plugin entirely?

raveit65 commented 5 years ago

In 2014 i had a issue with that plugin in rhel7. Issue was caused because a dependency was removed for xorg-server-utils in rhel7. And emacs and tk got segfaults. https://bugzilla.redhat.com/show_bug.cgi?id=1140329 https://src.fedoraproject.org/cgit/rpms/mate-settings-daemon.git/commit/?h=epel7&id=9ae307516caaaf92dc602e91072d3c94846c4142

I fixed that with using a fix from kde. See https://bugzilla.redhat.com/show_bug.cgi?id=1053891#c4 But before i use the fix i suggested to disable the plugin as workaround. So i think it is save to remove.

sc0w commented 5 years ago

The same PR worked in mate-control-center and mate-calc, so, it seems to be safe the merge.

I never used that plugin, so, I agree if you decide to remove it.

monsta commented 5 years ago

Ok, so it's really for applying our theme settings to these apps... and KDE 4 also did that, as I see from these bug reports on rhbz.

Not a very useful function; most of the time, if you want the style, the widget is realized, and realized widgets are guaranteed to have a style already.

Well, this case seems different from other uses of gtk_widget_ensure_style in other projects. Here the widget isn't realized, so this function is used to make sure it has our theme settings applied to it, as I understand it. That said, GtkStyle is deprecated, and so is GdkColor, so this "making sure" should be probably done via GtkStyleContext somehow?

muktupavels commented 5 years ago

Perhaps you should reconsider your attitude toward MATE project and/or other team mates? Merging something you have no clue about is not good thing...

I already told you why in this case this could not be same case as other changes. It took me less than 10 minutes to verify that in this particular case gtk_widget_ensure_style is needed. You can argue that this plugin might not be needed, but it is not excuse to merge broken "fix"...

/*
 * compile with:
 * gcc -Wall `pkg-config --cflags gtk+-3.0` -o style style.c `pkg-config --libs gtk+-3.0`
 */

#include <gtk/gtk.h>
#include <stdio.h>

int
main (void)
{
  GtkWidget *widget;
  GtkStyle *style;
  GdkColor *color;

  gtk_init (NULL, NULL);

  widget = gtk_window_new (GTK_WINDOW_TOPLEVEL);
  gtk_widget_ensure_style (widget);

  style = gtk_widget_get_style (widget);

  color = &style->bg[GTK_STATE_NORMAL];
  printf ("BACKGROUND: #%2.2hx%2.2hx%2.2hx\n",
          color->red >> 8, color->green >> 8, color->blue >> 8);

  gtk_widget_destroy (widget);

  return EXIT_SUCCESS;
}

With gtk_widget_ensure_style: BACKGROUND: #f2f1f0 Without gtk_widget_ensure_style: BACKGROUND: #dcdad5

sc0w commented 5 years ago

ok, sorry, no problem, if someone can confirm that, the commit can be reverted.

raveit65 commented 5 years ago

Perhaps you should reconsider your attitude toward MATE project and/or other team mates? Merging something you have no clue about is not good thing...

.....harsh words.....

@sc0w Can we remove that plugin? I don't really thing that we need that if gnome have already remove it. No unneeded code --> no bugs --> no harsh words ;)

raveit65 commented 5 years ago

PS: i will disable the plugin via gsettings key for testing. Let see if it does make any different.

raveit65 commented 5 years ago

https://github.com/GNOME/gnome-settings-daemon/commit/cc982caf2d02eeebf85ed763ddd4ca0819d72435 Nice commit message :) Because it's not the 1980's any more.

raveit65 commented 5 years ago

And parts of the code is already removed by @monsta https://github.com/mate-desktop/mate-settings-daemon/commit/606acad03a43083d1821e4b170c838a9b8dd40b2#diff-e890176499e85a83b872dd29249888cc

monsta commented 5 years ago

I've tested it with editres and tkdiff apps. This plugin applies our theme settings (at least some of them) to these apps. It only does so on start though. I couldn't make it react to any theme changes without restarting.

monsta commented 5 years ago

Forgot to mention that my testing was done w/o this PR, I didn't update to latest git master at that time. Will check the latest changes too.

raveit65 commented 5 years ago

I tested tkdiffs with the PR and the plugin applies theme setting only during session start to this application. If the plugin is disabled than tkdiffs desn't use our theme and simply use the gtk+ default theme (adwaita). Interesting that gnome gives a fuck about it. In gnome-shell session tkdiffs use always the the gtk+ default theme, no matter which other them is used. So, it looks like we need this plugin to support old-school applications. But i see no problems with reading theme settings during session start.

Btw. all those old apps are unusable with modern HIDPI monitors.

monsta commented 5 years ago

@muktupavels: I confirm the problem, I get different results with this PR and with previous commit in master (https://github.com/mate-desktop/mate-settings-daemon/commit/44343657a4dfa2d6d73b373663e84ff553629990).

With PR:

editres-no-ensure tkdiff-no-ensure

Without it:

editres-ensure-style tkdiff-ensure-style

muktupavels commented 5 years ago

And there might be problems also in other projects... Just gave quick look at mate-calc and there style is also used before widget is shown/realized...

I also don't get what was reason to removing one deprecated function when there are other related deprecated functions... From the beginning correct thing would have been to port that to GtkStyleContext.

raveit65 commented 5 years ago

I am voting for removing this plugin. The few old application will simply use adwaita theme (gtk+ fallback). No need for using deprecated code only for a few users. I never heard that cinnamon or gnome users have a problem with this decision from early gnome-3.0 branch. I will start with fedora rawhide/30 with removing this plugin. Feel free to follow or have fun with old deprecated code.

sc0w commented 5 years ago

I agree with removing the plugin

@monsta btw, step by step, how can I show that windows?

monsta commented 5 years ago

@sc0w: x11-utils package contains editres and a few more apps, and tkdiff app is in tkcvs package.

Personally, I don't mind removing this plugin.

lukefromdc commented 5 years ago

One point about MATE is that we've long been the DE people use if they want to do things they way they always have. That may well include legacy apps e.g I still use Audacious (a currently supported fork of XMMS, XMMS never got past GTK 1.4) as my audio player, complete with its standalone skins.

lukefromdc commented 5 years ago

I just found out git-gui is affected by this, so if we do remove this plugin I will have to use a startup script to ensure it gets enough of my theme to avoid white-on-white text

lukefromdc commented 5 years ago

See https://github.com/mate-desktop/mate-settings-daemon/pull/254#issuecomment-444373874 if the plugin is broken git-gui is hard to use, its just ugly if the plugin is disabled (or does not exist). Plenty readable though.

raveit65 commented 5 years ago

git-gui runs fine with https://github.com/mate-desktop/mate-settings-daemon/pull/254 at my boxes.

lukefromdc commented 5 years ago

With https://github.com/mate-desktop/mate-settings-daemon/pull/254 everything works fine, it was before this PR and after https://github.com/mate-desktop/mate-settings-daemon/pull/248 that I had problems with it. It works before the first PR, after the second PR, and is working but ugly if the plugin is disabled at session start. Between those two PR's and with the plugin active is what gave white on white text.