Hi,
On Wed, Dec 12, 2018 at 06:43:04AM -0500, Frediano Ziglio wrote:
> > Documentation saying that this feature rely on agent seems to be
> > lacking. e.g: grep for 'agent' on SpiceGtkSession
> >
> > > As spice-gtk is a public library with an API speaking of "Spice
> > > client" is not a good thing, should be Spice clientS, meaning
> > > all possible clients and being ALL would mean even
> > > unknown/proprietary ones.
> >
> > Hm, I did not mention a specific client. My usage of 'Spice
> > client' is meant to be generic. I have no problem in changing
> > this to make you happy but I don't see why is wrong.
> >
>
> With your patches and remove-viewer (1 minutes or so):
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:27:52.344: agent_clipboard_grab: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:27:52.344: agent_clipboard_grab: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:28:17.152: agent_clipboard_release: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:28:17.153: agent_clipboard_grab: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:28:17.638: agent_clipboard_release: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:28:17.640: agent_clipboard_grab: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:28:30.751: agent_clipboard_release: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:28:30.752: agent_clipboard_grab: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:28:32.672: agent_clipboard_release: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:28:32.679: agent_clipboard_grab: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:28:39.305: agent_clipboard_release: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:28:39.307: agent_clipboard_grab: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:28:40.929: agent_clipboard_release: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:28:40.931: agent_clipboard_grab: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:28:45.805: agent_clipboard_release: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:28:45.806: agent_clipboard_grab: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:28:48.843: agent_clipboard_release: assertion 'c->agent_connected' failed
>
> (remote-viewer:11191): GSpice-CRITICAL **: 11:28:48.847: agent_clipboard_grab: assertion 'c->agent_connected' failed
Thanks! This is exactly what should be fixed (I mean, I still
think my patches aren't wrong ;] ) ~ This also shows that my test
was a bit bad.
I have fix locally, I hope to send the patches before closing the
laptop today.
> > > As all clients should respect API documentation you are allow to change
> > > API behaviour where the behaviour was documented as not defined (for
> > > instance free() behaviour was changed in respect to NULL from undefined
> > > behaviour to ignored).
> >
> > Because I'm verbose on commit log, doesn't mean that I changed
> > the behavior? If any function dealing with clipboard is handled
> > while the agent is disconnected, that's bug and some
> > warning/critical should be hit. I'm making it explicit in
> > agent_clipboard_*() functions.
> >
>
> not talking about commit log, the status check.
> Is perfectly fine if the check is done in the functions, for
> instance has the advantage to be shared between all possible
> clients so you don't have to keep the state consistent for all
> possible operations. I think is pretty normal. That's the same
> reason free() is accepting now NULL ignoring it, remove the
> effort to have a "if (p) free(p)" in a lot of places.
>
> > > At least we should check that the "expected" clients (remote-viewer,
> > > virt-viewer, virt-manager I would say) already have this behaviour
> > > before changing API behaviour.
> >
> > SpiceGtkSession's clipboard handling is automatic if
> > auto-clipboard property set or when it is false, by using
> > spice_gtk_session_paste_from_guest() and
> > spice_gtk_session_copy_to_guest() functions.
> >
> > "auto-clipboard" is preferred by gnome-boxes, virt-viewer,
> > virt-manager. Spicy **testing tool** has menu to use both
> > functions above but they are disabled when agent is disconnected.
> >
>
> Don't get these comments.
Just that there are two ways to achieve the clipboard
functionality, setting SpiceGtkSession::auto-clipboard to true is
the preferred way. See:
https://www.spice-space.org/api/spice-gtk/SpiceGtkSession.html#SpiceGtkSession--auto-clipboard
>
> > > Is only a warning change but does not help if the change is
> > > producing hundred of warnings which would hide any possible
> > > issue raised by warnings, not having the agent running is not
> > > an impossible use case.
> >
> > The warning would happen anyway. It should happen. I agree with
> > lack of documentation.
> >
>
> Without these patches remote-viewer does not throw any message
> to me and IMHO not having the agent running is something that
> should be supported and not throw messages continuously.
Yes, thanks again for testing.
> As a user I would prefer a GUI message once stating that the
> client is not able to do clipboard at the moment as cannot
> detect the agent running. This will help the user to fix the
> issue.
I agree but the only way to do that today is for the clients to
connect to SpiceMainChannel::agent-connected and provide some
info. That should be enough I think, but
> Also I think that a "critical" is a bit strong as they
> are used also for programming error.
My intention is to have critical as programming error indeed.
I hope that my next version clarify that.
Cheers,
Victor
> > Cheers,
> >
> > >
> > > > As always, thanks for reviewing.
> > > >
> > > > >
> > > > > > ---
> > > > > > src/channel-main.c | 4 +---
> > > > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/src/channel-main.c b/src/channel-main.c
> > > > > > index 223043a..aa563d2 100644
> > > > > > --- a/src/channel-main.c
> > > > > > +++ b/src/channel-main.c
> > > > > > @@ -1352,9 +1352,7 @@ static bool
> > > > > > agent_clipboard_grab(SpiceMainChannel
> > > > > > *channel, guint selection,
> > > > > > size_t size;
> > > > > > int i;
> > > > > >
> > > > > > - if (!c->agent_connected)
> > > > > > - return false;
> > > > > > -
> > > > > > + g_return_val_if_fail(c->agent_connected, false);
> > > > > > g_return_val_if_fail(test_agent_cap(channel,
> > > > > > VD_AGENT_CAP_CLIPBOARD_BY_DEMAND), false);
> > > > > >
> > > > > > size = sizeof(VDAgentClipboardGrab) + sizeof(uint32_t) * ntypes;
> > > > >
> > > > > Frediano
> > > >
> >
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel