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.lur...@redhat.com> wrote: > > Hi > > > > On Fri, Aug 31, 2018 at 8:32 AM, Victor Toso <victort...@redhat.com> wrote: > >> Hi, > >> > >> On Tue, Jul 31, 2018 at 03:41:09PM +0200, marcandre.lur...@redhat.com > >> wrote: > >>> From: Marc-André Lureau <marcandre.lur...@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.lur...@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
signature.asc
Description: PGP signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@redhat.com https://www.redhat.com/mailman/listinfo/virt-tools-list
