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

[Frediano Ziglio]

> 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?
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).

> > 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.
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).

> > > ---
> > >  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
>