GNOME Bugzilla – Bug 686832
Move some API from MainView to MainViewGeneric
Last modified: 2012-10-29 20:17:00 UTC
See patches
Created attachment 227211 [details] [review] Move MainView.(un)select_all to MainViewGeneric MainView still keeps these methods but only as proxies.
Created attachment 227212 [details] [review] Move MainView::view-selection-changed to MainViewGeneric MainView keeps the signal but as proxy.
Review of attachment 227211 [details] [review]: ::: libgd/gd-main-view-generic.c @@ +125,3 @@ + iface = GD_MAIN_VIEW_GENERIC_GET_IFACE (self); + + (* iface->select_all) (self); Why do these need to be interface methods? The implementation in both views is identical so you can just move the calls here I think.
Review of attachment 227212 [details] [review]: ::: libgd/gd-main-view.c @@ +433,3 @@ } + g_signal_emit (generic, signals[VIEW_SELECTION_CHANGED], 0); I think it's fine to only emit this signal on the main view itself in this case, since this code is (currently) only used together with GdMainView.
Review of attachment 227211 [details] [review]: ::: libgd/gd-main-view-generic.c @@ +125,3 @@ + iface = GD_MAIN_VIEW_GENERIC_GET_IFACE (self); + + (* iface->select_all) (self); not exactly identical. They both need to call different methods to get the underlying model.
Review of attachment 227212 [details] [review]: ::: libgd/gd-main-view.c @@ +433,3 @@ } + g_signal_emit (generic, signals[VIEW_SELECTION_CHANGED], 0); 1. Not in Boxes, we are not using main view (yet). 2. It doesn't hurt to keep modules independent as much as possible/makes sense.
(In reply to comment #5) > not exactly identical. They both need to call different methods to get the > underlying model. I'd rather make get_model pluggable then, rather than select_all/unselect_all
(In reply to comment #6) > Review of attachment 227212 [details] [review]: > > ::: libgd/gd-main-view.c > @@ +433,3 @@ > } > > + g_signal_emit (generic, signals[VIEW_SELECTION_CHANGED], 0); > > 1. Not in Boxes, we are not using main view (yet). > 2. It doesn't hurt to keep modules independent as much as possible/makes sense. I agree it's good to keep modules independent as much as possible, that's the reason I don't really like emitting the signal on the generic in this case; if the main view is not used, the DnD code this signal is needed for won't run at all, so it's not useful to emit it on the generic.
(In reply to comment #7) > (In reply to comment #5) > > > not exactly identical. They both need to call different methods to get the > > underlying model. > > I'd rather make get_model pluggable then, rather than select_all/unselect_all Agreed. (In reply to comment #8) > (In reply to comment #6) > > Review of attachment 227212 [details] [review] [details]: > > > > ::: libgd/gd-main-view.c > > @@ +433,3 @@ > > } > > > > + g_signal_emit (generic, signals[VIEW_SELECTION_CHANGED], 0); > > > > 1. Not in Boxes, we are not using main view (yet). > > 2. It doesn't hurt to keep modules independent as much as possible/makes sense. > > I agree it's good to keep modules independent as much as possible, that's the > reason I don't really like emitting the signal on the generic in this case; if > the main view is not used, the DnD code this signal is needed for won't run at > all, so it's not useful to emit it on the generic. Ah yes, didn't think it through. I'll change this as well..
Created attachment 227574 [details] [review] Add MainViewGeneric.get_model()
Created attachment 227575 [details] [review] Move MainView.(un)select_all to MainViewGeneric MainView still keeps these methods but only as proxies.
Created attachment 227576 [details] [review] Add MainViewGeneric::view-selection-changed signal
Review of attachment 227574 [details] [review]: Looks good
Review of attachment 227575 [details] [review]: ++
Review of attachment 227576 [details] [review]: ++
Attachment 227574 [details] pushed as 4178979 - Add MainViewGeneric.get_model() Attachment 227575 [details] pushed as 52c4268 - Move MainView.(un)select_all to MainViewGeneric Attachment 227576 [details] pushed as 43134a3 - Add MainViewGeneric::view-selection-changed signal