[Spice-devel] [spice-gtk v1 1/4] gtk-session: remove extra clipboard selection check

[Victor Toso]

Hi,

On Thu, Dec 06, 2018 at 04:30:17AM -0500, Frediano Ziglio wrote:
> > Hi,
> > 
> > On Thu, Dec 06, 2018 at 03:54:41AM -0500, Frediano Ziglio wrote:
> > > > 
> > > > From: Victor Toso <me at victortoso.com>
> > > > 
> > > > Commit 284c1f2d switched to
> > > > spice_main_channel_clipboard_selection_release() which does check if
> > > > agent is connected and does the right thing (expected) in regards to
> > > > releasing the clipboard by calling agent_clipboard_release() which
> > > > does check VD_AGENT_CAP_CLIPBOARD_SELECTION (like current removed
> > > > code).
> > > > 
> > > > So this patch removes redundant check.
> > > > 
> > > > Same goes for spice_main_channel_clipboard_selection_grab() function.
> > > > 
> > > > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > > 
> > > This is not the same. The test in agent_clipboard_grab is done
> > > with g_return_if_fail which emits a warning while current code
> > > does not.
> > 
> > Which probably should
> > 
> 
> IMHO is the opposite, as we supports agents that does not
> support VD_AGENT_CAP_CLIPBOARD_BY_DEMAND why giving warning
> every time?

Hm, that was added in 2010 [0], spice-vdagent-0.6.3. Why should
we support supper earlier versions of the agent anyway?

https://gitlab.freedesktop.org/spice/linux/vd_agent/commit/653a2ac191dd699de7e9dd322e4b#cbc5e2eb746c0c8aaa6d4629e43dfb9da6b0c945_94_67

> If we would like to warn the user that guest is using a very old
> agent would not be better to warn once when the capabilities are
> received? Maybe if it's really important this could be also done
> with a GUI message (maybe with a persistent tick "don't warn
> about it next time" like for the exit message).

I'm pretty sure nobody tests such old agent... I'm in favor of
warning as it should help find bugs instead of silently not doing
it like current code.

> > > Also spice_main_channel_clipboard_selection_grab always calls
> > > spice_channel_wakeup which current code does not.
> > 
> > Which is probably a bug there (should call spice_channel_wakeup
> > if message was queued.
> > 
> > What do you think?
> > 
> 
> Not sure if this function is also expected to handle pending
> events and what would happen if this is removed.

No, the wakeup is after adding the message to the queue with the
purpose of not delaying that message to be sent. Some code in
client could block while waiting the clipboard request and that's
why the wakeup can be helpful.

> If it's safe to do so then I would move spice_channel_wakeup
> call inside spice_main_channel_clipboard_selection_grab at
> the end of agent_clipboard_grab (which is only called by
> spice_main_channel_clipboard_selection_grab).

I would rather make agent_clipboard_* to return true if
succeed in adding the message to the queue. That's implementation
detail, We/I can look later on.. Important is that we agree
wakeup shouldn't be called on failure.

> > > > ---
> > > >  src/spice-gtk-session.c | 7 ++-----
> > > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > > index 20a4060..8d31045 100644
> > > > --- a/src/spice-gtk-session.c
> > > > +++ b/src/spice-gtk-session.c
> > > > @@ -610,9 +610,7 @@ static void clipboard_get_targets(GtkClipboard
> > > > *clipboard,
> > > >      }
> > > >  
> > > >      s->clip_grabbed[selection] = TRUE;
> > > > -
> > > > -    if (spice_main_channel_agent_test_capability(s->main,
> > > > VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> > > > -        spice_main_channel_clipboard_selection_grab(s->main, selection,
> > > > types, num_types);
> > > > +    spice_main_channel_clipboard_selection_grab(s->main, selection,
> > > > types,
> > > > num_types);
> > > >  
> > > >      /* Sending a grab causes the agent to do an implicit release */
> > > >      s->nclip_targets[selection] = 0;
> > > > @@ -636,8 +634,7 @@ static void clipboard_owner_change(GtkClipboard
> > > > *clipboard,
> > > >  
> > > >      if (s->clip_grabbed[selection]) {
> > > >          s->clip_grabbed[selection] = FALSE;
> > > > -        if (spice_main_channel_agent_test_capability(s->main,
> > > > VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> > > > -            spice_main_channel_clipboard_selection_release(s->main,
> > > > selection);
> > > > +        spice_main_channel_clipboard_selection_release(s->main,
> > > > selection);
> > > >      }
> > > >  
> > > >      switch (event->reason) {
> > > 
> > > Frediano

Thanks again :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20181206/0188addb/attachment.sig>