GNOME Bugzilla – Bug 789157
Add dialog-question-symbolic icon for missing files source
Last modified: 2017-11-05 04:56:59 UTC
Created attachment 361820 [details] Add dialog-question-symbolic icon for missing files source Missing files source doesn't have an associated icon. Added dialog-question-symbolic icon, as it seemed appropriate.
Created attachment 361821 [details] [review] source: Add dialog-question-symbolic icon for missing files source
Review of attachment 361821 [details] [review]: The missing files source already tries to set the icon name, it just does it in the wrong place. The existing call to rb_display_page_set_icon_name just needs to be moved from rb_missing_files_source_init to rb_missing_files_source_constructed. Incidentally the import errors source (in rb-import-errors-source.c) needs the same change. ::: sources/rb-missing-files-source.c @@ +278,3 @@ "hidden-when-empty", TRUE, NULL)); + rb_display_page_set_icon_name (RB_DISPLAY_PAGE (source), "dialog-question-symbolic"); Where possible, object setup should be done in GObject class methods rather than external functions like rb_missing_files_source_new, since objects can be created by calling g_object_new directly. All the other source classes call rb_display_page_set_icon_name in their _constructed method, which is called from g_object_new.
I've fixed the import errors source.
might as well do the other one then.
Created attachment 362114 [details] [review] sources: set the icon name in the right place For Grilo & Music sources.
Created attachment 362115 [details] [review] podcast: set the icon name in the right place For podcasts. Actually, it would be more right if we move the "icon-name" property in this patch high upto RBSource, so all subclasses don't need to call rb_display_page_set_icon_name everytime.
(In reply to gkrithi8 from comment #6) > Actually, it would be more right if we move the "icon-name" property in this > patch high upto RBSource, so all subclasses don't need to call > rb_display_page_set_icon_name everytime. rb_display_page_set_icon_name is just a helper function that sets the "icon" property. I don't want a property that just sets a different property.
Created attachment 362245 [details] [review] sources: set the icon name in the right place
Created attachment 362264 [details] [review] podcast: set the icon during construction
Review of attachment 362264 [details] [review]: pushed as commit 67d845842
Review of attachment 362245 [details] [review]: ::: plugins/ipod/rb-ipod-source.c @@ -721,3 @@ if (itdb_playlist_is_podcasts(playlist)) { priv->podcast_pl = playlist_source; - rb_display_page_set_icon_name (RB_DISPLAY_PAGE (playlist_source), "application-rss+xml-symbolic"); I don't think this change is really an improvement ::: sources/rb-library-source.c @@ +389,3 @@ g_object_unref (model); + /* Set icon before syncing child sources for childs to replicate the icons */ I'm not sure it really helps to explain this in a comment
(In reply to Jonathan Matthew from comment #11) > Review of attachment 362245 [details] [review] [review]: > > ::: plugins/ipod/rb-ipod-source.c > @@ -721,3 @@ > if (itdb_playlist_is_podcasts(playlist)) { > priv->podcast_pl = playlist_source; > - rb_display_page_set_icon_name (RB_DISPLAY_PAGE (playlist_source), > "application-rss+xml-symbolic"); > > I don't think this change is really an improvement PROP_ITDB_PLAYLIST is a construct only property. Hence, the change. Another call to "rb_ipod_static_playlist_source_new()" elsewhere in the code would require another "rb_display_page_set_icon_name", which can be avoided with this change. > ::: sources/rb-library-source.c > @@ +389,3 @@ > g_object_unref (model); > > + /* Set icon before syncing child sources for childs to replicate the icons > */ > > I'm not sure it really helps to explain this in a comment The idea was to put it somewhere, since it is actually a bug different from the other icon code changes. If there is no need, I'll remove that.
(In reply to gkrithi8 from comment #12) > > > > I don't think this change is really an improvement > > PROP_ITDB_PLAYLIST is a construct only property. Hence, the change. Another > call to "rb_ipod_static_playlist_source_new()" elsewhere in the code would > require another "rb_display_page_set_icon_name", which can be avoided with > this change. I'll move this change to _constructed() function. > > + /* Set icon before syncing child sources for childs to replicate the icons > > */ > > > > I'm not sure it really helps to explain this in a comment > > The idea was to put it somewhere, since it is actually a bug different from > the other icon code changes. If there is no need, I'll remove that. I'll split this change into a separate patch with the comment as commit message.
Created attachment 362367 [details] [review] sources: set the icon name in the right place
Created attachment 362368 [details] [review] library: set icon before syncing child sources for childs to replicate the icons
(In reply to gkrithi8 from comment #13) > (In reply to gkrithi8 from comment #12) > > > > > > I don't think this change is really an improvement > > > > PROP_ITDB_PLAYLIST is a construct only property. Hence, the change. Another > > call to "rb_ipod_static_playlist_source_new()" elsewhere in the code would > > require another "rb_display_page_set_icon_name", which can be avoided with > > this change. > > I'll move this change to _constructed() function. I still don't think this is an improvement. Nothing else is ever going to create ipod playlist sources, and if it does, it will probably have to set its own icons anyway. Setting properties (and so on) inside the GObject implementation rather than in _new() functions isn't a rule set in stone, it's just helpful when a class might be instantiated in different ways by unrelated code, which is not the case for anything inside a plugin.
(In reply to Jonathan Matthew from comment #16) > (In reply to gkrithi8 from comment #13) > > I'll move this change to _constructed() function. > > I still don't think this is an improvement. Nothing else is ever going to > create ipod playlist sources, and if it does, it will probably have to set > its own icons anyway. > > Setting properties (and so on) inside the GObject implementation rather than > in _new() functions isn't a rule set in stone, it's just helpful when a > class might be instantiated in different ways by unrelated code, which is > not the case for anything inside a plugin. I did think about that while making the changes for plugins. But, then felt it would at least be consistent with the rest of the code. I'm fine either ways and don't have a strong opinion here. Should the other plugin changes in this patch ( dacp / grilo ) reflect this idea too ?
The only change left to make in the patch ( after the above discussion ), as I understand, is to replace the below line ( in dacp plugin ) "icon", g_themed_icon_new ("phone-symbolic"), to rb_display_page_set_icon_name (RB_DISPLAY_PAGE (page), "phone-symbolic"); following the g_object_new() line. This change is required as "g_themed_icon_new" leaks a reference here. But, this pattern of initialization in args is followed for the "settings" property via "g_settings_get_child" in all places ( ~ 15 ). ./plugins/grilo/rb-grilo-source.c:457 ./plugins/mtpdevice/rb-mtp-source.c:600 ./plugins/mtpdevice/rb-mtp-source.c:601 ./plugins/audiocd/rb-audiocd-source.c:472 ./plugins/iradio/rb-iradio-source.c:452 ./plugins/android/rb-android-plugin.c:207 ./plugins/android/rb-android-plugin.c:208 ./plugins/daap/rb-daap-source.c:366 ./plugins/ipod/rb-ipod-source.c:631 ./plugins/ipod/rb-ipod-source.c:632 ./plugins/generic-player/rb-generic-player-plugin.c:195 ./plugins/generic-player/rb-generic-player-plugin.c:196 ./podcast/rb-podcast-main-source.c:81 ./podcast/rb-podcast-source.c:1003 ./sources/rb-library-source.c:425 Should this be left as it is (or) fix both ?
those are leaks, but they're not important, and they're not relevant to this bug.
Comment on attachment 362367 [details] [review] sources: set the icon name in the right place Patch not relevant per bug comments. Marking it as obsolete.
(In reply to gkrithi8 from comment #15) > Created attachment 362368 [details] [review] [review] > library: set icon before syncing child sources for childs to replicate the > icons can we please review and close this bug ?
Review of attachment 362264 [details] [review]: I looked at this again and realised it didn't do anything useful, so I reverted it.
Review of attachment 362368 [details] [review]: pushed as commit 3352c624d
Thanks !