GNOME Bugzilla – Bug 754165
Replace uses of "instanceof" with virtual functions
Last modified: 2015-09-29 17:45:14 UTC
When adding support for new remote services, it helps to have all the service-specific code in one place instead of being scattered across the code base.
Created attachment 310083 [details] [review] documents, properties: Replace uses of "instanceof" with virtual functions When adding support for new remote services, it helps to have all the service-specific code in one place instead of being scattered across the code base.
Review of attachment 310083 [details] [review]: ::: src/documents.js @@ +782,3 @@ + use_markup: true, + halign: Gtk.Align.START, + ellipsize: Pango.EllipsizeMode.END }); The "const Pango = imports.gi.Pango;" line has to be moved to this file. ::: src/properties.js @@ +168,1 @@ This newline can now be removed.
Created attachment 310095 [details] [review] documents, properties: Replace uses of "instanceof" with virtual functions When adding support for new remote services, it helps to have all the service-specific code in one place instead of being scattered across the code base.
Review of attachment 310095 [details] [review]: ::: src/properties.js @@ +165,2 @@ // Source value + this._sourceData = doc.getSourceWidget() Is there a reason why we don't get back a URI and a display name from the source specific functions? And we'd create a label in the UI code.
Created attachment 311644 [details] [review] documents, properties: Replace uses of "instanceof" with virtual functions When adding support for new remote services, it helps to have all the service-specific code in one place instead of being scattered across the code base.
Review of attachment 311644 [details] [review]: Looks good, feel free to push after freeze with these style nits fixed ::: src/documents.js @@ +774,3 @@ + if (this.collection) { + return [ null, this.sourceName ]; + } else { No need for an else block @@ +1020,3 @@ + let source = Application.sourceManager.getItemById(this.resourceUrn); + let account = source.object.get_account(); + let presentation_identity = account.presentation_identity; Please use camel case for variables ::: src/properties.js @@ +173,3 @@ + } else { + // Collections don't have links + this._sourceData = new Gtk.Label({ label: name, halign: Gtk.Align.START }); One property per line
Created attachment 312049 [details] [review] documents, properties: Replace uses of "instanceof" with virtual functions When adding support for new remote services, it helps to have all the service-specific code in one place instead of being scattered across the code base.
Attachment 312049 [details] pushed as 14acf23 - documents, properties: Replace uses of "instanceof" with virtual functions
(In reply to Bastien Nocera from comment #4) > Review of attachment 310095 [details] [review] [review]: > > ::: src/properties.js > @@ +165,2 @@ > // Source value > + this._sourceData = doc.getSourceWidget() > > Is there a reason why we don't get back a URI and a display name from the > source specific functions? Because prior to commit 44780682c56ee43 (bug 754149), we weren't always creating a GtkLabel. However, as you have shown, it is not convincing enough as a reason. :)