GNOME Bugzilla – Bug 746367
Restore the search criteria when we move back from the preview or collection view to the search results
Last modified: 2015-05-07 13:24:16 UTC
To reproduce: 1) Type the name of a collection 2) Click the collection from the results 3) Click on an item to preview 4) Go back 2 steps to the list of search results 5) The search term is not restored
Created attachment 299637 [details] [review] Hide & restore search when moving to preview or collection view & back
Created attachment 299639 [details] [review] Don't stomp on the search criteria when going back from a collection
Created attachment 299640 [details] [review] embed: Workaround spurious signals from GtkStack and GtkStackSwitcher I will see if I can fix this in gtk+. Till then this is a workaround.
Review of attachment 299637 [details] [review]: ::: src/embed.js @@ +311,3 @@ + _restoreSearch: function() { + if (!this._searchState.saved) I think instead of having the saved variable, you could just have searchState = null to signify the same thing instead. @@ +323,3 @@ + _saveSearch: function() { + if (this._searchState.saved) + Application.sourceManager.setActiveItem(this._searchState.source); Will this not make it only work the first time or if _restoreSearch() has been called after _restoreSearch()? I would expect searchState to be updated with the current state every time I call this function. @@ +325,3 @@ + return; + + Application.searchController.setString(this._searchState.str); No reason to clear this now if you're basically discarding the object immediately after @@ +335,3 @@ _onActiveItemChanged: function(manager, doc) { + let windowMode = Application.modeController.getWindowMode(); + let showSearch = (windowMode == WindowMode.WindowMode.PREVIEW && !doc How can the app be in PREVIEW mode without an active doc?
Review of attachment 299639 [details] [review]: OK
Review of attachment 299640 [details] [review]: ::: src/embed.js @@ +91,3 @@ + // _onVisibleChildChanged from from being called while we're in + // an inconsistent state. + this._stack.disconnect(this._visibleChildChangedId); What happens in that case? The embed should be destroyed together with the stack, right?
(In reply to Cosimo Cecchi from comment #4) > @@ +335,3 @@ > _onActiveItemChanged: function(manager, doc) { > + let windowMode = Application.modeController.getWindowMode(); > + let showSearch = (windowMode == WindowMode.WindowMode.PREVIEW && > !doc > > How can the app be in PREVIEW mode without an active doc? Because when we go from PREVIEW to one of the overviews, we first unset the active doc and then change the mode. Therefore, when we receive this signal doc is null, but the mode is PREVIEW. Such inconsistencies during state changes is the reason why I want to merge ModeController with DocumentManager (just like we merged CollectionManager). That way we can emit the signals at well defined points and the invariants won't be violated irrespective of the order in which the rest of the code does things.
(In reply to Cosimo Cecchi from comment #6) > Review of attachment 299640 [details] [review] [review]: > > ::: src/embed.js > @@ +91,3 @@ > + // _onVisibleChildChanged from from being called while > we're in > + // an inconsistent state. > + this._stack.disconnect(this._visibleChildChangedId); > > What happens in that case? The embed should be destroyed together with the > stack, right? I don't know. I was not able to make it misbehave, but I do know that we would have problems during destroying the Embed object in C: https://git.gnome.org/browse/gnome-photos/tree/src/photos-embed.c#n560 I don't know the order in which things are destroyed in JavaScript. I added that part as a 'who-knows-just-in-case'.
Created attachment 301128 [details] [review] Hide & restore search when moving to preview or collection view & back
(In reply to Cosimo Cecchi from comment #4) > Review of attachment 299637 [details] [review] [review]: > > ::: src/embed.js > @@ +311,3 @@ > > + _restoreSearch: function() { > + if (!this._searchState.saved) > > I think instead of having the saved variable, you could just have > searchState = null to signify the same thing instead. Fixed. > @@ +323,3 @@ > + _saveSearch: function() { > + if (this._searchState.saved) > + Application.sourceManager.setActiveItem(this._searchState.source); > > Will this not make it only work the first time or if _restoreSearch() has > been called after _restoreSearch()? I would expect searchState to be updated > with the current state every time I call this function. The problem is that the designers haven't made up their mind about how search works with collections. For example, the mock ups for the split view, which were done keeping gnome-music in mind, do not let you search inside a collection: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/music/wire-search.png That is probably not a good fit here because collections can be quite big. It is undecided whether (a) searching inside a collection should be restricted to the collection itself, or whether (b) it should always be global. The behaviour of this code depends on whether we choose (a) or (b). If it is (a), then the save/restore would use a stack to keep the states. If it is (b), then something else. As I mentioned in the commit message, I don't know what we want, so I tried band-aid things to get it to work for the following case: - search in overview - open a collection - open a document - back, back save/restore should be strictly alternating in this case. > @@ +325,3 @@ > + return; > + > + Application.searchController.setString(this._searchState.str); > > No reason to clear this now if you're basically discarding the object > immediately after Fixed.
Created attachment 301132 [details] [review] Hide & restore search when moving to preview or collection view & back Drop an unused function.
Review of attachment 301132 [details] [review]: Looks good to me
Comment on attachment 301132 [details] [review] Hide & restore search when moving to preview or collection view & back Thanks, Cosimo.
(In reply to Debarshi Ray from comment #8) > (In reply to Cosimo Cecchi from comment #6) > > Review of attachment 299640 [details] [review] [review] [review]: > > > > ::: src/embed.js > > @@ +91,3 @@ > > + // _onVisibleChildChanged from from being called while > > we're in > > + // an inconsistent state. > > + this._stack.disconnect(this._visibleChildChangedId); > > > > What happens in that case? The embed should be destroyed together with the > > stack, right? > > I don't know. I was not able to make it misbehave, but I do know that we > would have problems during destroying the Embed object in C: > https://git.gnome.org/browse/gnome-photos/tree/src/photos-embed.c#n560 I poked at this a bit, and found that the behaviour is not very different from what I have seen in gnome-photos [1]. When the GtkBox inside Embed is destroyed, the GtkStack's dispose is called. That leads to 'notify::visible-child' being emitted and Embed._onVisibleChildChanged being invoked. At this point, accessing this._stack.visible_child or this._stack.get_visible_child() does not lead to any CRITICALs, but calling gtk_stack_get_visible_child does. I don't know why, and this where the two programs differ. GtkStack's finalize is called afterwards. [1] https://git.gnome.org/browse/gnome-photos/tree/src/photos-embed.c#n576
(In reply to Debarshi Ray from comment #14) > At this point, accessing this._stack.visible_child or > this._stack.get_visible_child() does not lead to any CRITICALs, but calling > gtk_stack_get_visible_child does. I don't know why, and this where the two > programs differ. Ignore this part. I confused myself. This because I was explicitly NULLing the priv->stack in gnome-photos after disconnecting the handler, and GTK_IS_STACK (NULL) will obviously lead to CRITICALs.
I filed bug 749012 and bug 749021 to fix these properly.
Review of attachment 299640 [details] [review]: This has been fixed in gtk+ master and gtk-3-16. No need to work around this anymore.