GNOME Bugzilla – Bug 701039
Add apply logic to gnome-initial-setup
Last modified: 2013-05-27 16:30:13 UTC
One of the reasons that the Enterprise login stuff in gnome-initial-setup is completely unusable is that the page changes before the join completes/errors out. These patches will add support for async 'apply' logic which is invoked on the Next button.
Created attachment 245318 [details] [review] assistant: Add async mechanism for applying page changes This kicks in when 'Next' is pressed. If applying fails, then stays on the same page. Cancellation of apply will be further expanded in later commits.
Created attachment 245319 [details] [review] assistant: Page becomes insensitive while applying
Created attachment 245320 [details] [review] assistant: Make apply cancellable by the user Some of the actual operations are not yet cancellable, but that will come shortly.
Created attachment 245321 [details] [review] account: Make kerberos kinit for enterprise login cancellable Migrate to GTask in order to accomplish this.
Created attachment 245322 [details] [review] assistant: Add spinner when applying This makes it clear that something is going on.
Review of attachment 245318 [details] [review]: Hm. I've sort of been on the fence about this, but I'm not sure that application should happen at next button time, as the user can go back and re-do the page. I think it makes sense for keyboard / language, since those need to be immediate, but I think everything else should happen when we transition to summary. And I don't think apply should *ever* fail. If there's an error, we should know about it when the user selects it on the page.
Review of attachment 245321 [details] [review]: Why do we need GTask? It looks like we already pass in a GCancellable and everything.
Well then you need another button (In reply to comment #6) > Review of attachment 245318 [details] [review]: > > Hm. I've sort of been on the fence about this, but I'm not sure that > application should happen at next button time, as the user can go back and > re-do the page. I think it makes sense for keyboard / language, since those > need to be immediate, but I think everything else should happen when we > transition to summary. > > And I don't think apply should *ever* fail. If there's an error, we should know > about it when the user selects it on the page. That's a nice theory and all, but just doesn't reflect reality: * Just basic validating of the fields on the page can take between 1 and 30 seconds. This includes discovery of the domain, checking the user, password. This must occur before leaving the page, or else the user has no idea even basic validation failed. * You will never know if this failed until you actually do the join operation. Having this occur later is strange, probably means you have to go back to the page once the join fails. * There are multiple steps to the join, which can only be known once the join fails, including prompting for additional password, and/or changing the computer name. But most importantly, as it stands right now the Enterprise Login stuff is completely and utterly broken. I'm happy for you to somehow rewrite this so that it fits in with the concept you have in mind (although it's hard to imagine how that would be done). But without such a rewrite, this feature should just be removed, as it doesn't work.(In reply to comment #7) > Review of attachment 245321 [details] [review]: > > Why do we need GTask? It looks like we already pass in a GCancellable and > everything. I tried that. But it doesn't really fit, because the caller doesn't need an error (callee shows/handles errors, rather than caller), cannot run multiple async tasks (and thus no per-task context necessary), and lastly I don't want to abide by the guarantees of callback-is-never-called-directly-before-mainloop. More on the last reason above. We don't want flicker in the case where the page does no applying. In order to use GTask, we have to fire the callback once we get back to the main loop, we cannot call it directly. Therefore we have to track the fact that we are applying, when actually we're really not. This causes all the GUI logic to kick in and desensitize, show spinner, buttons and so on, unnecessarily. This is just asking for flicker, I saw it on my machine when debugging. So anyway, that's why I discarded the idea of making gis_page_apply_begin() a gio style (ie: GTask) async routine.
(In reply to comment #8) > Well then you need another button ??? > That's a nice theory and all, but just doesn't reflect reality: If the user goes back and changes a previous field, I think we need to support that. If we can't do everything immediately before hitting Next, then I'm fine with a cancellable page apply system. But I think doing the apply stuff to the keyboard page is wrong. It should happen immediately, not on Next. > > Review of attachment 245321 [details] [review] [details]: > > > > Why do we need GTask? It looks like we already pass in a GCancellable and > > everything. > > I tried that. I mean porting from GSimpleAsyncResult to GTask. As far as I can tell, it's just some code churn, and I'm not sure what GTask gives us over GSimpleAsyncResult.
(In reply to comment #9) > (In reply to comment #8) > > Well then you need another button > > ??? Indeed. Sorry got distracted. Kids :) > > That's a nice theory and all, but just doesn't reflect reality: > > If the user goes back and changes a previous field, I think we need to support > that. If we can't do everything immediately before hitting Next, then I'm fine > with a cancellable page apply system. Yes, I think we can support that. I've tried it and it doesn't seem to cause a problem. Similar to how a second user is created if you go back and change the local user, a second enterprise login is added if you go back and change the enterprise stuff. > But I think doing the apply stuff to the keyboard page is wrong. It should > happen immediately, not on Next. > > > > Review of attachment 245321 [details] [review] [details] [details]: > > > > > > Why do we need GTask? It looks like we already pass in a GCancellable and > > > everything. > > > > I tried that. > > I mean porting from GSimpleAsyncResult to GTask. As far as I can tell, it's > just some code churn, and I'm not sure what GTask gives us over > GSimpleAsyncResult. Ah yes. So the reason that is ported is because GTask supports return_on_cancel behavior. Which we need to cancel the kerberos kinit operation happening in a thread in an inherently non-cancellable library.
Review of attachment 245319 [details] [review]: OK.
Review of attachment 245321 [details] [review]: OK.
Review of attachment 245320 [details] [review]: OK.
Review of attachment 245322 [details] [review]: ::: gnome-initial-setup/gis-assistant.c @@ +266,3 @@ + + if (applying) + gtk_spinner_start (GTK_SPINNER (assistant->priv->spinner)); gtk_spinner_set_active (applying); ?
(In reply to comment #14) > gtk_spinner_set_active (applying); ? gtk_spinner_set_active() is not a public function.
Created attachment 245397 [details] [review] assistant: Add async mechanism for applying page changes This kicks in when 'Next' is pressed. If applying fails, then stays on the same page. Cancellation of apply will be further expanded in later commits.
Review of attachment 245397 [details] [review]: OK.
Attachment 245319 [details] pushed as 4ceefd8 - assistant: Page becomes insensitive while applying Attachment 245320 [details] pushed as 73824e5 - assistant: Make apply cancellable by the user Attachment 245321 [details] pushed as 553d060 - account: Make kerberos kinit for enterprise login cancellable Attachment 245322 [details] pushed as 95683c0 - assistant: Add spinner when applying Attachment 245397 [details] pushed as f5decfa - assistant: Add async mechanism for applying page changes