After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 686832 - Move some API from MainView to MainViewGeneric
Move some API from MainView to MainViewGeneric
Status: RESOLVED FIXED
Product: libgd
Classification: Platform
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: libgd maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2012-10-25 01:41 UTC by Zeeshan Ali
Modified: 2012-10-29 20:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Move MainView.(un)select_all to MainViewGeneric (7.58 KB, patch)
2012-10-25 01:41 UTC, Zeeshan Ali
needs-work Details | Review
Move MainView::view-selection-changed to MainViewGeneric (3.63 KB, patch)
2012-10-25 01:42 UTC, Zeeshan Ali
needs-work Details | Review
Add MainViewGeneric.get_model() (3.74 KB, patch)
2012-10-29 19:18 UTC, Zeeshan Ali
committed Details | Review
Move MainView.(un)select_all to MainViewGeneric (4.29 KB, patch)
2012-10-29 19:18 UTC, Zeeshan Ali
committed Details | Review
Add MainViewGeneric::view-selection-changed signal (2.51 KB, patch)
2012-10-29 19:18 UTC, Zeeshan Ali
committed Details | Review

Description Zeeshan Ali 2012-10-25 01:41:53 UTC
See patches
Comment 1 Zeeshan Ali 2012-10-25 01:41:56 UTC
Created attachment 227211 [details] [review]
Move MainView.(un)select_all to MainViewGeneric

MainView still keeps these methods but only as proxies.
Comment 2 Zeeshan Ali 2012-10-25 01:42:01 UTC
Created attachment 227212 [details] [review]
Move MainView::view-selection-changed to MainViewGeneric

MainView keeps the signal but as proxy.
Comment 3 Cosimo Cecchi 2012-10-26 02:05:16 UTC
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.
Comment 4 Cosimo Cecchi 2012-10-26 02:10:43 UTC
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.
Comment 5 Zeeshan Ali 2012-10-29 16:03:12 UTC
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.
Comment 6 Zeeshan Ali 2012-10-29 16:06:22 UTC
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.
Comment 7 Cosimo Cecchi 2012-10-29 16:37:12 UTC
(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
Comment 8 Cosimo Cecchi 2012-10-29 16:39:33 UTC
(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.
Comment 9 Zeeshan Ali 2012-10-29 18:35:49 UTC
(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..
Comment 10 Zeeshan Ali 2012-10-29 19:18:04 UTC
Created attachment 227574 [details] [review]
Add MainViewGeneric.get_model()
Comment 11 Zeeshan Ali 2012-10-29 19:18:20 UTC
Created attachment 227575 [details] [review]
Move MainView.(un)select_all to MainViewGeneric

MainView still keeps these methods but only as proxies.
Comment 12 Zeeshan Ali 2012-10-29 19:18:28 UTC
Created attachment 227576 [details] [review]
Add MainViewGeneric::view-selection-changed signal
Comment 13 Cosimo Cecchi 2012-10-29 19:21:47 UTC
Review of attachment 227574 [details] [review]:

Looks good
Comment 14 Cosimo Cecchi 2012-10-29 19:22:18 UTC
Review of attachment 227575 [details] [review]:

++
Comment 15 Cosimo Cecchi 2012-10-29 19:22:48 UTC
Review of attachment 227576 [details] [review]:

++
Comment 16 Zeeshan Ali 2012-10-29 20:16:51 UTC
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