GNOME Bugzilla – Bug 652459
Prompt for keyring unlocks using gnome-shell
Last modified: 2012-03-07 15:29:22 UTC
gnome-keyring should have gnome-shell integration. It should prompt for keyrings to be unlocked using the system model dialog style. Part of the stated goals of gnome-keyring are to not put passwords in pageable memory (which allows them to be written to disk). This is a tracker bug which will cover the various bits that need to be implemented in order to complete this work.
Just to make sure it doesn't get lost, I'm attaching some earlier work and discussion by Ray and David here.
Created attachment 189867 [details] some discussion
Created attachment 189868 [details] [review] incomplete patch
Unfortunately, the keyring-daemon bits did get lost when I cleaned up my jhbuild trees... I think it was only half a days work anyway, though.
*** Bug 527361 has been marked as a duplicate of this bug. ***
Created attachment 204861 [details] [review] Add gnome-keyring prompter * Add a keyring prompter based on GcrSystemPrompter * Adds dependency on gcr version 3.3.3 or higher * Not yet using unmerged support for non-pageable memory * Not yet including checkbox choices
Created attachment 204950 [details] [review] Updated patch with checkbox and fixed various bugs. The checkbox design may need some tweaking from designers. To show the prompt build the latest gcr, and run: $ gcr/tests/frob-prompt gcr/tests/files/prompt-tests/password-choice.prompt Or lock and unlock a keyring using seahorse although that may not show all the various options and bits Add gnome-keyring prompter * Add a keyring prompter based on GcrSystemPrompter * Adds dependency on gcr version 3.3.4 or higher * Not yet using unmerged support for non-pageable memory
Review of attachment 204950 [details] [review]: Overall looks pretty good - most of the comments are mere style nits, except for some visuals (mostly the checkbox of course) and the C/JS split being somewhat unclear. ::: data/theme/gnome-shell.css @@ +285,3 @@ + background-image: url("check-off.svg"); + background-size: contain; + background-size: 22px 20px; That doesn't work - background-size has to be either 'contain' or a fixed length. ::: js/ui/keyringPrompt.js @@ +7,3 @@ + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2, or (at your option) + * any later version. Could do without the copyright notice (which afaik doesn't have any real legal meaning anyway) - JS comments are loaded and stripped at runtime, which has a (admittedly minimal) cost (I guess that's the reason why we generally avoid copyright headers in our JS sources). No strong opinion though. @@ +42,3 @@ + + _init: function() { + this.parent({ styleClass: 'polkit-dialog' }); I know we do that as well in networkAgent.js, but it still feels wrong - in my opinion, it would be better to either rename 'polkit-foo' to something more general like 'password-foo' (should be done in a separate patch), or use a proper style class like 'keyring-foo' and modify the CSS to use '.polkit-dialog, .keyring-dialog' etc. (a patch for networkAgent.js would be appreciated in that case ;-). @@ +44,3 @@ + this.parent({ styleClass: 'polkit-dialog' }); + + this.native = new Shell.KeyringPrompt(); Can we come up with a better name than 'native'? How about s/PromptDialog/KeyringDialog/ and s/native/prompt/? @@ +67,3 @@ + + let subject = new St.Label({ style_class: 'polkit-dialog-headline' }); + subject.set_text("This is the invalid subject of the prompt"); Is this really necessary? @@ +77,3 @@ + description.clutter_text.ellipsize = Pango.EllipsizeMode.NONE; + description.clutter_text.line_wrap = true; + this.native.bind_property("description", description, "text", GObject.BindingFlags.SYNC_CREATE); Nit: we use double quotes for translatable text, so use 'message' and 'text' here (and elsewhere as appropriate) @@ +88,3 @@ + key: Clutter.Escape + }, + { label: _("Continue"), "Continue" is a weird label for a system dialog (it has quite a ring of GtkAssistant ...) - at least for unlock dialogs, I think we'll want "Unlock" (not sure about confirmation dialogs - "Store password" maybe? We should ask the designers) @@ +108,3 @@ + this._passwordEntry.clutter_text.connect('activate', Lang.bind(this, this._onPasswordActivate)); + table.add(this._passwordEntry, { row: row, col: 1, x_expand: true, x_fill: true, x_align: St.Align.START }); + this.native.set_password_entry(this._passwordEntry); I'm not quite sure about the C/JS split here - passing a widget from the UI to the "backend" seems odd. Any particular reason why the code parts in question cannot be moved to JS? @@ +109,3 @@ + table.add(this._passwordEntry, { row: row, col: 1, x_expand: true, x_fill: true, x_align: St.Align.START }); + this.native.set_password_entry(this._passwordEntry); + this.setInitialKeyFocus(this._passwordEntry); Just for clarification - if password_visible is false (I guess when using a fancier authentication method?), we want keyboard focus on the dialog buttons, so it's ok to not call setInitialKeyFocus() in that case. Right? @@ +115,3 @@ + if (this.native.confirm_visible) { + var label = new St.Label(({ style_class: 'polkit-dialog-password-label' })); + label.set_text(_("Type again:")); Sounds a bit clumsy to me, but no better suggestions on my part - I guess it's a good idea to run all user-visible strings by the design team anyway. @@ +129,3 @@ + + if (this.native.choice_visible) { + let choice = new St.Button({ toggle_mode: true, style_class: "check-box" }); 'check-box' (single quotes!) is very generic (which is fine by me), but the corresponding CSS is not - I wonder if it would be clearer to use a composite widget here (for instance a BoxLayout with a Bin (for the check-box image) and a Label) and set it as the Button child. In addition to the artwork, the ellipsized label also feels wrong (in my testing, I get "Automatically unlock this keyring whenever ...", which means the most important part is missing). I suspect this is way easier to fix with a custom Button child than with crazy CSS. @@ +152,3 @@ + // NOTE: ModalDialog.open() is safe to call if the dialog is + // already open - it just returns true without side-effects + if (!this.open(global.get_current_time())) { Could save one indentation level by using if (this.option()) return true; // This can fail ... @@ +204,3 @@ + +function init() { + /* TODO: probably should be passing arguments, but can't figure out gjs errors */ I'm not sure what you mean by that comment, but it obviously needs to go before merging :) @@ +207,3 @@ + prompter = new Gcr.SystemPrompter(); + prompter.connect('new-prompt', function(prompter) { + var prompt = new PromptDialog(); why not let? ::: src/Makefile.am @@ +85,3 @@ -DGNOME_SHELL_PKGLIBDIR=\"$(pkglibdir)\" \ + -DJSDIR=\"$(pkgdatadir)/js\" \ + -DGCR_API_SUBJECT_TO_CHANGE Any reason for not having the define in shell-keyring-prompt.c? ::: src/shell-keyring-prompt.c @@ +33,3 @@ +#define SHELL_KEYRING_PROMPT_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST ((klass), SHELL_TYPE_KEYRING_PROMPT, ShellKeyringPromptClass)) +#define SHELL_IS_KEYRING_PROMPT_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), SHELL_TYPE_KEYRING_PROMPT)) +#define SHELL_KEYRING_PROMPT_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), SHELL_TYPE_KEYRING_PROMPT, ShellKeyringPromptClass)) We are admittedly not quite consistent, but almost every class has its GObject glue typedefs/defines in the header. (And no, I don't really care deeply about that ;-) @@ +42,3 @@ + PROMPT_NONE, + PROMPT_CONFIRMING, + PROMPT_PASSWORDING "passwording" sounds odd to me - maybe use CONFIRMATION/PASSWORD or CONFIRM/UNLOCK? (I may completely misunderstand what the different modes are about - if so, there should probably be a comment) @@ +61,3 @@ + GSimpleAsyncResult *async_result; + StEntry *password; + StEntry *confirm; I'd call those password_entry and confirm_entry (which match the corresponding property names) - at a later point, you do char *password = st_entry_get_text (self->password) which reads funny @@ +85,3 @@ + PROP_CONFIRM_VISIBLE, + PROP_WARNING_VISIBLE, + PROP_CHOICE_VISIBLE, The naming is a bit odd here (due to the C/JS split) - the properties are less about what *is* visible, but rather about what *should be* visible. So 'show-foo' sounds more appropriate to me, though it looks like you could just move those conditions to javascript if you expose PromptMode @@ +243,3 @@ + if (self->shown) { + self->shown = FALSE; + g_signal_emit (self, signals[SIGNAL_HIDE_PROMPT], 0); Emitting signals in dispose is icky, isn't it? @@ +378,3 @@ + + if (self->async_result != NULL) { + g_warning ("this prompt is already prompting"); I love this warning :)
Created attachment 207264 [details] [review] Updated: Add gnome-keyring prompter * Add a keyring prompter based on GcrSystemPrompter * Adds dependency on gcr version 3.3.5 or higher * Not yet using unmerged support for non-pageable memory
(In reply to comment #8) > Review of attachment 204950 [details] [review]: > ::: data/theme/gnome-shell.css > @@ +285,3 @@ > + background-image: url("check-off.svg"); > + background-size: contain; > + background-size: 22px 20px; > > That doesn't work - background-size has to be either 'contain' or a fixed > length. Fixed. > ::: js/ui/keyringPrompt.js > @@ +7,3 @@ > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2, or (at your option) > + * any later version. > > Could do without the copyright notice (which afaik doesn't have any real legal > meaning anyway) - JS comments are loaded and stripped at runtime, which has a > (admittedly minimal) cost (I guess that's the reason why we generally avoid > copyright headers in our JS sources). No strong opinion though. So the copyright holder wouldn't be recorded anywhere in that case? Obviously the copyright holder still holds the copyright regardless of it being recorded, but that's not what I mean. Maybe I'm not understanding correctly. Am I missing something? BTW, it's sort of makes me giggle that we choose to implement the shell in a non-compiled language like javascript but then are worried about the performance of parsing comments :P > @@ +42,3 @@ > + > + _init: function() { > + this.parent({ styleClass: 'polkit-dialog' }); > > I know we do that as well in networkAgent.js, but it still feels wrong - in my > opinion, it would be better to either rename 'polkit-foo' to something more > general like 'password-foo' (should be done in a separate patch), or use a > proper style class like 'keyring-foo' and modify the CSS to use > '.polkit-dialog, .keyring-dialog' etc. (a patch for networkAgent.js would be > appreciated in that case ;-). Done in bug #669776 > @@ +44,3 @@ > + this.parent({ styleClass: 'polkit-dialog' }); > + > + this.native = new Shell.KeyringPrompt(); > > Can we come up with a better name than 'native'? How about > s/PromptDialog/KeyringDialog/ and s/native/prompt/? Done. > @@ +67,3 @@ > + > + let subject = new St.Label({ style_class: 'polkit-dialog-headline' }); > + subject.set_text("This is the invalid subject of the prompt"); > > Is this really necessary? Nope, a leftover from some frustrating testing :) Removed. > @@ +77,3 @@ > + description.clutter_text.ellipsize = Pango.EllipsizeMode.NONE; > + description.clutter_text.line_wrap = true; > + this.native.bind_property("description", description, "text", > GObject.BindingFlags.SYNC_CREATE); > > Nit: we use double quotes for translatable text, so use 'message' and 'text' > here (and elsewhere as appropriate) Good to know. Changed all instances. > @@ +88,3 @@ > + key: Clutter.Escape > + }, > + { label: _("Continue"), > > "Continue" is a weird label for a system dialog (it has quite a ring of > GtkAssistant ...) - at least for unlock dialogs, I think we'll want "Unlock" > (not sure about confirmation dialogs - "Store password" maybe? We should ask > the designers) Done. Made changes to gcr and gnome-keyring to do this. Updated gcr dependency to 3.3.5. Make sure to also use latest gnome-keyring git master when testing, if you want to see the new labels. > @@ +108,3 @@ > + this._passwordEntry.clutter_text.connect('activate', > Lang.bind(this, this._onPasswordActivate)); > + table.add(this._passwordEntry, { row: row, col: 1, x_expand: true, > x_fill: true, x_align: St.Align.START }); > + this.native.set_password_entry(this._passwordEntry); > > I'm not quite sure about the C/JS split here - passing a widget from the UI to > the "backend" seems odd. Any particular reason why the code parts in question > cannot be moved to JS? This is related to bug# 652460. This prompt can be used for various passwords of differring secrecy requirements. We want to keep the passwords in non-pageable memory. To do this we keep them from being passed through javascript (where we can't even zero out memory after use). > @@ +109,3 @@ > + table.add(this._passwordEntry, { row: row, col: 1, x_expand: true, > x_fill: true, x_align: St.Align.START }); > + this.native.set_password_entry(this._passwordEntry); > + this.setInitialKeyFocus(this._passwordEntry); > > Just for clarification - if password_visible is false (I guess when using a > fancier authentication method?), we want keyboard focus on the dialog buttons, > so it's ok to not call setInitialKeyFocus() in that case. Right? Yes that's right. In addition there's no password field visible for confirmation prompts. > @@ +115,3 @@ > + if (this.native.confirm_visible) { > + var label = new St.Label(({ style_class: > 'polkit-dialog-password-label' })); > + label.set_text(_("Type again:")); > > Sounds a bit clumsy to me, but no better suggestions on my part - I guess it's > a good idea to run all user-visible strings by the design team anyway. Alright. > @@ +129,3 @@ > + > + if (this.native.choice_visible) { > + let choice = new St.Button({ toggle_mode: true, style_class: > "check-box" }); > > 'check-box' (single quotes!) is very generic (which is fine by me), but the > corresponding CSS is not - I wonder if it would be clearer to use a composite > widget here (for instance a BoxLayout with a Bin (for the check-box image) and > a Label) and set it as the Button child. In addition to the artwork, the > ellipsized label also feels wrong (in my testing, I get "Automatically unlock > this keyring whenever ...", which means the most important part is missing). I > suspect this is way easier to fix with a custom Button child than with crazy > CSS. I agree that it needs to be done better. I tried to do it similar to what you're suggesting but ran out of time somewhere between undocumented St toolkit, unfamiliar (or strange) clutter behavior, and CSS selectors that didn't work as expected. In addition a new widget would need to support keyboard, active, focus, hover and other things I'm sure. StButton has all those already, so that was the reason behind restyling it. The ellipsizing behavior of StButton is quite brittle, and should probably be tweaked or removed. I can imagine it running into problems with translated labels as well. In addition the checkbox needs to have its look designed properly. So maybe I should setup a new bug to create a checkbox control, and make this depend on it, since it seems I got both the design and implementation wrong. Could you come up with a rough patch that implements it as you describe? > @@ +152,3 @@ > + // NOTE: ModalDialog.open() is safe to call if the dialog is > + // already open - it just returns true without side-effects > + if (!this.open(global.get_current_time())) { > > Could save one indentation level by using > > if (this.option()) > return true; > > // This can fail ... > > @@ +204,3 @@ > + > +function init() { > + /* TODO: probably should be passing arguments, but can't figure out gjs > errors */ > > I'm not sure what you mean by that comment, but it obviously needs to go before > merging :) Wanted to pass the "flags" property of GCR_SYSTEM_PROMPTER_SINGLE. But couldn't make it work and ran out of time. So relying on defaults here. Have now removed the comment. > @@ +207,3 @@ > + prompter = new Gcr.SystemPrompter(); > + prompter.connect('new-prompt', function(prompter) { > + var prompt = new PromptDialog(); > > why not let? Changed. Just not used to using the let extension. > ::: src/Makefile.am > @@ +85,3 @@ > -DGNOME_SHELL_PKGLIBDIR=\"$(pkglibdir)\" \ > + -DJSDIR=\"$(pkgdatadir)/js\" \ > + -DGCR_API_SUBJECT_TO_CHANGE > > Any reason for not having the define in shell-keyring-prompt.c? Moved. > ::: src/shell-keyring-prompt.c > @@ +33,3 @@ > +#define SHELL_KEYRING_PROMPT_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST > ((klass), SHELL_TYPE_KEYRING_PROMPT, ShellKeyringPromptClass)) > +#define SHELL_IS_KEYRING_PROMPT_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE > ((klass), SHELL_TYPE_KEYRING_PROMPT)) > +#define SHELL_KEYRING_PROMPT_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS > ((obj), SHELL_TYPE_KEYRING_PROMPT, ShellKeyringPromptClass)) > > We are admittedly not quite consistent, but almost every class has its GObject > glue typedefs/defines in the header. (And no, I don't really care deeply about > that ;-) The class is not in the header. The class is 'private'. This is a pretty standard This is pretty standard pattern when the class is not in the header. Since these defines aren't used, I've removed them. > > @@ +42,3 @@ > + PROMPT_NONE, > + PROMPT_CONFIRMING, > + PROMPT_PASSWORDING > > "passwording" sounds odd to me - maybe use CONFIRMATION/PASSWORD or > CONFIRM/UNLOCK? (I may completely misunderstand what the different modes are > about - if so, there should probably be a comment) Changed to PROMPTING_CONFIRM, and PROMPTING_PASSWORD. We don't want to have a symbol here called PROMPT_PASSWORD as that would be confusing. > @@ +61,3 @@ > + GSimpleAsyncResult *async_result; > + StEntry *password; > + StEntry *confirm; > > I'd call those password_entry and confirm_entry (which match the corresponding > property names) - at a later point, you do > > char *password = st_entry_get_text (self->password) > > which reads funny Fixed. > @@ +85,3 @@ > + PROP_CONFIRM_VISIBLE, > + PROP_WARNING_VISIBLE, > + PROP_CHOICE_VISIBLE, > > The naming is a bit odd here (due to the C/JS split) - the properties are less > about what *is* visible, but rather about what *should be* visible. So > 'show-foo' sounds more appropriate to me, though it looks like you could just > move those conditions to javascript if you expose PromptMode Alright, but this adds a lot of complexity. Done. > @@ +243,3 @@ > + if (self->shown) { > + self->shown = FALSE; > + g_signal_emit (self, signals[SIGNAL_HIDE_PROMPT], 0); > > Emitting signals in dispose is icky, isn't it? That's what GtkWidget does. Why is it icky? Certainly shouldn't do it from finalize, but during dispose self is intact. But anyway, this is a direct result of the inability for gjs to implement a proper GObject. So we have to tie the life of the js widget to the life of the ShellKeyringPrompt using a signal like this. > @@ +378,3 @@ > + > + if (self->async_result != NULL) { > + g_warning ("this prompt is already prompting"); > > I love this warning :) Heh, fixed.
(In reply to comment #10) > So the copyright holder wouldn't be recorded anywhere in that case? Obviously > the copyright holder still holds the copyright regardless of it being recorded, > but that's not what I mean. Maybe I'm not understanding correctly. Am I missing > something? Maybe I'm naive, but I'd assume that git is a sufficiently good record. I am not a lawyer of course, but I doubt that if you add a 10.000 LOC file, I can "steal" your copyright by changing a single line (-Copyright Stef Walters +Copyright Florian Müllner). Anyway, I'm not insisting that you remove it, I just doubt it's really necessary. > BTW, it's sort of makes me giggle that we choose to implement the shell in a > non-compiled language like javascript but then are worried about the > performance of parsing comments :P I don't really know *why* we don't have copyright notices in most javascript files, but it's most certainly the case - fun stats: $ find js -name *.js | wc -l 80 $ find js -name *.js | xargs grep -l "terms of the GNU" | wc -l 6 > > I'm not quite sure about the C/JS split here - passing a widget from the UI to > > the "backend" seems odd. Any particular reason why the code parts in question > > cannot be moved to JS? > > This is related to bug# 652460. This prompt can be used for various passwords > of > differring secrecy requirements. We want to keep the passwords in non-pageable > memory. > To do this we keep them from being passed through javascript (where we can't > even zero > out memory after use). Hmm, that's what I suspected - but it should still be possible to pass the underlying ClutterText rather than the StEntry? That split would make a little more sense to me ... > So maybe I should setup a new bug to create a checkbox control, and make this > depend on it, since it seems I got both the design and implementation wrong. > Could > you come up with a rough patch that implements it as you describe? I attached a patch in bug 669811 - the new classes are rather small, so it might make more sense to keep the code in keyringPrompt.js > > I'm not sure what you mean by that comment, but it obviously needs to go before > > merging :) > > Wanted to pass the "flags" property of GCR_SYSTEM_PROMPTER_SINGLE. But couldn't > make it work and ran out of time. So relying on defaults here. > > Have now removed the comment. > The class is not in the header. The class is 'private'. This is a pretty > standard > This is pretty standard pattern when the class is not in the header. Since > these > defines aren't used, I've removed them. Sure. > > "passwording" sounds odd to me - maybe use CONFIRMATION/PASSWORD or > > CONFIRM/UNLOCK? (I may completely misunderstand what the different modes are > > about - if so, there should probably be a comment) > > Changed to PROMPTING_CONFIRM, and PROMPTING_PASSWORD. We don't want to have a > symbol here called PROMPT_PASSWORD as that would be confusing. Works for me. > > @@ +243,3 @@ > > + if (self->shown) { > > + self->shown = FALSE; > > + g_signal_emit (self, signals[SIGNAL_HIDE_PROMPT], 0); > > > > Emitting signals in dispose is icky, isn't it? > > That's what GtkWidget does. Why is it icky? Certainly shouldn't do it from > finalize, but during dispose self is intact. The question was serious - so thanks for the clarification :-) > But anyway, this is a direct result of the inability for gjs to implement a > proper > GObject. So we have to tie the life of the js widget to the life of the > ShellKeyringPrompt > using a signal like this. It is now actually possible to implement a GObject in gjs, but that's hardly a blocking issue for now. > > @@ +378,3 @@ > > + > > + if (self->async_result != NULL) { > > + g_warning ("this prompt is already prompting"); > > > > I love this warning :) > > Heh, fixed. Heh - it took me a while to figure out whether the warning was silly or genius, but I finally decided on the latter :(
Created attachment 208504 [details] [review] Updated patch after Florian's review. Still has some layout problems shown at: http://thewalter.net/stef/misc/screencast-keyring-layout-1.webm Add gnome-keyring prompter * Add a keyring prompter based on GcrSystemPrompter * Adds dependency on gcr version 3.3.5 or higher * Not yet using unmerged support for non-pageable memory
Review of attachment 208504 [details] [review]: (In reply to comment #12) > Created an attachment (id=208504) [details] [review] > Updated patch after Florian's review. > > Still has some layout problems shown at: > http://thewalter.net/stef/misc/screencast-keyring-layout-1.webm I don't see anything wrong with the code. The problem seems to be that the password entry's natural width (which depends on the mount of text entered) is passed as forWidth in the checkbox' height request. So the source is probably somewhere in StTable, let's land this first and fix it later. Looking good except for minor nits and some stuff that would be better off in separate commits. ::: js/ui/checkBox.js @@ +97,3 @@ + + getLabelActor: function() { + return this._container.label; Unrelated change, please split ::: js/ui/keyringPrompt.js @@ +6,3 @@ +const St = imports.gi.St; +const Pango = imports.gi.Pango; +const GLib = imports.gi.GLib; Unused import. @@ +139,3 @@ + + this._controlTable = table; + this._messageBox.add(table, { x_fill: true, y_fill: true, x_expand: true }); StBoxLayoutChild doesn't have an 'x_expand' property, just 'expand' (I don't think you want 'expand' here though, so just remove it) ::: src/shell-keyring-prompt.h @@ +4,3 @@ + Copyright (C) 2011 Stefan Walter + + Gnome keyring is free software; you can redistribute it and/or Copy-paste error? (existing notes mostly use "This program ...", with some uses of the correct "This library ..." mixed in) @@ +9,3 @@ + License, or (at your option) any later version. + + Gnome keyring is distributed in the hope that it will be useful, Dito. ::: tests/interactive/checkbox.js @@ +1,1 @@ +// -*- mode: js; js-indent-level: 4; indent-tabs-mode: nil -*- Not sure what exactly this text is testing, but it should be a separate commit anyway (so you can explain it in the commit message ;-) @@ +20,3 @@ + width: stage.width, + height: stage.height, + style: 'padding: 10px; spacing: 10px; font: 15px sans-serif;' }); You should add 'color: black;' or something to make the text visible.
Created attachment 208659 [details] [review] Updated for latest review * Split out checkBox.js changes * Remove checkbox.js interactive test * Remove unused import * don't use x_expand as directed * Fix copy paste error
Review of attachment 208659 [details] [review]: Looks good. I overlooked some other 'x_expand'/'y_expand' errors in the previous review, sorry about that - please fix those before pushing (and after getting a freeze break approval of course) ::: js/ui/keyringPrompt.js @@ +31,3 @@ + vertical: false }); + this.contentLayout.add(mainContentBox, + { x_fill: true, y_fill: true, x_expand: true, y_expand: true }); Sorry, didn't notice that in the previous review - contentLayout is another BoxLayout, so there's no x_expand/y_expand, just expand (in fact, mainContentBox is the only child, so I don't think you need any of the child properties) @@ +43,3 @@ + vertical: true }); + mainContentBox.add(this._messageBox, + { y_align: St.Align.START, x_expand: true, y_expand: true, x_fill: true, y_fill: true }); Again, there's just 'expand'
Created attachment 208675 [details] [review] Fixes for expand child properties
Review of attachment 208675 [details] [review]: Assuming no other changes than the ones pointed out before
Comment on attachment 208675 [details] [review] Fixes for expand child properties Merged into master.
I was hoping to try to get gnome-shell 3.3.91 into Ubuntu Precise next week. However, we were planning to stick with gnome-keyring 3.2 so this late change is a big headache for us.
Maybe it's just a small headache; I'll look into it more this weekend. Thanks.
This doesn't depend on gnome-keyring 3.4. All you'll need to bring in is gcr 3.4 to satisfy dependencies. Then the prompter code will be present in the shell but gnome-keyring won't use it.
Stef, I don't think what you're proposing is possible. gcr 3.4 provides the same files as gnome-keyring 3.2 so they have to be upgraded together. Ubuntu developers are hesitant to allow gnome-keyring into our LTS release. https://bugs.launchpad.net/ubuntu/+source/gnome-keyring/+bug/947447 So we're left with some difficult choices: 1. Upgrade gnome-shell to 3.4 and carry a patch to revert this dependency on gcr 3.4 2. Upgrade gnome-shell to 3.4 and hope that gnome-keyring, which is a critical part of the infrastructure doesn't have too many bugs in the rewrite, which hasn't been tested near as much as the older gnome-keyring. 3. Relegate gnome-shell 3.4 and gnome-keyring 3.4 to the GNOME3 PPA where it currently sits, and leave gnome-shell 3.2 and gnome-keyring 3.2 in the ordinary Ubuntu repositories until next release cycle. I think this will be a major disappointment to GNOME developers and users (even though the PPA isn't that hard to use).
You're right. I hope you identify the solution that's right for you. If you want some more details from me about any specifics, i can help you out. I'm stefw on gimpnet irc.
This (shell keyring dialogs) has been on the list of gnome 3.4 features for a long time. It wasn't exactly sneaked in at the last minute...