mate-desktop / marco

MATE default window manager
https://mate-desktop.org
GNU General Public License v2.0
196 stars 87 forks source link

Replace deprecated API and remove compilation warnings #748

Closed zhuyaliang closed 1 year ago

zhuyaliang commented 1 year ago

Replace deprecated API and Remove some compilation warnings

lukefromdc commented 1 year ago

I don't know much about this part of the code, as most of what I've done with march concerned understanding the libmarco portions also used by compiz, and I have not played with keybindings or the underlying code that much simply as a consequence of my workflow. My test determined only that it worked, I will leave the "is this an upgrade or not" question to those with a better understanding of this part of the code

zhuyaliang commented 1 year ago

@cwendling

[22/190] Compiling C object src/libmarco-private.so.2.2600.0.p/core_keybindings.c.o
../src/core/keybindings.c: In function 'reload_modmap':
../src/core/keybindings.c:201:31: warning: variable 'str' set but not used [-Wunused-but-set-variable]
  201 |                   const char *str;
      |                               ^~~
../src/core/keybindings.c: In function 'meta_display_process_key_event':
../src/core/keybindings.c:1287:15: warning: variable 'str' set but not used [-Wunused-but-set-variable]
 1287 |   const char *str;
      |               ^~~
[25/190] Compiling C object src/libmarco-private.so.2.2600.0.p/core_screen.c.o
../src/core/screen.c: In function 'meta_screen_sn_event':
../src/core/screen.c:2742:21: warning: variable 'wmclass' set but not used [-Wunused-but-set-variable]

I can remove these changes and keep these warnings temporarily

cwendling commented 1 year ago

@zhuyaliang ah, I get it now, it's all meta_topic() calls, and you're building without WITH_VERBOSE_MODE, right? In this case, meta_topic() is a no-op, so nothing uses the variable.

For this case, we can either use your code (which I don't quite like as mentioned above, but that probably is fairly harmless in practice, yet I don't really know the cost of those functions), or the variable could be behind a #ifdef WITH_VERBOSE_MODE:

diff --git a/src/core/keybindings.c b/src/core/keybindings.c
index 6211a94..92f8758 100644
--- a/src/core/keybindings.c
+++ b/src/core/keybindings.c
@@ -194,6 +194,7 @@ reload_modmap (MetaDisplay *display)

           while (j < display->keysyms_per_keycode)
             {
+#ifdef WITH_VERBOSE_MODE
               if (syms[j] != 0)
                 {
                   const char *str;
@@ -204,6 +205,7 @@ reload_modmap (MetaDisplay *display)
                               str ? str : "none",
                               (1 << ( i / modmap->max_keypermod)));
                 }
+#endif /* WITH_VERBOSE_MODE */

               if (syms[j] == XK_Num_Lock)
                 {

A third option could also be shutting up the compiler, letting it know we know about that:

diff --git a/src/core/keybindings.c b/src/core/keybindings.c
index 6211a94..067136d 100644
--- a/src/core/keybindings.c
+++ b/src/core/keybindings.c
@@ -196,7 +196,7 @@ reload_modmap (MetaDisplay *display)
             {
               if (syms[j] != 0)
                 {
-                  const char *str;
+                  const char *str G_GNUC_UNUSED;

                   str = XKeysymToString (syms[j]);
                   meta_topic (META_DEBUG_KEYBINDINGS,

Either way, now that I see it's for debug messages, I'm less concerned. This said, it seems to be the default to build with verbose mode enabled, so it makes sense for that to be the best-behaving code path.

cwendling commented 1 year ago

This said, it seems to be the default to build with verbose mode enabled, so it makes sense for that to be the best-behaving code path.

My bad, the default changed a while back in 397e31879bf79861a1a21ec4e2a75017e243d34b, so the default is not to have verbose mode by default.

muktupavels commented 1 year ago

This said, it seems to be the default to build with verbose mode enabled, so it makes sense for that to be the best-behaving code path.

My bad, the default changed a while back in 397e318, so the default is not to have verbose mode by default.

So verbose logging has been repurposed as something intended for embedded devices... So what do you all do now, when you need verbose output? Do you ask users to recompile marco?

cwendling commented 1 year ago

My bad, the default changed a while back in 397e318, so the default is not to have verbose mode by default.

So verbose logging has been repurposed as something intended for embedded devices... So what do you all do now, when you need verbose output? Do you ask users to recompile marco?

Clearly the help string update was unfortunate. And well, I wouldn't know, but it indeed seems a bit misguided unless it has a sizeable performance penalty affecting real use cases, but probably @rbuj knows -- yet the #666 doesn't say, and merely lines it up to the newer meson default…

zhuyaliang commented 1 year ago

@cwendling Update completed

zhuyaliang commented 1 year ago

I will close this PR