GNOME Bugzilla – Bug 725328
Unbreak activating a search provider result
Last modified: 2014-02-28 07:35:24 UTC
Nothing happens if a search result is activated in gnome-shell.
Created attachment 270483 [details] [review] Unbreak activating a search provider result
Review of attachment 270483 [details] [review]: ::: src/photos-application.c @@ +187,2 @@ photos_application_create_window (self); + photos_base_manager_set_active_object (priv->item_mngr, item); I think window mode should explicitly be set to PHOTOS_WINDOW_MODE_NONE. ::: src/photos-embed.c @@ +159,3 @@ + view_container = NULL; + break; + Since now there are two cases that has view_container = NULL, we can move the case "PHOTOS_WINDOW_MODE_PREVIEW" up to this case (PHOTOS_WINDOW_MODE_NONE) and use switch's fall-through construct. This will make the code more elegant + easy to understand.
(In reply to comment #3) > Review of attachment 270483 [details] [review]: Thanks for the review, Pranav! > ::: src/photos-application.c > @@ +187,2 @@ > photos_application_create_window (self); > + photos_base_manager_set_active_object (priv->item_mngr, item); > > I think window mode should explicitly be set to PHOTOS_WINDOW_MODE_NONE. I guess you are suggesting so to stress the point that we are in NONE before moving to the PREVIEW. Thing is that NONE is our "NULL mode" -- we start from NONE and then move between the valid modes. Hence I don't want to explicitly set a NONE mode after the application has started. However, you are right. We can do better. We can rework the patch to avoid talking about NONE at all, which matches with our above rule. > ::: src/photos-embed.c > @@ +159,3 @@ > + view_container = NULL; > + break; > + > > Since now there are two cases that has view_container = NULL, we can move the > case "PHOTOS_WINDOW_MODE_PREVIEW" up to this case (PHOTOS_WINDOW_MODE_NONE) and > use switch's fall-through construct. Good point. Thinking about it a bit more, I decided to remove the cases for NONE and PREVIEW altogether, because we should never call get_view_container_from_mode in those cases. If we do it would be a coding mistake. I moved the call in item_load inside the if branch.
Created attachment 270495 [details] [review] Unbreak activating a search provider result
Looks great now !
Comment on attachment 270495 [details] [review] Unbreak activating a search provider result Thanks for the review.