[virt-tools-list] [PATCH 06/22] window: do not reset sensitivity of menu-send in rebuild

[Victor Toso]

Hi,

On Fri, Aug 31, 2018 at 01:18:31PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Aug 31, 2018 at 1:14 PM, Marc-André Lureau
> <marcandre.lureau at redhat.com> wrote:
> > Hi
> >
> > On Fri, Aug 31, 2018 at 8:32 AM, Victor Toso <victortoso at redhat.com> wrote:
> >> Hi,
> >>
> >> On Tue, Jul 31, 2018 at 03:41:09PM +0200, marcandre.lureau at redhat.com wrote:
> >>> From: Marc-André Lureau <marcandre.lureau at redhat.com>
> >>>
> >>> A mere app::enable-accel notification shouldn't modify the
> >>> sensitivity, which is handled for all menu items elsewhere, leave the
> >>> current state untouched.
> >>>
> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> >>
> >> Not sure if this could introduce regression around
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1152574
> >>
> >> Before, the code was always setting sensitive to FALSE on
> >> app:enable-accel. With Jonathon's commit 65560fa4664e, it was
> >> changed to take in consideration if display existed.
> >>
> >> Not sure how to test.
> >>
> >
> > I lack arguments on why this patch was needed, let's drop it for now.
> > I'll add it back when I remember or can better explain the reason.
> >
> > thanks
> >
> 
> Ah, looking at the following patch helps a bit.
> 
> The sensitivy of "menu-send" is getting more complex. I tried
> to have a single place to put the logic.

Right.

> So rebuild_combo_menu() is called in 2 cases:
> 1. notify::enable-accel: there is no need to update the
> sensitivy of "menu-send", ok
> 2. on construction: default to false since display == NULL. It will be
> later updated when virt_viewer_window_set_menus_sensitive()
>
> I think the correct patch is to move gtk_widget_set_sensitive(menu,
> FALSE) in virt_viewer_window_constructed.
> 
> Does it make sense? I'll updatre the patch.

Yes, it does make sense to have the sensitive set to FALSE on construct
while setting it to TRUE when we already have a VirtViewerDisplay
on virt_viewer_window_set_menus_sensitive() but I take that is
already the case with this patch, no?

I tested with your previous patch that set the sensitive to FALSE
in the .ui instead of virt_viewer_window_init(). The worrisome
situation is when there is a problem with display channel and we
would like to send-key using the menu but I fail to see how
removing gtk_widget_set_sensitive() call from enable-accel
handler would affect it (and cause regression over rhbz#1152574).

I think this patch is fine as is, it might be good to add that
the intention is to reduce/concentrate the paths that change
menu's sensitive.

Victor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20180831/483bf469/attachment.sig>