Merge lp:~junrrein/pantheon-files/fix-1069275 into lp:~elementary-apps/pantheon-files/trunk

[Julián Unrrein]

Do not prematurely destroy the file list used on the Properties window. Fixes bug #1069275.

When creating a Properties window from a context menu created from the directory's blank space, the file list that holds information about that directory was destroyed just after creating the Properties window. This made possible that the folder info showed up correctly, but that it also was impossible to change.

[Julián Unrrein (junrrein)]

Is it necessary to destroy the list explicitely, or does Vala handles that automatically?

If it is necesarry, what would be the best way? Destroy the list when destroying the Properties window (connecting to the "destroy" signal) ?

[Sergey "Shnatsel" Davidoff (shnatsel)]

Vala's memory management is explained at https://live.gnome.org/Vala/ReferenceHandling

This piece looks like C to me anyway, so yes, you'll have to free the list explicitly OR rewrite the whole thing in Vala. I'm not really a programmer and I don't know what info exactly the list holds, so I'm afraid I can't comment further.

[Julián Unrrein (junrrein)]

Thanks for the link Shnatsel!

This piece is, in fact, C code. But the Properties window (created with "marlin_view_properties_window_new") is Vala code. That's why I thought the properties window would handle the deletion of the list.

It turns out that the Properties window holds an "unowned" reference to the list. This means the Properties window won't free the list on its side. So, I'll have to free the list explicitely on the C side.

[Julián Unrrein (junrrein)]

I tried this:

MarlinViewPropertiesWindow *properties = marlin_view_properties_window_new (file_list, view, GTK_WINDOW (view->details->window));
g_signal_connect (properties, "destroy", G_CALLBACK (g_list_free), file_list);

But now Files segfaults when closing the Properties window.
Unfortunately, I don't know what to do now. Need help guys.

[lp:~junrrein/pantheon-files/fix-1069275]

1085. By Julián Unrrein on 2013-01-28

Implement a real fix (one that handles memory properly).

[Julián Unrrein (junrrein)]

Now it should do.

Now, the Properties window copies the list that it receives in the constructor, and I let Vala's memory management take care of it.

[Cody Garver (codygarver)]

Nice!

review: Approve

[Julián Unrrein (junrrein)]

This commit brought a regression:

- Open the Properties window when a/some file/s are selected (not folders).
- Close it and select another file/folder

The selected files dissappear from the view. In icon view, clicking on the blank space where any of the files should be causes Files to crash. In list/column view, Files doesn't crash, but in column view it introduces weird behavior (the missing file/s are replaced with some random folders).

review: Needs Fixing

[Julián Unrrein (junrrein)]

In Vala, list.copy () returns a shallow copy of the list, so, while
the container itself is different, the members are the original ones.
So, at the end, it frees them, causing the breakage.

There are two alternatives to solve this:
- In the constructor of the Properties window, make a deep copy
  of the list that is received as a parameter.
    This one would be the best solution, since "from the outside"
    we no longer need to care if the Properties window will destroy
    the original members or not.
- When we create a Properties window from C code, pass a deep copy
  of the list as the parameter.
    Another solution, but the less ideal since now we must be careful
    to this every time we create a Properties window from C code.

I know how to implement #2, but #1 seems difficult because apparently
the GLib.List API in Vala doesn't support deep copies.

See:
http://developer.gnome.org/glib/2.35/glib-Doubly-Linked-Lists.html#g-list-copy-deep
http://www.valadoc.org/#!api=glib-2.0/GLib.List

I will try to implement #2 in the meantime.

[Julián Unrrein (junrrein)]

A missing note:
For some reason, this commit didn't break Files when viewing the properties of folders. I can't explain that.

=== modified file 'src/View/PropertiesWindow.vala'
--- src/View/PropertiesWindow.vala	2012-11-06 20:18:52 +0000
+++ src/View/PropertiesWindow.vala	2013-01-28 19:25:24 +0000
@@ -35,7 +35,7 @@
     private Gtk.ListStore store_apps;
 
     private uint count;
-    private unowned GLib.List<GOF.File> files;
+    private GLib.List<GOF.File> files;
     private GOF.File goffile;
     private FM.Directory.View view;
 
@@ -83,7 +83,7 @@
         content_vbox.margin_left = 5;
 
         view = _view;
-        files = _files;
+        files = _files.copy ();
         count = files.length();
         goffile = (GOF.File) files.data;