GNOME Bugzilla – Bug 784985
Clean up modal dialogs
Last modified: 2017-07-16 16:36:30 UTC
Almost all our dialogs follow the same pattern, so add a small helper widget to share the implementation and cut down on extra styling. See patches. Depends on the ModalDialog <-> Dialog split from bug 762083.
Created attachment 355683 [details] [review] shellMountOperation: Share a style class Two of the dialogs use virtually identical styles, so it makes sense to reuse the actual style classes.
Created attachment 355684 [details] [review] dialog: Add MessageDialogContent A lot of our modal dialogs share a similar structure: [Icon] Some title Maybe a subtitle And sometimes even a body for stuff like longer descriptions. A dedicated widget with a common style will allow us to significantly reduce duplication of both code and CSS.
Created attachment 355685 [details] [review] ui: Use MessageDialogContent where appropriate The gros of our ModalDialogs follow a UI pattern that matches the newly added widget, so port them over to cut down on duplication.
Comment on attachment 355683 [details] [review] shellMountOperation: Share a style class Looks good! The "show-processes-dialog" style class family already seemed kind of misleading since it's an impl detail of the mount operation.
Review of attachment 355684 [details] [review]: Makes total sense to share this stuff :). One thing I'm missing when looking at the next patch is some concept of "extension points" that let you add further content without fiddling with internal layout details. ::: js/ui/dialog.js @@ +222,3 @@ + + _setLabel(prop, value) { + let label = this[`_${prop}`]; This reaches perl levels of magic :p. maybe just pass the label actor to _setLabel?
Review of attachment 355685 [details] [review]: A very nice cleanup overall! I'll follow up the changes in bug #762083 ::: js/ui/accessDialog.js @@ +74,2 @@ this._choices = new Map(); + let choiceIdx = 2; // after title+subtitle This is the one place it'd IMHO be better to have some insertBeforeBody() or somesuch in MessageDialogContent... ::: js/ui/components/keyring.js @@ +37,3 @@ + this.contentLayout.add(this._content); + + // FIXME: Why does this break now? Hmm, can't do but guess. If this is a crash, it's maybe related to the one I mentioned in https://bugzilla.gnome.org/show_bug.cgi?id=762083#c41 ?. However I saw that the troublesome paths were C property getters from gjs objects, and AFAICT SYNC_CREATE shouldn't result in it. ::: js/ui/extensionDownloader.js @@ +212,3 @@ box.add(icon); + let label = new St.Label({ style_class: 'message-dialog-title headline', As per the commit log, I wonder if these (and the related style changes) belong together :).
(In reply to Carlos Garnacho from comment #6) > This reaches perl levels of magic :p. Harsh :-) But yeah, passing the label is a lot more readable ... (In reply to Carlos Garnacho from comment #7) > This is the one place it'd IMHO be better to have some insertBeforeBody() or > somesuch in MessageDialogContent... Agreed, added. > ::: js/ui/components/keyring.js > @@ +37,3 @@ > + this.contentLayout.add(this._content); > + > + // FIXME: Why does this break now? > > Hmm, can't do but guess. If this is a crash, it's maybe related to the one I > mentioned in https://bugzilla.gnome.org/show_bug.cgi?id=762083#c41 ?. > However I saw that the troublesome paths were C property getters from gjs > objects, and AFAICT SYNC_CREATE shouldn't result in it. It is a crash, but I can't figure out what's wrong (it doesn't help that the segfault is buried under several layers of C++). Some more fun info: 1. the crash is independent from the SYNC_CREATE_FLAG 2. I can bind prompt.message to _content._title.text (no surprise, but it shows that binding KeyringPrompt to some C GObject works) 3. I can then bind _content._title.text to _content.body (so binding from some other C GObject to the GJS GObject works) > ::: js/ui/extensionDownloader.js > As per the commit log, I wonder if these (and the related style changes) > belong together :). They kind of do, as the reason for the change is that the style classes it's (re)using are removed by the commit. But we can do that just as well as part of the preparation, so I've split it out for clarity now.
Created attachment 355707 [details] [review] dialog: Add MessageDialogContent Less perl, more "extension points".
Created attachment 355708 [details] [review] extensionDownloader: Don't reuse .prompt-* style classes Those will go away when we port authentication prompts to the new MessageDialogContent widget, so pick the style classes from there and adjust individual properties with more specific rules to re- produce the existing style.
Created attachment 355709 [details] [review] ui: Use MessageDialogContent where appropriate The gros of our ModalDialogs follow a UI pattern that matches the newly added widget, so port them over to cut down on duplication.
(In reply to Florian Müllner from comment #8) > (In reply to Carlos Garnacho from comment #6) > > This reaches perl levels of magic :p. > > Harsh :-) Hey, just picking on JS, you know me :p. I did maintain a large perl codebase, and ended up thinking that the less magic oneliners, the less future readers (and myself) had to spend thinking on correctness/results. > > ::: js/ui/components/keyring.js > > @@ +37,3 @@ > > + this.contentLayout.add(this._content); > > + > > + // FIXME: Why does this break now? > > > > Hmm, can't do but guess. If this is a crash, it's maybe related to the one I > > mentioned in https://bugzilla.gnome.org/show_bug.cgi?id=762083#c41 ?. > > However I saw that the troublesome paths were C property getters from gjs > > objects, and AFAICT SYNC_CREATE shouldn't result in it. > > It is a crash, but I can't figure out what's wrong (it doesn't help that the > segfault is buried under several layers of C++). > > Some more fun info: > 1. the crash is independent from the SYNC_CREATE_FLAG > 2. I can bind prompt.message to _content._title.text > (no surprise, but it shows that binding KeyringPrompt to some C GObject > works) > 3. I can then bind _content._title.text to _content.body > (so binding from some other C GObject to the GJS GObject works) Fishy...
Comment on attachment 355707 [details] [review] dialog: Add MessageDialogContent Looks good! Feel free to push together with the other a-c-n patches in #762083 (which I assume you have in your tree), or I can do it.
Comment on attachment 355708 [details] [review] extensionDownloader: Don't reuse .prompt-* style classes LGTM
Comment on attachment 355709 [details] [review] ui: Use MessageDialogContent where appropriate Looks great, except the pending fixme... the rest of the patch is too good to wait for this though :), I'll make it your call whether to leave the fixme or keep the keyring dialog change for a future commit.
Attachment 355683 [details] pushed as fbf5f98 - shellMountOperation: Share a style class Attachment 355707 [details] pushed as b5860d6 - dialog: Add MessageDialogContent Attachment 355708 [details] pushed as f77333e - extensionDownloader: Don't reuse .prompt-* style classes Attachment 355709 [details] pushed as 593b431 - ui: Use MessageDialogContent where appropriate
(In reply to Carlos Garnacho from comment #15) > Looks great, except the pending fixme... Yeah, this will need a bit of investigation. I went ahead and pushed with the fixme, because the workaround isn't terrible (it's not even clear why it's substantially different than the crashing code) and the fix is likely not on the gnome-shell side. If we are lucky, gjs updating to mozjs52 may fix it for free even ...