GNOME Bugzilla – Bug 752792
Port the codebase to use the 'Extends' keyword
Last modified: 2015-10-12 18:36:22 UTC
Now with gjs we can use the 'Extends' keyword to inherit from a object instead of encapsulate it.
Created attachment 308120 [details] [review] selections: Extend objects instead of encapsulate them.
Created attachment 308121 [details] [review] selections: Extend objects instead of encapsulate them
Created attachment 308123 [details] [review] selections: Extend objects instead of encapsulate them
Created attachment 308124 [details] [review] embed: Extend objects instead of encapsulate them
Created attachment 308126 [details] [review] errorBox: Extend objects instead of encapsulate them
Created attachment 308127 [details] [review] mainToolbar: Extend objects instead of encapsulate them
Created attachment 308129 [details] [review] mainWindow: Extend objects instead of encapsulate them
Created attachment 308130 [details] [review] manager: Extend objects instead of encapsulate them
Created attachment 308133 [details] [review] password: Extend objects instead of encapsulate them
Created attachment 308134 [details] [review] places: Extend objects instead of encapsulate them
Created attachment 308136 [details] [review] mainToolbar: Extend objects instead of encapsulate them
Created attachment 308139 [details] [review] presentation: Extend objects instead of encapsulate them https://bugzilla.gnome.org/show_bug.cgi?id=752792 https://bugzilla.gnome.org/show_bug.cgi?id=726450
Created attachment 308140 [details] [review] presentation: Extend objects instead of encapsulate them
Created attachment 308142 [details] [review] preview: Extend objects instead of encapsulate them
Created attachment 308143 [details] [review] properties: Extend objects instead of encapsulate them
Created attachment 308144 [details] [review] sharing: Extend objects instead of encapsulate them
Created attachment 308146 [details] [review] view: Extend objects instead of encapsulate them
Created attachment 308166 [details] [review] mainWindow: Extend objects instead of encapsulate them
Created attachment 308167 [details] [review] presentation: Extend objects instead of encapsulate them
Created attachment 308168 [details] [review] view: Extend objects instead of encapsulate them
Review of attachment 308123 [details] [review]: Looks good to me, thanks. One small nitpick - I think "Use inheritance instead of composition" would be a more idiomatic way of describing the change.
Review of attachment 308124 [details] [review]: Thanks. Pushed.
Review of attachment 308126 [details] [review]: Looks good to me.
Review of attachment 308130 [details] [review]: ::: src/manager.js @@ +182,3 @@ const BaseModel = new Lang.Class({ Name: 'BaseModel', + Extends: Gio.Menu, I must point out the GMenu is defined as a final type in C. So, even if we can inherit Gio.Menu in JavaScript, it might be best not to do so, unless we can convince the right people that GMenu should be derivable.
Review of attachment 308136 [details] [review]: Thanks for the patch, Alessandro. ::: src/preview.js @@ +1015,2 @@ this.revealer = new Gtk.Revealer({ valign: Gtk.Align.START }); + this.revealer.add(this); I am not so sure about this part. The idiom for composite widgets is to inherit from the top-most widget type. In this case, it would be Gtk.Revealer. Having a member widget own the instance itself looks strange, and can lead to weird life cycle issues. Instead, how about inheriting PreviewFullscreenToolbar from Gtk.Revealer, and having the PreviewToolbar as a member variable (say this._toolbar)? The only PreviewToolbar method that is used for PreviewFullscreenToolbar is setModel. We can add a setModel to it, which in turn calls this._toolbar.setModel.
Created attachment 308263 [details] [review] mainToolbar: Use inheritance instead of composition
Review of attachment 308263 [details] [review]: Looks good to me. ::: src/preview.js @@ +1048,1 @@ }, Thanks for catching this.
Review of attachment 308133 [details] [review]: Perfect. Thanks Alessandro.
Review of attachment 308134 [details] [review]: ++
Review of attachment 308166 [details] [review]: ++
Review of attachment 308167 [details] [review]: ++
Review of attachment 308142 [details] [review]: ++
Review of attachment 308143 [details] [review]: ++
Review of attachment 308144 [details] [review]: ++
Review of attachment 308168 [details] [review]: ++
Thanks for all the wonderful patches.
Created attachment 308320 [details] [review] edit: Use inheritance instead of composition
Review of attachment 308320 [details] [review]: ++
Created attachment 308419 [details] [review] searchbar: Use inheritance instead of composition
Created attachment 308554 [details] [review] selections: Use inheritance instead of composition
Review of attachment 308419 [details] [review]: Thanks for the patch, Alessandro. ::: src/searchbar.js @@ +74,3 @@ Application.application.disconnect(searchStateId); Application.application.change_action_state('search', GLib.Variant.new('b', false)); })); For some reason this anonymous handler is not getting called anymore. Interestingly, if I use vfunc_destroy (to override the destroy vfunc of the widget) it gets called. Since this is not getting called, we are not disconnecting from searchStateId, which leads to a CRITICAL if we go to the preview and toggle the search bar. :/ @@ +96,3 @@ handleEvent: function(event) { // Skip if the search bar is shown and the focus is elsewhere + if (this.get_search_mode() && !this._searchEntry.is_focus) this.search_mode_enabled should work, so no need to change this. @@ +109,3 @@ + reveal: function() { + this.set_search_mode(true); Ditto.
(In reply to Debarshi Ray from comment #41) > Review of attachment 308419 [details] [review] [review]: > > Thanks for the patch, Alessandro. > > ::: src/searchbar.js > @@ +74,3 @@ > Application.application.disconnect(searchStateId); > Application.application.change_action_state('search', > GLib.Variant.new('b', false)); > })); > > For some reason this anonymous handler is not getting called anymore. > Interestingly, if I use vfunc_destroy (to override the destroy vfunc of the > widget) it gets called. This is the culprit: Signals.addSignalMethods(Searchbar.prototype); It needs to be changed to: Utils.addJSSignalMethods(Searchbar.prototype); We can not use the pure gjs signal machinery to connect to GObject signals (like 'destroy'). At the same time this class adds its own pure gjs signal ('activate-result'), which can not be used with the GObject signal machinery. So, we need to use a separate set of connect/disconnect methods for these two categories. The Application class is another example of such a hybrid class.
Review of attachment 308142 [details] [review]: ::: src/preview.js @@ +49,3 @@ const PreviewView = new Lang.Class({ Name: 'PreviewView', + Extends: Gtk.Stack, Now that PreviewView inherits from a GObject and also adds its own pure gjs signal ('search-changed'), we need to make the same change as comment 42.
Review of attachment 308167 [details] [review]: ::: src/presentation.js @@ +202,3 @@ function(widget, response) { this.emit('output-activated', null); })); Needs the same change as comment 42.
Created attachment 308690 [details] [review] searchbar: Use inheritance instead of composition
Review of attachment 308263 [details] [review]: ::: src/preview.js @@ +1008,3 @@ const PreviewFullscreenToolbar = new Lang.Class({ Name: 'PreviewFullscreenToolbar', + Extends: Gtk.Revealer, Same as comment 42
Review of attachment 308690 [details] [review]: Thanks, Alessandro. Looks good now.
Review of attachment 308554 [details] [review]: ++
Created attachment 308694 [details] [review] presentation,preview: separate JS signals from GObject signals Fallout from the composition->inheritance port
Created attachment 308695 [details] [review] edit,embed: remove unused lines
Created attachment 308699 [details] [review] searchbar: turn destroy method into a handler for 'destroy' signal
Review of attachment 308695 [details] [review]: ++
Review of attachment 308694 [details] [review]: Perfect. Thanks, Alessandro.
Review of attachment 308699 [details] [review]: ++
Created attachment 309607 [details] [review] notifications: Use inheritance instead of composition
Review of attachment 309607 [details] [review]: Looks good, based on the status of the other patches.
Review of attachment 309607 [details] [review]: Indeed, looks fine
Review of attachment 308130 [details] [review]: ::: src/manager.js @@ +182,3 @@ const BaseModel = new Lang.Class({ Name: 'BaseModel', + Extends: Gio.Menu, Extending the class in C or Javascript is very different, and there's not much that can be done AFAIK to prevent subclassing a GObject from Javascript. I think I'm comfortable that this patch won't break things.
(In reply to Cosimo Cecchi from comment #58) > Review of attachment 308130 [details] [review] [review]: > > ::: src/manager.js > @@ +182,3 @@ > const BaseModel = new Lang.Class({ > Name: 'BaseModel', > + Extends: Gio.Menu, > > Extending the class in C or Javascript is very different, and there's not > much that can be done AFAIK to prevent subclassing a GObject from Javascript. > I think I'm comfortable that this patch won't break things. Ok. Thanks for taking a look. I pushed the last two patches to master.