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 739245 - support multiple desktops in XDG_CURRENT_DESKTOP
support multiple desktops in XDG_CURRENT_DESKTOP
Status: RESOLVED FIXED
Product: gnome-menus
Classification: Core
Component: libgnome-menu
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-menus dummy account
gnome-menus dummy account
Depends on:
Blocks:
 
 
Reported: 2014-10-27 14:30 UTC by Alberts Muktupāvels
Modified: 2014-11-02 20:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
desktop-entries: support multiple desktops in XDG_CURRENT_DESKTOP (4.95 KB, patch)
2014-10-27 14:30 UTC, Alberts Muktupāvels
needs-work Details | Review
support multiple desktops in XDG_CURRENT_DESKTOP (4.98 KB, patch)
2014-10-27 16:43 UTC, Alberts Muktupāvels
accepted-commit_now Details | Review

Description Alberts Muktupāvels 2014-10-27 14:30:33 UTC
Created attachment 289413 [details] [review]
desktop-entries: support multiple desktops in XDG_CURRENT_DESKTOP

Add support for multiple desktops in XDG_CURRENT_DESKTOP.
Comment 1 Jasper St. Pierre (not reading bugmail) 2014-10-27 16:26:49 UTC
Review of attachment 289413 [details] [review]:

::: libmenu/desktop-entries.c
@@ +161,1 @@
       strv = g_key_file_get_string_list (key_file,

Can you get these outside of the loop?

@@ +180,3 @@
+      else
+        {
+          strv = g_key_file_get_string_list (key_file,

You stomp on the other strv, causing a leak. Use two different variables for this. And fetch them outside the loop.

@@ +192,3 @@
+                  if (g_str_equal (strv[j], current_desktops[i]))
+                    {
+                      show_in = FALSE;

If you do something like:

...

  if (g_str_equal (not_show_in[j], current_desktops[i]))
   {
     show_in = FALSE;
     goto out;
   }

...

out:
  g_strfreev (only_show_in);
  g_strfreev (not_show_in);
  return show_in;

That would drastically simplify the code.
Comment 2 Alberts Muktupāvels 2014-10-27 16:43:59 UTC
Created attachment 289469 [details] [review]
support multiple desktops in XDG_CURRENT_DESKTOP
Comment 3 Alberts Muktupāvels 2014-10-27 16:48:50 UTC
Thanks for quick review. Patch updated.

Was it really memory leak? NotShowIn was read only if ShowOnlyIn returned NULL...
Comment 4 Jasper St. Pierre (not reading bugmail) 2014-10-27 16:49:07 UTC
Review of attachment 289469 [details] [review]:

Looks good.
Comment 5 Alberts Muktupāvels 2014-10-27 21:01:42 UTC
Thanks! When you are planning to commit this patch? Do you have any plans on making 3.14 release?
Comment 6 Alberts Muktupāvels 2014-11-02 20:03:23 UTC
(In reply to comment #4)
> Review of attachment 289469 [details] [review]:
> 
> Looks good.

Jasper, are you going to commit this patch?
Comment 7 Jasper St. Pierre (not reading bugmail) 2014-11-02 20:42:40 UTC
When I mark a patch as ACN, I expect the bug reporter to push it.
Comment 8 Alberts Muktupāvels 2014-11-02 20:46:12 UTC
(In reply to comment #7)
> When I mark a patch as ACN, I expect the bug reporter to push it.

Ok, pushed. Thanks!