Thank you for your detailed comment! 2012/11/12 Marc-André Lureau <marcandre.lureau@xxxxxxxxx>: > Hi! > > > On Mon, Nov 5, 2012 at 10:01 AM, Dunrong Huang <riegamaths@xxxxxxxxx> wrote: >> >> These patches simply implement file drag-and-drop from client to guest. >> > > nice! > >> >> There are some TODOs needed to be done: >> * transfer multiple files and directories >> * check whether guest has enough space to store file >> * a progress bar that allows user to cancels file transfer, >> since we always pre-cache before sending file content, >> it has a little difficult to finish. >> >> Comments are welcome, please review! > > > Good start! I imagine there will be a lot more to do to support all the > cases. It brings lot of questions and remarks, so I will put there some of > my thoughts in random order: > > We shouldn't need the SpiceDisplay to copy a file nor gtk, so we should put > it on the glib / channel level. In general, channel functions are not Agree with you. > mapping the protocol 1-1, they implement a fair amount of logic so instead > of having two spice_main_dnd* functions, perhaps we could have only one > spice_main_dnd_files() that would deal with the long task and protocol. The > function could take a GError, in order to report to the user like you did in > spice_dnd_file_too_large(). Yes, this makes our code more simple and easily extensible. > > nautilus is an example of a generic file dnd gtk code, and it seems it uses > more than "text/plain" target list, I am not familiar with Gtk+ DnD support > yet, so more investigation needed. > > I am not convinced we need to limit file size now and especially at > spice-gtk level. However, I think it would be useful to announce the file > size to the guest agent. (in the future, the guest could ask or cancel > operation, during negotiation or later), and perhaps more information about > the dnd context (although we quite clearly don't want to implement a full > dnd protocol but rather just file dnd) > > I wonder if the initial request should also give the guest some more context > than just a filename. We might want to easily extend those informations in > the future, for example the file mime, a file list, date, attributes, drop > position etc.. Perhaps the dnd request should use some metadata format where > fields can easily be added. The request should probably have an id so we can > identify and control the operation during its lifetime, and be able to > process several dnd simultaneously in the future. > In VD_AGENT_DND_START message, file name and file size had been included. But as you said, more information is needed for future expansion. I will design a common struct to hold these information. > It's easy to use glib API for file operations from the start, it saves us > from some portability issues. Since we are in a UI context, it is a > requirement to use async IO operations too. Could you update the patches to > use those? This makes sense, I will fix it in v2. > > The DND_CHUNCK_SIZE should probably match VD_AGENT_MAX_DATA_SIZE, to avoid > some extra overhead when marshalling. Yes. > > We need to throttle or prioritize the agent queue if we start to fill it > with lot of data, and we shouldn't copy the whole file there but that can be > improved a bit later too. > > Later, we should consider compressing the files in tar.xz archive > (optionally). That could be part of the initial request. very meaningful!, this option will reduce many bandwidth. > > Please clean up the g_print or use SPICE_DEBUG for logging. will be fixed in v2. > > All of this is quite some work, and I don't know how much effort you want to > put into it. It would be nice to agree on a first iteration to get it I am very happy to implement this feature in spare time. And I have enough time to do it, :-) > upstream soonish. My shortlist would be: > > - the initial dnd spice protocol supports single file, and initial > request/reply > - the ongoing dnd operations can be identified and cancelled, at any time by > client or agent > - the protocol should be relatively easy to extend for the future use cases > - the implementation must do async io (using gio functions in spice-gtk) > - the initial spice client API could be as simple as a single exported > function like spice_main_dnd_files() for now imho > > Thanks a lot for working on this, it's really appreciated! > > -- > Marc-André Lureau -- Best Regards, Dunrong Huang _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel