(In reply to comment #0)
> > Would be great to mount not
> > only an ISO into clients DVD drive or so but also to have a shared
> > drive which is a USB memory stick on the client and somehow available
> > on the host or so.
> This way we'd have a transparent file exchange in both directions.
> elmarco: zeenix said you did some similar things in spice. Do you have more
> information about this?

Yes, there is a "webdav" channel feature that is not yet fully ready. Well, it is working, but all components have not been released, and testing has been very limited so far. Feel free to give it a try, there are some pretty complete instructions here:

http://lists.freedesktop.org/archives/spice-devel/2014-April/016582.html

Based on that folder sharing, I think we could implement later a better "native" file drag-and-drop..

btw, I was planning to add Boxes support soonish, however it's a bit hard to test that all required packages version are being used. In particular a yet-to-be-released-version of spice server, and qemu "master" (1.7 stable branch still has a virtio/spice port bounding bug). Neverthess, the libvirt-glib bits are now upstream ready to be consummed by boxes, after a release happen ;)

Sounds great :) I see forward to this.

(In reply to comment #2)
> btw, I was planning to add Boxes support soonish, however it's a bit hard to
> test that all required packages version are being used. In particular a
> yet-to-be-released-version of spice server, and qemu "master" (1.7 stable
> branch still has a virtio/spice port bounding bug). Neverthess, the
> libvirt-glib bits are now upstream ready to be consummed by boxes, after a
> release happen ;)

I'm hoping everything is merged and release by now? If so, if you could work on this before 3.16, that would be immensely appreciated.

(In reply to comment #4)
> (In reply to comment #2)
> > btw, I was planning to add Boxes support soonish, however it's a bit hard to
> > test that all required packages version are being used. In particular a
> > yet-to-be-released-version of spice server, and qemu "master" (1.7 stable
> > branch still has a virtio/spice port bounding bug). Neverthess, the
> > libvirt-glib bits are now upstream ready to be consummed by boxes, after a
> > release happen ;)
> 
> I'm hoping everything is merged and release by now? If so, if you could work on
> this before 3.16, that would be immensely appreciated.

Hello? Marc-Andre thats for you. If you don't have time for this anytime soon, please say so I could allocate time to look into this.

Please look into it, it should be fairly straightforward.
See also https://elmarco.fedorapeople.org/manual.html#_folder_sharing

(In reply to comment #6)
> Please look into it, it should be fairly straightforward.
> See also https://elmarco.fedorapeople.org/manual.html#_folder_sharing

Cool! Will do. Thanks.

(In reply to comment #7)
> (In reply to comment #6)
> > Please look into it, it should be fairly straightforward.
> > See also https://elmarco.fedorapeople.org/manual.html#_folder_sharing
> 
> Cool! Will do. Thanks.

Actually its not very straightforward at all since it requires configuration in the guest. :( I guess it'll be something that we can make it to work out of the box with express installations only. The linux part should be easy (depending on whether we want libosinfo's script templates to unconditionally install the package(s) or not) but windows one would need some digging and work.

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Please look into it, it should be fairly straightforward.
> > > See also https://elmarco.fedorapeople.org/manual.html#_folder_sharing
> > 
> > Cool! Will do. Thanks.
> 
> Actually its not very straightforward at all since it requires configuration in
> the guest. :( I guess it'll be something that we can make it to work out of the
> box with express installations only. The linux part should be easy (depending
> on whether we want libosinfo's script templates to unconditionally install the
> package(s) or not) but windows one would need some digging and work.

I can see several ways you could get spice-webdavd running the guest, for example, you could use virt-customize or use qemu-ga with nasty tricks... (assuming it is already installed - it should)

Ok, first things first. Let's first try to get this working in new VMs through express installation.

The mockup is here at the bottom:

https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/boxes/wires/file-sharing.png

Why not a GtkFileChooserButton for the source selection, instead of an entry/button combo?

(In reply to Zeeshan Ali (Khattak) from comment #10)
> Ok, first things first. Let's first try to get this working in new VMs
> through express installation.

With express installation I guess that you mean by having spice-webdavd into spice-guest-tools for windows or is there anything else?

(In reply to Victor Toso from comment #13)
> (In reply to Zeeshan Ali (Khattak) from comment #10)
> > Ok, first things first. Let's first try to get this working in new VMs
> > through express installation.
> 
> With express installation I guess that you mean by having spice-webdavd into
> spice-guest-tools for windows or is there anything else?

Also on Linux guests, we need to ensure right packages are installed as part of express installation.

(In reply to Bastien Nocera from comment #12)
> Why not a GtkFileChooserButton for the source selection, instead of an
> entry/button combo?

? The "Browse" button is supposed to be a GtkFileChooserButton AFAICT.

(In reply to Zeeshan Ali (Khattak) from comment #15)
> (In reply to Bastien Nocera from comment #12)
> > Why not a GtkFileChooserButton for the source selection, instead of an
> > entry/button combo?
> 
> ? The "Browse" button is supposed to be a GtkFileChooserButton AFAICT.

Look at what a GtkFileChooserButton looks like, and you can see that it will show the name of the selected directory, which is redundant with an entry (which the file chooser dialogue already has).

Created attachment 329379 [details] [review]
spice-display: Add UI elements for shared folders

We need these new UI elements so that we can implement the
new shared folders feature.

Add a shared folders section to the properties of SPICE.

Add a dialog for user input of shared folders.

Review of attachment 329379 [details] [review]:

Not good. I need to use a popover instead of a dialog and to consider the fact that we can currently have a single shared folder at a time.

Created attachment 330342 [details] [review]
spice-display: Add folder share in 'Device' page

This new section in the SPICE-display properties' 'DEVICE'
page will be used by the new Folder Sharing feature.

Created attachment 330343 [details] [review]
spice-display: Implement folder sharing

Created attachment 330344 [details] [review]
spice-display: Add folder share in DEVICES page

This new section in the SPICE-display properties' DEVICES
page will be used by the new folder sharing feature.

Change the DEVICES page name from "Devices" to "Devices & Share",
in order to accomodate the new feature.

Review of attachment 330343 [details] [review]:

No description of what is meant by folder sharing. :( From the title, it sounds like it implement the whole feature (especially due to lack of any description).

::: src/spice-display.vala
@@ +27,3 @@
     private bool closed;
 
+    private bool _share_folder;

* For simple properties, you don't need this, nor your own getter/setter implementation:

public bool share_folder { get; set; }

* Let's use the term "shared_folder" rather than "share_folder".

@@ +38,3 @@
+    }
+    private string _shared_folder;
+    private bool _share_folder_ro;

So you are going to have only 1 shared folder at time? That's not the design. We want user to be able to add/remove shared folders.

@@ +321,3 @@
     }
 
+    private void update_share_folder (Spice.Session session, bool share_folder) {

* No need to pass session to this instance method.
* share -> shared (in other places too).
* I think sharing of folders should always be enabled. Which folders to share, is what the UI should decide.

@@ +443,3 @@
+                    if(!can_share_folder (session)) {
+                        folder_share_toggle.active = false;
+                        got_error ("Please install spice-webdavd in guest.");

* Erroring out this late will make up for a horrible user experience. We should not be showing the folder sharing UI if guest can't handle it. Instead we should show a nice message telling user to install needed tools, preferably with a link to where to get them from.

* These messages go to UI and hence need to be nice and marked for translation.

Review of attachment 330343 [details] [review]:

No description of what is meant by folder sharing. :( From the title, it sounds like it implement the whole feature (especially due to lack of any description).

::: src/spice-display.vala
@@ +27,3 @@
     private bool closed;
 
+    private bool _share_folder;

* For simple properties, you don't need this, nor your own getter/setter implementation:

public bool share_folder { get; set; }

* Let's use the term "shared_folder" rather than "share_folder".

@@ +38,3 @@
+    }
+    private string _shared_folder;
+    private bool _share_folder_ro;

So you are going to have only 1 shared folder at time? That's not the design. We want user to be able to add/remove shared folders.

@@ +321,3 @@
     }
 
+    private void update_share_folder (Spice.Session session, bool share_folder) {

* No need to pass session to this instance method.
* share -> shared (in other places too).
* I think sharing of folders should always be enabled. Which folders to share, is what the UI should decide.

@@ +443,3 @@
+                    if(!can_share_folder (session)) {
+                        folder_share_toggle.active = false;
+                        got_error ("Please install spice-webdavd in guest.");

* Erroring out this late will make up for a horrible user experience. We should not be showing the folder sharing UI if guest can't handle it. Instead we should show a nice message telling user to install needed tools, preferably with a link to where to get them from.

* These messages go to UI and hence need to be nice and marked for translation.

Review of attachment 330343 [details] [review]:

::: src/spice-display.vala
@@ +38,3 @@
+    }
+    private string _shared_folder;
+    public string shared_folder {

Currently, SPICE doesn't have the ability to handle multiple shared folders at a time. I can make the design handle multiple shared folders, but the user would have to always select one of them.

Review of attachment 330343 [details] [review]:

::: src/spice-display.vala
@@ +27,3 @@
     private bool closed;
 
+    private bool _share_folder;

i though of it this way:
share_folder is true if we do share a folder/false otherwise.
shared_folder is the actual shared_folder ( regardless of the the design, it will always be one shared_folder)

We need both info, so I think that using the term shared_folder rather than share_folder may be a little confusing.

Created attachment 330625 [details] [review]
vm-configurator: Ensure WebDAV channel existence

Because folder sharing has been implemented in GNOME-Boxes, we now
have to make sure that the WebDAV channel always exists in the
machine's XML configuration file, for the feature to work correcty.

Review of attachment 330343 [details] [review]:

::: src/spice-display.vala
@@ +27,3 @@
     private bool closed;
 
+    private bool _share_folder;

Yeah, makes sense then but I don't see why wee need a separate boolean property for enabling/sharing. A value of null for "shared_folder" property should flag disabling folder sharing I think.

Review of attachment 330343 [details] [review]:

::: src/spice-display.vala
@@ +27,3 @@
     private bool closed;
 
+    private bool _share_folder;

I get what you mean, but there are certain implications to know before taking this decision:
1. We won't be able to save the value of shared_folder accross display disconnections (you can't save a null parameter apparently)
2. Session.shared_dir is not allowed to be null (we get a segmentation fault otherwise), so we would have to loose the binding to shared_folder and create a more elaborate setter method for the shared_folder property.


Also, referring to your comment on not needing my own getter/setter for simple properties, we need public getter/setter to save the properties in between display connection/disconnection.
When we will implement the multiple folder sharing, will we expect the user to input all the data again each time connecting to a machine ? It's a similar case here, on a minor scale.

Review of attachment 330343 [details] [review]:

::: src/spice-display.vala
@@ +27,3 @@
     private bool closed;
 
+    private bool _share_folder;

1: Well why can't you modify BoxConfig.save_property to be able to handle null string property?

2. You can have custom property binding.

about getter/setter, you are not doing anything at all other than what the default getter/setter do for you. So I don't know what you mean.

@@ +38,3 @@
+    }
+    private string _shared_folder;
+    private bool _share_folder_ro;

And apparently we can't query the path in the guest either. :( Can we at least give users a URL to connect to in the guest, like new mockups show?

Created attachment 331325 [details] [review]
spice-display: Always check for properties after 'USB Devices'

Currently, if USB devices are not enabled or are not found, we no
longer try to add any other property to the properties list.

The problem is that we will need to add another section,
'Folder Sharing' to the properties of spice-display, which
will follow 'USB Devices', so this behaviour isn't a valid
one anymore and thus needs to be changed.

Review of attachment 330343 [details] [review]:

::: src/spice-display.vala
@@ +38,3 @@
+    }
+    private string _shared_folder;
+    public string shared_folder {

This URL, although not quite good looking, appears to be constant for any local folder name or any machine.
dav+sd://Spice%2520client%2520folder._webdav._tcp.local/

Should I put this URL in the message from the mockups ?
Also, we won't have much to do with the Name entry, so it shouldn't appear in the UI for the moment.

Created attachment 331402 [details] [review]
spice-display: Add 'Folder Sharing' in 'Devices' page

Add a new section in the DEVICES property page of a SPICE display,
which will be used to further implement the Folder Sharing feature.

Created attachment 331403 [details] [review]
properties-page-widget: Change 'DEVICES' page name

Change the 'DEVICES' page name to 'Devices & Shares', in order to
accomodate the new 'Folder Sharing' section.

Created attachment 331617 [details] [review]
spice-display: Add 'Folder Sharing' in 'Devices' page

Add a new section in the DEVICES property page of a SPICE display,
which will be used to further implement the Folder Sharing feature.

Created attachment 331619 [details] [review]
spice-display: Implement Folder Sharing

Let the user enable and choose a shared folder from the host
machine to a guest machine with SPICE display.

This feature requires the WebDAV Channel to be present in the
guest's configuration and spice-webdavd to be installed
in the guest, which will be ensured in following patches.

Review of attachment 330343 [details] [review]:

::: src/spice-display.vala
@@ +38,3 @@
+    }
+    private string _shared_folder;
+    private bool _share_folder_ro;

That URL looks ugly cause of escape characters for spaces in URLs. who's genious idea was to provide a URL with spaces in it? :(

Anyway, we need to add API to query this URL and not just assume it. Given that URL is ugly and I do hope it changes at some point, it's further bad idea to assume it.

Review of attachment 330625 [details] [review]:

"Because folder sharing has been implemented in GNOME-Boxes" is it? this is the first patch in the series so that is definitely not true.

::: src/vm-configurator.vala
@@ +92,3 @@
+        spice_port.set_channel ("org.spice-space.webdav.0");
+        channel_webdav.set_source (spice_port);
+        channel_webdav.set_target_name ("org.spice-space.webdav.0");

Surely you see that you have 7 lines here that are duplicated below? Guess what I want you to do here? :)

Review of attachment 331325 [details] [review]:

I do apprecicate your effort to split the patch here but shortlog is pretty bad and sounds incorrect.

::: src/spice-display.vala
@@ +338,3 @@
                 var devs = get_usb_devices (manager);
 
+                if (devs.length > 0 && !(PropertyCreationFlag.NO_USB in flags) && Config.HAVE_USBREDIR && connected) {

PropertyCreationFlag.NO_USB has been dropped so you gotta rebase and see if this patch is really needed.

Review of attachment 331403 [details] [review]:

Shortlog is missing the most important info: Changed to what? You could always shorten "properties" to "props" in shortlog prefix and use "X -> Y" to summarize a rename.

Created attachment 332004 [details] [review]
vm-configurator: Ensure WebDAV channel existence

We have to make sure that the WebDAV channel always exists in the
machine's XML configuration file, for the planned shared folders
feature to work.

Review of attachment 331617 [details] [review]:

shortlog: "in" -> "on".

"Implement.. feature" term is very generic and should be avoided whenever possible. You could simply go with "Add UI for folder sharing, which will be implemented in a following patch".

::: data/gtk-style.css
@@ +66,3 @@
+.boxes-folder-sharing-notice-label {
+    color: #808080;
+    font-style: italic;

I don't think they are in italic, nor a different color in the mockup. Check again.

::: src/spice-display.vala
@@ +403,3 @@
             }
+
+            var frame_folder_share = new Gtk.Frame (null);

your naming scheme seems to be reversed: folder_share_frame. For local variables, you don't have to give very unique names. This could be just "frame".

@@ +436,3 @@
+            hbox_local_folder.pack_start (entry_folder_share);
+
+            var message = _("The local folder above can be accessed in the box by browsing to:\ndav+sd://Spice%2520client%2520folder._webdav._tcp.local/");

so with this naming scheme, reader would think this variable holds some pointer to directory holding images, rather than an image of directory icon.

Created attachment 332007 [details] [review]
spice-display: Try to add props following 'USB Devices'

Currently, if USB devices are not enabled or are not found, we no
longer try to add any other property to the properties list.

The problem is that we will need to add another section,
'Folder Sharing' to the properties of spice-display, which
will follow 'USB Devices', so this behaviour isn't a valid
one anymore and thus needs to be changed.

Review of attachment 331619 [details] [review]:

Not sure why you capitalise "folder sharing". We only do that in label/titles in UI and we capitalise all words there.

Since you are implementing it entirely in spice-display module, I'm guessing this will also enable it for SPICE remote boxes and hence you desrciption needs to be changed to not give the wrong impression that this is only for VMs.

Not sure why you capitalized "Channel" either. You want to mention that you are talking of local VMs in the last para. Oh and I'm guessing you didn't fix the order of patches since you actually add guest config in a previous patch.

::: src/spice-display.vala
@@ +318,3 @@
+                    channel.connect ();
+                else
+        foreach (var channel in channel_list) {

* Do we really need to disconnect from the channel? Can't we always connect to it?

* Won't this break after these steps:
  * User unselects the toggle (and shared_folder is set to "").
  * User selects the toggle (shared_folder is still set to "" so we'll try to disconnect again here.

?

@@ +502,3 @@
+            if (shared_folder != "")
+                entry_folder_share.text = shared_folder;
+            entry_folder_share.bind_property ("text", this, "shared-folder", BindingFlags.DEFAULT);

Same thing about reversed naming scheme.

Created attachment 332010 [details] [review]
props-page-widget: 'Devices' -> 'Devices & Shares'

Change the 'DEVICES' page name to 'Devices & Shares', in order to
accomodate the new 'Folder Sharing' section.

Review of attachment 331619 [details] [review]:

::: src/spice-display.vala
@@ +318,3 @@
+                    channel.connect ();
+                else
+                    channel.disconnect (Spice.ChannelEvent.NONE);

We could always connect to it. We do want the user to be able to disconnect it, so the only option would be to just set the empty string as shared directory, which makes the guest's shared folder drive return an error. I am not sure if this is a desirable behaviour, though.

SPICE doesn't seem to mind disconnecting from an already disconnected channel.

Review of attachment 331619 [details] [review]:

::: src/spice-display.vala
@@ +318,3 @@
+                    channel.connect ();
+                else
+        foreach (var channel in channel_list) {

No it's not a desired behaviour. I thought we want to set the shared foler on spice to null in that case and SPICE is able to handle that? If not, I guess we just go with this solution.

Review of attachment 331619 [details] [review]:

::: src/spice-display.vala
@@ +318,3 @@
+                    channel.connect ();
+                else
+                    channel.disconnect (Spice.ChannelEvent.NONE);

SPICE can't handle a null shared directory (seg fault), but that wouldn't stop us from considering the null shared folder as no shared folder.
The arguments for using the empty string instead of the null value are the following:
1) When retrieving the saved value for shared folder, the null value can't be retrieved as null, but maybe as an empty string --> requires some workarounds
2) The user can actually set the shared folder to an empty string (leaving the entry empty) and this must be considered as 'no shared folder' --> more workarounds

Either we would have many workarounds and extra custom bindings (I actually tried it and the code can become very complicated to follow and ugly) or we would consider both "" and null as 'no shared folder', which is more or less the same as using only 
"", but with the need to always check both cases.

I can prepare the version with the workarounds and custom bindings if you want, for comparison.

Created attachment 333423 [details] [review]
spice-display: Refactor 'USB Devices' property

The current way the code is organized doesn't allow for another
section to be further added after the 'USB Devices' section.

Based on the folder sharing UI mockups, we will add another
'Folder Sharing' section after 'USB Devices', so we need
some code refactoring.

Created attachment 333424 [details] [review]
spice-display: Add 'Folder Sharing' on 'Devices' page

Add UI for folder sharing, which will be implemented in
following patches.

Created attachment 333425 [details] [review]
props-page-widget: 'Devices' -> 'Devices & Shares'

Change the 'DEVICES' page name to 'Devices & Shares', in order to
accomodate the new 'Folder Sharing' section.

Created attachment 333426 [details] [review]
vm-configurator: Ensure WebDAV channel existence

We have to make sure that the WebDAV channel always exists in the
machine's XML configuration file, for the planned shared folders
feature to work on local machines.

Created attachment 333427 [details] [review]
spice-display: Implement folder sharing

Let the user enable and choose a shared folder between the host
machine and a local or remote machine with SPICE display.

This feature requires the WebDAV channel to be present in the
guest's configuration, which was ensured for local machines in
a previous patch. It also requires spice-webdavd to be installed
in the guest machine, which will be ensured for local machines
in following patches to libosinfo and spice-nsis.

About the segmentation fault I receive when setting the session.shared_dir to null:
* This happens when trying to access the null shared folder from inside the guest machine.
* It's very ugly and renders my host machine useless (fedora 24)
* I can't get the backtrace because I am no longer able to type in the terminal, but I manually copied this message on my cellphone:

Thread 1 "gnome-boxes" received signal SIGSEGV, Segmentation fault.
soup_server_accept_socket (server=server@entry=0x0, sock=sock@entry=0x199f2c0)
at /home/alex/spice/libsoup/libsoup/soup-server.c
1461 priv->clients = g_slist_prepend (priv->clients, client);

Pasting messages from IRC in case they are useful here:

18:06 < teuf> visarion: you can't type because keyboard input does not get through?
18:06 < teuf> if that's because of this that you can't type, you've got 2 ways of avoiding this
18:06 < teuf> 1st is to set SPICE_NOGRAB in your environment
18:07 < teuf> the other would be to run gdb in screen/tmux
18:07 < teuf> and switch to a VT (ctrl + alt + fN) when the crash happens
18:07 < teuf> and get back your gdb session from there
18:07 < teuf> as soon as you kill boxes, you'll get back the keyboard I think

Review of attachment 331619 [details] [review]:

::: src/spice-display.vala
@@ +318,3 @@
+                    channel.connect ();
+                else
+        foreach (var channel in channel_list) {

Assuming for sake of arguments, these really are workarounds, the solution to avoid workarounds is not to treat them as solutions. The string should be null everywhere but if it can't be (in other components), it's not at all complicated to convert between "" and null. Using "" as unset string is weird.

The crash should have been fixed in SPICE.

Review of attachment 333423 [details] [review]:

::: src/spice-display.vala
@@ +338,3 @@
                 var devs = get_usb_devices (manager);
 
+                if (devs.length > 0 && connected) {

Large blocks of code are ugly already and the more indented (nested) they are, the uglier they get. I'd rather you move the code to another function and only indent the call. Besides, "Refactor" in the commit message implies a clean-up and this is quite the opposite of clean-up.

Having said that, a lot of this code will be removed hopefully soon after I finish my work on big properties code clean-up and move all properties UI setup to .ui files. It's in branch wip/props-ui-files if you are interested to see.

Having said that (#2), if you finish act on the first para before I'm done with my branch, you'll be the winner and I'll have to rebase. :)

Review of attachment 333424 [details] [review]:

::: src/spice-display.vala
@@ +402,3 @@
             } catch (GLib.Error error) {
             }
+

After the previous review, I see that you are making an already pretty huge function even bigger. Could you please put all this code in a separate function?

Review of attachment 333425 [details] [review]:

ack

Review of attachment 333426 [details] [review]:

ack

Review of attachment 333427 [details] [review]:

::: src/spice-display.vala
@@ +265,3 @@
+        if (channel is Spice.WebdavChannel) {
+            shared_folder_id = notify["shared-folder"].connect ( () => {
+                if (shared_folder != "") {

still not sure about using "" as null.

@@ +637,3 @@
 
+            if (channel is Spice.WebdavChannel && display.shared_folder != "")
+                channel.open_fd (-1);

* why do we need to explicitly open this channel ourselves?

* IMO it will be more obvious what's going on if you just pass it our fd directly, or just call on_open_fd() yourself directly here.

Created attachment 341117 [details] [review]
vm-configurator: Ensure WebDAV channel existence

We have to make sure that the WebDAV channel always exists in the
machine's XML configuration file, for the planned shared folders
feature to work on local machines.

Review of attachment 341117 [details] [review]:

was accepted, but I altered it a little because of the "vm-configurator: Don't access invalid device objects" previous patch

Created attachment 341118 [details] [review]
props-page-widget: 'Devices' -> 'Devices & Shares'

Change the 'DEVICES' page name to 'Devices & Shares', in order to
accomodate the new 'Folder Sharing' section.

Review of attachment 341118 [details] [review]:

was accepted

Created attachment 341119 [details] [review]
spice-display: Reorganize 'USB Devices' property

The current way the code is organized doesn't allow for another
section to be further added after the 'USB Devices' section.

Based on the folder sharing UI mockups, we will add another
'Folder Sharing' section after 'USB Devices', so we need
some code reorganizing.

Created attachment 341120 [details] [review]
Add SharedFolderPopover

Let's add a custom popover to aid us when adding/editing
shared folders.

Created attachment 341121 [details] [review]
spice-display: Add UI for 'Folder Shares' section

Let's add the main UI elements, to support the upcoming patches
which will implement multiple shared folders.

Created attachment 341122 [details] [review]
spice-display: Implement multiple shared folders

Create a symlink on the host for every folder the user wants to
share, inside a temporary directory.
Share the temporary directory with the guest.

Automatically share by default the "Public" directory.

*** Bug 776230 has been marked as a duplicate of this bug. ***

Created attachment 342835 [details]
Screenshot of popup appearing in the wrong place

Looks good but two issues I noticed, this one being the major one: popup appears in a wrong place.

Created attachment 342836 [details]
Spacing issue screenshot

This is a minor issue to me but might be very major to designers. :) At least to me it seems there is way too much space between the rows. Did you show the screenshot to designers?

(In reply to Zeeshan Ali (Khattak) from comment #70)
> Created attachment 342835 [details]
> Screenshot of popup appearing in the wrong place
> 
> Looks good but two issues I noticed, this one being the major one: popup
> appears in a wrong place.

This is the first time I see this issue. Which Gtk version do you have and does this happen every time you use the popover ?

(In reply to Zeeshan Ali (Khattak) from comment #71)
> Created attachment 342836 [details]
> Spacing issue screenshot
> 
> This is a minor issue to me but might be very major to designers. :) At
> least to me it seems there is way too much space between the rows. Did you
> show the screenshot to designers?

I remember I did, but I'll do it again.

(In reply to Visarion Alexandru from comment #72)
> (In reply to Zeeshan Ali (Khattak) from comment #70)
> > Created attachment 342835 [details]
> > Screenshot of popup appearing in the wrong place
> > 
> > Looks good but two issues I noticed, this one being the major one: popup
> > appears in a wrong place.
> 
> This is the first time I see this issue. Which Gtk version do you have and
> does this happen every time you use the popover ?

$ pkg-config --modversion gtk+-3.0 
3.22.6

It should git master actually. I'll update my jhbuild to ensure but please you also do the same.

(In reply to Visarion Alexandru from comment #72)
> (In reply to Zeeshan Ali (Khattak) from comment #70)
> > Created attachment 342835 [details]
> > Screenshot of popup appearing in the wrong place
> > 
> > Looks good but two issues I noticed, this one being the major one: popup
> > appears in a wrong place.
> 
> This is the first time I see this issue. Which Gtk version do you have and
> does this happen every time you use the popover ?

And yes, it happens everytime.

(In reply to Zeeshan Ali (Khattak) from comment #75)
> (In reply to Visarion Alexandru from comment #72)
> > (In reply to Zeeshan Ali (Khattak) from comment #70)
> > > Created attachment 342835 [details]
> > > Screenshot of popup appearing in the wrong place
> > > 
> > > Looks good but two issues I noticed, this one being the major one: popup
> > > appears in a wrong place.
> > 
> > This is the first time I see this issue. Which Gtk version do you have and
> > does this happen every time you use the popover ?
> 
> And yes, it happens everytime.

This is most likely a gtk bug. The popup is an internal mechanism of the Gtk.FileChooserButton.

I talked with the guys from gtk and they told me that "there are quite a few bugs open about popup placement"

Also, I updated my gtk. Now,
<jhbuild -f ~/spice/spice-jhbuild/jhbuildrc run pkg-config --modversion gtk+-3.0>
returns 3.22.6, but I still can't reproduce the issue.

(In reply to Zeeshan Ali (Khattak) from comment #71)
> Created attachment 342836 [details]
> Spacing issue screenshot
> 
> This is a minor issue to me but might be very major to designers. :) At
> least to me it seems there is way too much space between the rows. Did you
> show the screenshot to designers?

I talked to jimmac, concerned about the spacing between the rows.
He told me that the '+' button will need a separator above and that the whole list would be nice to have top and bottom borders, so I will try doing that.

(In reply to Visarion Alexandru from comment #76)
> (In reply to Zeeshan Ali (Khattak) from comment #75)
> > (In reply to Visarion Alexandru from comment #72)
> > > (In reply to Zeeshan Ali (Khattak) from comment #70)
> > > > Created attachment 342835 [details]
> > > > Screenshot of popup appearing in the wrong place
> > > > 
> > > > Looks good but two issues I noticed, this one being the major one: popup
> > > > appears in a wrong place.
> > > 
> > > This is the first time I see this issue. Which Gtk version do you have and
> > > does this happen every time you use the popover ?
> > 
> > And yes, it happens everytime.
> 
> This is most likely a gtk bug. The popup is an internal mechanism of the
> Gtk.FileChooserButton.
> 
> I talked with the guys from gtk and they told me that "there are quite a few
> bugs open about popup placement"
> 
> Also, I updated my gtk. Now,
> <jhbuild -f ~/spice/spice-jhbuild/jhbuildrc run pkg-config --modversion
> gtk+-3.0>
> returns 3.22.6, but I still can't reproduce the issue.


I just updated from fedora 24 to fedora 25 and now it also happens to me.

There were two major issues. The one you noticed and another one I noticed (Gtk.FileChooserButton would seg fault when shown without an initial directory).
Both were major internal bugs from Gtk+ and now seem to have been solved by them.

Could you confirm that these are gone, please ?

Also, now I notice a new, different issue, probably from Gtk+ master. If I show the shared-folder popover and I close the properties and re-open them, the system logs out with no information (not even gdb helps me here) at the point where the shared-folder frame is added to the list.

I'll try to figure out what's happening and discuss with the Gtk+ folks.

Created attachment 344046 [details] [review]
vm-configurator: Ensure WebDAV channel existence

We have to make sure that the WebDAV channel always exists in the
machine's XML configuration file, for the planned shared folders
feature to work on local machines.

Created attachment 344047 [details] [review]
props-page-widget: 'Devices' -> 'Devices & Shares'

Change the 'DEVICES' page name to 'Devices & Shares', in order to
accomodate the new 'Folder Sharing' section.

Created attachment 344048 [details] [review]
spice-display: Reorganize 'USB Devices' property

The current way the code is organized doesn't allow for another
section to be further added after the 'USB Devices' section.

Based on the folder sharing UI mockups, we will add another
'Folder Sharing' section after 'USB Devices', so we need
some code reorganizing.

Created attachment 344049 [details] [review]
Add SharedFolderPopover

Let's add a custom popover to aid us when adding/editing
shared folders.

Created attachment 344050 [details] [review]
spice-display: Add UI for 'Folder Shares' section

Let's add the main UI elements, to support the upcoming patches
which will implement multiple shared folders.

Created attachment 344051 [details] [review]
spice-display: Implement multiple shared folders

Create a symlink on the host for every folder the user wants to
share, inside a temporary directory.
Share the temporary directory with the guest.

Automatically share by default the "Public" directory.

Created attachment 344140 [details] [review]
vm-configurator: Ensure WebDAV channel existence

We have to make sure that the WebDAV channel always exists in the
machine's XML configuration file, for the planned shared folders
feature to work on local machines.

Created attachment 344141 [details] [review]
props-page-widget: 'Devices' -> 'Devices & Shares'

Change the 'DEVICES' page name to 'Devices & Shares', in order to
accomodate the new 'Folder Sharing' section.

Created attachment 344142 [details] [review]
spice-display: Reorganize 'USB Devices' property

The current way the code is organized doesn't allow for another
section to be further added after the 'USB Devices' section.

Based on the folder sharing UI mockups, we will add another
'Folder Sharing' section after 'USB Devices', so we need
some code reorganizing.

Created attachment 344143 [details] [review]
Add SharedFolderPopover

Let's add a custom popover to aid us when adding/editing
shared folders.

Created attachment 344144 [details] [review]
spice-display: Add UI for 'Folder Shares' section

Let's add the main UI elements, to support the upcoming patches
which will implement multiple shared folders.

Created attachment 344145 [details] [review]
spice-display: Implement multiple shared folders

Create a symlink on the host for every folder the user wants to
share, inside a temporary directory.
Share the temporary directory with the guest.

Automatically share by default the "Public" directory.

Review of attachment 344140 [details] [review]:

It seems pretty straightforward. Thanks!

Marking as reviewed because I would like to get them all in together. ;-)

::: src/vm-configurator.vala
@@ +269,3 @@
+            else if (device is DomainChannel) {
+                var device_channel = device as DomainChannel;
+                if (device_channel.get_target_name () == "org.spice-space.webdav.0")

nitpick: this constant could be defined just once in the code, to make it simpler in case it changes. I'd call it WEBDAV_CHANNEL_URI or something in these lines.

Review of attachment 344141 [details] [review]:

sure.

Review of attachment 344142 [details] [review]:

Not a big fan of the whole UI bits to be mixed with the logic instead of widget templates, but since that's the way it was, lets keep it this way for now.

::: src/spice-display.vala
@@ +338,2 @@
             try {
+                    var manager = UsbDeviceManager.get (session);

The indentation here.

@@ +342,1 @@
+                    if (connected && devs.length > 0) {

I wouldn't change the connected guard conditional without a good reason. Besides, it should _break_ before you get the UsdDeviceManager and run get_usb_devices.

@@ +449,3 @@
+                    manager.connect_device_async.begin (dev, null, (obj, res) => {
+                        try {
+            hbox.margin_bottom = 6;

indentation.

Review of attachment 344143 [details] [review]:

::: data/gnome-boxes.gresource.xml
@@ +35,3 @@
     <file preprocess="xml-stripblanks">ui/snapshot-list-row.ui</file>
     <file preprocess="xml-stripblanks">ui/troubleshoot-log.ui</file>
+    <file preprocess="xml-stripblanks">ui/shared-folder-popover.ui</file>

I know some of them are not on alphabetical order, but it would be nice. :)

::: data/ui/shared-folder-popover.ui
@@ +6,3 @@
+    <property name="can_focus">False</property>
+    <property name="modal">True</property>
+    <property name="position">0</property>

<property name="position">bottom</property>

@@ +95,3 @@
+                <property name="homogeneous">True</property>
+                <child>
+                  <object class="GtkButton" id="button_cancel">

Connect it to a [GtkCallback] handler and you don't need to instantiate it on the vala code neither an id.

<signal name="clicked" handler="on_cancel_button_clicked"/>

@@ +109,3 @@
+                <child>
+                  <object class="GtkButton" id="button_save">
+                    <property name="label" translatable="yes">Save</property>

ditto.

::: src/shared-folder-popover.vala
@@ +14,3 @@
+    private Gtk.Button button_cancel;
+    [GtkChild]
+    private Gtk.Button button_save;

No need for these two if you connect the signals in the .ui file.

@@ +16,3 @@
+    private Gtk.Button button_save;
+
+    public int target_position;

Is there an interest of having an specific sorting in the list? If not, we could just "append". In doing so, there's no need to keep track of target_position.

@@ +18,3 @@
+    public int target_position;
+    public string uri { set { file_chooser_button.set_uri ("file://" + value); } }
+    public string folder_name { set { name_entry.set_text (value); } }

You could "bind_property" between the uri and folder_name properties or drop them entirely and make the getters and setters of the GtkWidgets handle the mapping.

@@ +42,3 @@
+
+    public void clean_up () {
+        this.relative_to = null;

The name_entry and the file_chooser_button would still keep their previous values.

Review of attachment 344144 [details] [review]:

For this one I would propose a significantly different patch. (See below).

::: data/gtk-style.css
@@ +123,3 @@
 /* Adds a border to the ISOs lists top undershoot */
 .boxes-menu-scrolled.undershoot.top { border-top: 1px solid @borders; }
+

not really necessary (all these css).

::: src/spice-display.vala
@@ +480,3 @@
+        frame.add (listbox);
+
+        var row_plus = new Gtk.ListBoxRow ();

still not a big fan of gtk/ui specific code mixed with code logic.

@@ +496,3 @@
+        button_plus.set_image (image_plus);
+        hbox_plus.pack_start (button_plus, true, true, 0);
+        row_plus.add (hbox_plus);

Wouldn't it be easier if row_plus wouldn't be part of the list box?

@@ +671,3 @@
 }
+
+private class Boxes.SharedFolderRow : Gtk.ListBoxRow {

If this object would have its UI specifics defined in a template file, you could directly bind the properties to their respective UI widgets.

Created attachment 351042 [details] [review]
spice-display: Add UI for 'Folder Shares' section

Let's add the main UI elements, to support the upcoming patches
which will implement multiple shared folders.

Review of attachment 344145 [details] [review]:

::: src/spice-display.vala
@@ +70,3 @@
         }
+        var folder_name = "boxes_shared_" + "%u".printf (GLib.Random.next_int ());
+        shared_folder = GLib.Path.build_filename (GLib.Environment.get_tmp_dir (), folder_name);

Do we want it to be volatile by living in the /tmp dir?

Also, storing the "Names" in the directory name might cause restrictions according to folder-name-sizes, valid chars, spaces...

@@ +586,3 @@
         });
 
+        var hash = get_shared_folders ();

By calling this here wouldn't it load the folders just once?

Thanks for the patches, and I'm sorry for taking so long to review them.

Please, let me know if you don't have time to follow up in these reviews. In this case I would take it from where you stopped.

(In reply to Felipe Borges from comment #100)
> Thanks for the patches, and I'm sorry for taking so long to review them.
> 
> Please, let me know if you don't have time to follow up in these reviews. In
> this case I would take it from where you stopped.

Thanks for the reviews, I will follow up as soon as possible.

Review of attachment 344142 [details] [review]:

::: src/spice-display.vala
@@ +342,1 @@
+                    if (connected && devs.length > 0) {

I removed the lines

>if (!connected)
>    break;

because otherwise we don't have the chance the get to the "Shared Folders" frame, which will be located after "USB devices". It would skip it, just because the spice-agent isn't connected, but the shared folders work even without the spice-agent. We want to only skip the "USB devices" in case the spice-agent is not connected.
Basically, the reason is to make room for the "Shared Folders" Frame.

Review of attachment 344143 [details] [review]:

::: src/shared-folder-popover.vala
@@ +16,3 @@
+    private Gtk.Button button_save;
+
+    public int target_position;

Actually, yes. When we edit already existing entries, we want to know the position of the entry being updated.

Review of attachment 344145 [details] [review]:

::: src/spice-display.vala
@@ +70,3 @@
         }
+        var folder_name = "boxes_shared_" + "%u".printf (GLib.Random.next_int ());
+        shared_folder = GLib.Path.build_filename (GLib.Environment.get_tmp_dir (), folder_name);

I think it's a good idea to be volatile, because it will end up containing a lot of useless directories filled with symlinks. We can not clean up these directories because we don't know how the SPICE session ends, so we should at least let the system clean them for us.

I am not sure what "Names" you are referring to. The names of the volatile directories containing the symlinks will have the pattern "boxes_shared_*random int*".

@@ +586,3 @@
         });
 
+        var hash = get_shared_folders ();

The folders are loaded into the UI each time the user opens the "Properties" Page.

(In reply to Visarion Alexandru from comment #102)
> Review of attachment 344142 [details] [review] [review]:
> 
> ::: src/spice-display.vala
> @@ +342,1 @@
> +                    if (connected && devs.length > 0) {
> 
> I removed the lines
> 
> >if (!connected)
> >    break;
> 
> because otherwise we don't have the chance the get to the "Shared Folders"
> frame, which will be located after "USB devices". It would skip it, just
> because the spice-agent isn't connected, but the shared folders work even
> without the spice-agent. We want to only skip the "USB devices" in case the
> spice-agent is not connected.
> Basically, the reason is to make room for the "Shared Folders" Frame.

sure.

(In reply to Visarion Alexandru from comment #103)
> Review of attachment 344143 [details] [review] [review]:
> 
> ::: src/shared-folder-popover.vala
> @@ +16,3 @@
> +    private Gtk.Button button_save;
> +
> +    public int target_position;
> 
> Actually, yes. When we edit already existing entries, we want to know the
> position of the entry being updated.

Ok. I'd rather bind the view to a model, but I would accept this as it is and patch it later. No worries.

(In reply to Visarion Alexandru from comment #104)
> Review of attachment 344145 [details] [review] [review]:
> 
> ::: src/spice-display.vala
> @@ +70,3 @@
>          }
> +        var folder_name = "boxes_shared_" + "%u".printf
> (GLib.Random.next_int ());
> +        shared_folder = GLib.Path.build_filename
> (GLib.Environment.get_tmp_dir (), folder_name);
> 
> I think it's a good idea to be volatile, because it will end up containing a
> lot of useless directories filled with symlinks. We can not clean up these
> directories because we don't know how the SPICE session ends, so we should
> at least let the system clean them for us.

Hmm, I am not sure about it. I will think and discuss the topic and get back here.

Thanks for your work. I am looking forward for your patches!

Created attachment 351923 [details] [review]
vm-configurator: Ensure WebDAV channel existence

We have to make sure that the WebDAV channel always exists in the
machine's XML configuration file, for the planned shared folders
feature to work on local machines.

Created attachment 351924 [details] [review]
props-page-widget: 'Devices' -> 'Devices & Shares'

Change the 'DEVICES' page name to 'Devices & Shares', in order to
accomodate the new 'Folder Sharing' section.

Created attachment 351925 [details] [review]
spice-display: Reorganize 'USB Devices' property

The current way the code is organized doesn't allow for another
section to be further added after the 'USB Devices' section.

Based on the folder sharing UI mockups, we will add another
'Folder Sharing' section after 'USB Devices', so we need
some code reorganizing.

Created attachment 351926 [details] [review]
GResource: Order files alphabetically

Created attachment 351927 [details] [review]
Add SharedFolderPopover

Let's add a custom popover to aid us when adding/editing
shared folders.

Created attachment 351928 [details] [review]
spice-display: Add UI for 'Folder Shares' section

Let's add the main UI elements, to support the upcoming patches
which will implement multiple shared folders.

Created attachment 351929 [details] [review]
spice-display: Implement multiple shared folders

Create a symlink on the host for every folder the user wants to
share, inside a temporary directory.
Share the temporary directory with the guest.

Automatically share by default the "Public" directory.

Just to summarize what we have discussed on IRC:

1. We want the folders to be permanent (not on /tmp)
2. Their names and path mapping could be stored as a GSetting
3. If there's no "name" set, use the basename of the folder

Thanks for your work!

Created attachment 353067 [details] [review]
vm-configurator: Ensure WebDAV channel existence

We have to make sure that the WebDAV channel always exists in the
machine's XML configuration file, for the planned shared folders
feature to work on local machines.

Created attachment 353068 [details] [review]
props-page-widget: 'Devices' -> 'Devices & Shares'

Change the 'DEVICES' page name to 'Devices & Shares', in order to
accomodate the new 'Folder Sharing' section.

Created attachment 353069 [details] [review]
spice-display: Reorganize 'USB Devices' property

The current way the code is organized doesn't allow for another
section to be further added after the 'USB Devices' section.

Based on the folder sharing UI mockups, we will add another
'Folder Sharing' section after 'USB Devices', so we need
some code reorganizing.

Created attachment 353070 [details] [review]
GResource: Order files alphabetically

Created attachment 353071 [details] [review]
Add SharedFolderPopover

Let's add a custom popover to aid us when adding/editing
shared folders.

Created attachment 353072 [details] [review]
spice-display: Add UI for 'Folder Shares' section

Let's add the main UI elements, to support the upcoming patches
which will implement multiple shared folders.

Created attachment 353073 [details] [review]
spice-display: Implement multiple shared folders

Create a symlink on the host for every folder the user wants to
share, inside a config directory.
Share the config directory with the guest.

Automatically share by default the "Public" directory.

Created attachment 353074 [details] [review]
spice-display: Store shared folders using GSettings

Let's assure the persistance of shared folders, by storing
their names and path mapping for each SPICE display.

(In reply to Felipe Borges from comment #113)
> Just to summarize what we have discussed on IRC:
> 
> 1. We want the folders to be permanent (not on /tmp)
> 2. Their names and path mapping could be stored as a GSetting
> 3. If there's no "name" set, use the basename of the folder

1 -> In "Implement multiple shared folders" patch, I changed to directory to the user default config directory.
2 -> The last patch.
3 -> Solved in "Add SharedFolderPopover",

No changes elsewhere.

Could you please rebase your changes?

Created attachment 353199 [details] [review]
vm-configurator: Ensure WebDAV channel existence

We have to make sure that the WebDAV channel always exists in the
machine's XML configuration file, for the planned shared folders
feature to work on local machines.

Created attachment 353200 [details] [review]
props-page-widget: 'Devices' -> 'Devices & Shares'

Change the 'DEVICES' page name to 'Devices & Shares', in order to
accomodate the new 'Folder Sharing' section.

Created attachment 353201 [details] [review]
spice-display: Reorganize 'USB Devices' property

The current way the code is organized doesn't allow for another
section to be further added after the 'USB Devices' section.

Based on the folder sharing UI mockups, we will add another
'Folder Sharing' section after 'USB Devices', so we need
some code reorganizing.

Created attachment 353202 [details] [review]
GResource: Order files alphabetically

Created attachment 353203 [details] [review]
Add SharedFolderPopover

Let's add a custom popover to aid us when adding/editing
shared folders.

Created attachment 353204 [details] [review]
spice-display: Add UI for 'Folder Shares' section

Let's add the main UI elements, to support the upcoming patches
which will implement multiple shared folders.

Created attachment 353205 [details] [review]
spice-display: Implement multiple shared folders

Create a symlink on the host for every folder the user wants to
share, inside a config directory.
Share the config directory with the guest.

Automatically share by default the "Public" directory.

Created attachment 353206 [details] [review]
spice-display: Store shared folders using GSettings

Let's assure the persistance of shared folders, by storing
their names and path mapping for each SPICE display.

*** Bug 783605 has been marked as a duplicate of this bug. ***

Review of attachment 353199 [details] [review]:

sure!

Review of attachment 353200 [details] [review]:

lgtm

Review of attachment 353201 [details] [review]:

sure

Review of attachment 353202 [details] [review]:

thx

Review of attachment 353203 [details] [review]:

::: src/shared-folder-popover.vala
@@ +24,3 @@
+
+        if (path != null) {
+            path.scanf ("file://%s", path);

maybe later we will want to support other prefixes. But that's good for now.

Review of attachment 353204 [details] [review]:

nice!

Review of attachment 353205 [details] [review]:

sure.

Review of attachment 353206 [details] [review]:

Good. I am happy that we can finally land this feature.

I would like people to test it so we can address any issues that might eventually pop up.

Thanks a lot for your contribution!

I am happy to finally land this!

Attachment 353199 [details] pushed as 164dd20 - vm-configurator: Ensure WebDAV channel existence
Attachment 353200 [details] pushed as 40dda4e - props-page-widget: 'Devices' -> 'Devices & Shares'
Attachment 353201 [details] pushed as 21b4ca6 - spice-display: Reorganize 'USB Devices' property
Attachment 353202 [details] pushed as 8d1495c - GResource: Order files alphabetically
Attachment 353203 [details] pushed as 20dfa5d - Add SharedFolderPopover
Attachment 353204 [details] pushed as c89f76f - spice-display: Add UI for 'Folder Shares' section
Attachment 353205 [details] pushed as 4f08ea1 - spice-display: Implement multiple shared folders
Attachment 353206 [details] pushed as 08f55da - spice-display: Store shared folders using GSettings