search

mate-desktop / mate-screensaver

MATE screen saver and locker
https://mate-desktop.org
GNU General Public License v2.0
48 stars 40 forks source link

Replace deprecated gtk_css_provider_get_default #203

Closed raveit65 closed 5 years ago

raveit65 commented 5 years ago

Please test.

lukefromdc commented 5 years ago

This looks like we make a new cssprovider each time the theme is changed. Does the old one need to be freed or is it still used anywhere?

sc0w commented 5 years ago

original gtk function:

GtkCssProvider *
gtk_css_provider_get_default (void)
{
  static GtkCssProvider *provider;

  if (G_UNLIKELY (!provider))
    {
      provider = gtk_css_provider_new ();
    }

  return provider;
}

in this PR I suggest this line instead:

static GtkCssProvider *style_provider = gtk_css_provider_new ();

and the provider is created only one time.

lukefromdc commented 5 years ago

Declaring the cssprovider as static is exactly what we have done elsewhere in similar commits and it has worked

raveit65 commented 5 years ago

PR is updated, please review again.

sc0w commented 5 years ago

I think the job must be done at the same line, and gtk_css_provider_new will be called only one time

    if (g_file_test (css, G_FILE_TEST_IS_REGULAR))
    {
        static GtkCssProvider *style_provider = gtk_css_provider_new ();
        gtk_css_provider_load_from_path (style_provider, css, NULL);
    }
raveit65 commented 5 years ago

The result is a warning if i do it like your suggestion.

gs-lock-plug.c: In function 'load_theme':
gs-lock-plug.c:2011:43: error: initializer element is not constant
 2011 |   static GtkCssProvider *style_provider = gtk_css_provider_new ();
      |                                           ^~~~~~~~~~~~~~~~~~~~
make[3]: *** [Makefile:971: gs-lock-plug.o] Error 1
raveit65 commented 5 years ago

I have no idea what i should use for G_UNLIKELY when doing something like this?

GtkCssProvider *
gtk_css_provider_get_default (void)
{
  static GtkCssProvider *provider;

  if (G_UNLIKELY (!provider))
    {
      provider = gtk_css_provider_new ();
    }

  return provider;
}

See this example https://github.com/mate-desktop/pluma/commit/d3757df#diff-8aa7cde5ea9987eb3addd88a057f2f05R712

sc0w commented 5 years ago

ok, I think this must work:

        if (g_file_test (css, G_FILE_TEST_IS_REGULAR))
        {
               static GtkCssProvider *style_provider = NULL;

               if (style_provider == NULL)
                      style_provider = gtk_css_provider_new ();

               gtk_css_provider_load_from_path (style_provider, css, NULL);
        }
raveit65 commented 5 years ago

PR is updated, please review again.

lukefromdc commented 5 years ago

See https://github.com/mate-desktop/mate-desktop/pull/408#issuecomment-520080138 and https://github.com/mate-desktop/pluma/commit/d3757dfa7b822dba1dc1b686ae60fdf77a097cf2 for how this had to be handled in Pluma's override_font code

raveit65 commented 5 years ago

And what you want to see here from your references? Btw. what is wrong here?

raveit65 commented 5 years ago

Should i close PR?

sc0w commented 5 years ago

@lukefromdc I answered you in https://github.com/mate-desktop/mate-desktop/pull/408#issuecomment-520100953

lukefromdc commented 5 years ago

I am thinking we may still get a new cssprovider each time, but I do NOT know enough about it to know for sure. If we do, we may be getting that result in master as well, as cssprovider_get_default is mostly a wrapper around cssprovider_new .

Not blocking anything her, just bringing up we've dealt with this before in Pluma. My work there was based on Albert's advice.

On 8/9/2019 at 6:50 PM, "raveit65" notifications@github.com wrote:

And what you want to see here from your references? Btw. what is wrong here?

-- You are receiving this because your review was requested. Reply to this email directly or view it on GitHub: https://github.com/mate-desktop/mate- screensaver/pull/203#issuecomment-520088357

rbuj commented 5 years ago

style_provider is not used:

$ git diff
diff --git a/src/gs-lock-plug.c b/src/gs-lock-plug.c
index c3081c2..f52c888 100644
--- a/src/gs-lock-plug.c
+++ b/src/gs-lock-plug.c
@@ -1984,6 +1984,7 @@ load_theme (GSLockPlug *plug)
        GtkBuilder *builder;
        GtkWidget  *lock_dialog;
        GError     *error=NULL;
+       static GtkCssProvider *style_provider = NULL;

        theme = get_dialog_theme_name (plug);
        if (theme == NULL)
@@ -2008,8 +2009,6 @@ load_theme (GSLockPlug *plug)
        g_free (filename);
        if (g_file_test (css, G_FILE_TEST_IS_REGULAR))
        {
-               static GtkCssProvider *style_provider = NULL;
-
                if (style_provider == NULL)
                        style_provider = gtk_css_provider_new ();

@@ -2029,6 +2028,7 @@ load_theme (GSLockPlug *plug)

        lock_dialog = GTK_WIDGET (gtk_builder_get_object(builder, "lock-dialog"));
        gtk_container_add (GTK_CONTAINER (plug), lock_dialog);
+       gtk_style_context_add_provider_for_screen (gdk_screen_get_default(), GTK_STYLE_PROVIDER (style_provider), GTK_STYLE_PROVIDER_PRIORITY_USER);

        plug->priv->vbox = NULL;
        plug->priv->notebook = GTK_WIDGET (gtk_builder_get_object(builder, "notebook"));

test:

sudo cp /usr/share/mate-screensaver/lock-dialog-default.ui /usr/share/mate-screensaver/lock-dialog-test.ui
sudo wget -O /usr/share/mate-screensaver/lock-dialog-test.css https://gist.githubusercontent.com/geki-yaba/19262f9af9b2a6f838636e6f019e96a3/raw/24a5ee22a0338c6c145db15f363bcbe73c64630d/gistfile1.txt 
gsettings set org.mate.screensaver lock-dialog-theme test
mate-screensaver-command -l
sc0w commented 5 years ago

@lukefromdc

I am thinking we may still get a new cssprovider each time, but I do NOT know enough about it to know for sure. If we do, we may be getting that result in master as well, as cssprovider_get_default is mostly a wrapper around cssprovider_new . Not blocking anything her, just bringing up we've dealt with this before in Pluma. My work there was based on Albert's advice.

I did the same in pluma with https://github.com/mate-desktop/pluma/pull/495, and you can test it by yourself easily there with printf for example, showing messages. 


if (provider == NULL)
    printf ("new provider added!\n");
else
    printf("using old provider!\n");

@rbuj this PR currently works exactly as before, your proposed changes seems to add new behavior, probably it must be added with new commit?

sc0w commented 5 years ago

@rbuj

gtk_style_context_add_provider_for_screen only needs to work one time over the same provider

I think the job can be done into the if (style_provider == NULL) condition:

        if (g_file_test (css, G_FILE_TEST_IS_REGULAR))
        {
               static GtkCssProvider *style_provider = NULL;

               if (style_provider == NULL)
               {
                      style_provider = gtk_css_provider_new ();
                      gtk_style_context_add_provider_for_screen (gdk_screen_get_default(), GTK_STYLE_PROVIDER (style_provider), GTK_STYLE_PROVIDER_PRIORITY_USER);
               }

               gtk_css_provider_load_from_path (style_provider, css, NULL);
        }
rbuj commented 5 years ago

@sc0w

1st case: lock-dialog-theme is set to 'default'

if lock-dialog-theme is set to 'default' and gtk-theme is set to 'TraditionalOk':

$ gsettings get org.mate.screensaver lock-dialog-theme
'default'
$ gsettings get org.mate.interface gtk-theme
'TraditionalOk'

then you should apply the css code stored at /usr/share/themes/TraditionalOk/gtk-3.0/mate-applications.css:

$ grep lock-dialog /usr/share/themes/TraditionalOk/gtk-3.0/*.css
/usr/share/themes/TraditionalOk/gtk-3.0/mate-applications.css:.lock-dialog {
/usr/share/themes/TraditionalOk/gtk-3.0/mate-applications.css:.lock-dialog notebook {

2nd case: lock-dialog-theme is not set to 'default'

If lock-dialog-theme is not set to 'default':

$ gsettings get org.mate.screensaver lock-dialog-theme
'test'

then you should apply the css code stored at /usr/share/mate-screensaver/lock-dialog-test.css.

diff --git a/src/gs-lock-plug.c b/src/gs-lock-plug.c
index c3081c2..0a64acf 100644
--- a/src/gs-lock-plug.c
+++ b/src/gs-lock-plug.c
@@ -1984,6 +1984,7 @@ load_theme (GSLockPlug *plug)
        GtkBuilder *builder;
        GtkWidget  *lock_dialog;
        GError     *error=NULL;
+       static GtkCssProvider *style_provider = NULL;

        theme = get_dialog_theme_name (plug);
        if (theme == NULL)
@@ -2004,15 +2005,15 @@ load_theme (GSLockPlug *plug)
        filename = g_strdup_printf ("lock-dialog-%s.css", theme);
        g_free (theme);

-       css = g_build_filename (GTKBUILDERDIR, filename, NULL);
-       g_free (filename);
-       if (g_file_test (css, G_FILE_TEST_IS_REGULAR))
+       if (style_provider == NULL)
        {
-               static GtkCssProvider *style_provider = NULL;
-
-               if (style_provider == NULL)
-                       style_provider = gtk_css_provider_new ();
+               style_provider = gtk_css_provider_new ();
+       }

+        css = g_build_filename (GTKBUILDERDIR, filename, NULL);
+        g_free (filename);
+       if (g_file_test (css, G_FILE_TEST_IS_REGULAR))
+       {
                gtk_css_provider_load_from_path (style_provider, css, NULL);
        }
        g_free (css);
@@ -2029,6 +2030,7 @@ load_theme (GSLockPlug *plug)

        lock_dialog = GTK_WIDGET (gtk_builder_get_object(builder, "lock-dialog"));
        gtk_container_add (GTK_CONTAINER (plug), lock_dialog);
+       gtk_style_context_add_provider_for_screen (gdk_screen_get_default(), GTK_STYLE_PROVIDER (style_provider), GTK_STYLE_PROVIDER_PRIORITY_USER);

        plug->priv->vbox = NULL;
        plug->priv->notebook = GTK_WIDGET (gtk_builder_get_object(builder, "notebook"));

or

diff --git a/src/gs-lock-plug.c b/src/gs-lock-plug.c
index c3081c2..38bfec5 100644
--- a/src/gs-lock-plug.c
+++ b/src/gs-lock-plug.c
@@ -1984,6 +1984,7 @@ load_theme (GSLockPlug *plug)
        GtkBuilder *builder;
        GtkWidget  *lock_dialog;
        GError     *error=NULL;
+       static GtkCssProvider *style_provider = NULL;

        theme = get_dialog_theme_name (plug);
        if (theme == NULL)
@@ -2001,21 +2002,19 @@ load_theme (GSLockPlug *plug)
                return FALSE;
        }

-       filename = g_strdup_printf ("lock-dialog-%s.css", theme);
-       g_free (theme);
-
-       css = g_build_filename (GTKBUILDERDIR, filename, NULL);
-       g_free (filename);
-       if (g_file_test (css, G_FILE_TEST_IS_REGULAR))
+       if (style_provider == NULL)
        {
-               static GtkCssProvider *style_provider = NULL;
-
-               if (style_provider == NULL)
-                       style_provider = gtk_css_provider_new ();
-
-               gtk_css_provider_load_from_path (style_provider, css, NULL);
+               style_provider = gtk_css_provider_new ();
+               filename = g_strdup_printf ("lock-dialog-%s.css", theme);
+               css = g_build_filename (GTKBUILDERDIR, filename, NULL);
+               g_free (filename);
+               if (g_file_test (css, G_FILE_TEST_IS_REGULAR))
+               {
+                       gtk_css_provider_load_from_path (style_provider, css, NULL);
+               }
+               g_free (css);
        }
-       g_free (css);
+       g_free (theme);

        builder = gtk_builder_new();
        if (!gtk_builder_add_from_file (builder,gtkbuilder,&error))
@@ -2029,6 +2028,7 @@ load_theme (GSLockPlug *plug)

        lock_dialog = GTK_WIDGET (gtk_builder_get_object(builder, "lock-dialog"));
        gtk_container_add (GTK_CONTAINER (plug), lock_dialog);
+       gtk_style_context_add_provider_for_screen (gdk_screen_get_default(), GTK_STYLE_PROVIDER (style_provider), GTK_STYLE_PROVIDER_PRIORITY_USER);

        plug->priv->vbox = NULL;
        plug->priv->notebook = GTK_WIDGET (gtk_builder_get_object(builder, "notebook"));
raveit65 commented 5 years ago

Behaviour isn't change by this PR. I works same like before. @rbuj Feel free to open a new PR if you think old behaviour is wrong.

rbuj commented 5 years ago

New PR https://github.com/mate-desktop/mate-screensaver/pull/204