GNOME Bugzilla – Bug 701252
Stuck in preview/edit mode if Google Documents is disabled while working with one
Last modified: 2013-05-31 18:09:42 UTC
Steps to reproduce: 1. Enable the Documents switch on a Google account in GOA 2. Preview or edit a document from Google. 3. Disable the Documents switch. 4. The "<" button does not work in the preview/edit mode.
Created attachment 245621 [details] [review] Don't freeze if a GOA account is disabled when previewing or editing
Review of attachment 245621 [details] [review]: It's not completely clear to me how this would solve the bug. The flow here should be: - documentManager.setActiveItem() - Manager.setActiveItem() - the 'active-changed' signal is emitted - Embed._onActiveItemChanged() is called - Application.modeController.setWindowMode() is called which should have the same effect of your explicit calls here. Where is this flow failing in case the GOA account doesn't exist anymore?
(In reply to comment #2) > Review of attachment 245621 [details] [review]: > > It's not completely clear to me how this would solve the bug. Sorry for not being clear. > The flow here should be: > - documentManager.setActiveItem() > - Manager.setActiveItem() > - the 'active-changed' signal is emitted The trick is that TrackerController calls documentManager.clear when a GOA source is removed. This causes BaseManager._activeItem to be set to 'null', which is why documentManager.setActiveItem does not cause a 'active-changed' signal to be emitted. > - Embed._onActiveItemChanged() is called > - Application.modeController.setWindowMode() is called > > which should have the same effect of your explicit calls here. > Where is this flow failing in case the GOA account doesn't exist anymore? I should probably explain this in the commit message?
(In reply to comment #3) > The trick is that TrackerController calls documentManager.clear when a GOA > source is removed. This causes BaseManager._activeItem to be set to 'null', > which is why documentManager.setActiveItem does not cause a 'active-changed' > signal to be emitted. I see; there's no reason for TrackerController to do that - or be active at all - when we're not in overview mode though. Perhaps a cleaner way to get the same result is to just ensure the TrackerController is disabled when we're not in the overview.
Created attachment 245730 [details] [review] Don't freeze if a GOA account is disabled when previewing or editing
Review of attachment 245730 [details] [review]: Thanks, this looks better. Just a minor comment below. ::: src/trackerController.js @@ +132,3 @@ + if (this._refreshPending && newMode == WindowMode.WindowMode.OVERVIEW) { + this._refreshForSource(); + this._refreshPending = false; I think _refreshForSource() itself should unset the pending flag.
Created attachment 245765 [details] [review] Don't freeze if a GOA account is disabled when previewing or editing
Review of attachment 245765 [details] [review]: Thanks, this looks good to me for this bug; I am just wondering if there is any other case where we don't want to refresh the model depending on an external event - in which case we should set refreshPeding unconditionally when the mode != OVERVIEW directly in refreshInternal. Don't block on this though, as this patch looks good as is.
(In reply to comment #8) > Review of attachment 245765 [details] [review]: > > Thanks, this looks good to me for this bug; I am just wondering if there is any > other case where we don't want to refresh the model depending on an external > event - in which case we should set refreshPeding unconditionally when the mode > != OVERVIEW directly in refreshInternal. True. Lets see if something like that comes up in future or not.
Comment on attachment 245765 [details] [review] Don't freeze if a GOA account is disabled when previewing or editing Thanks for the review!