Created attachment 306403 [details] [review]
app-window: Put collection view in stack

Move the window's collection view in a Gtk.Stack.

This is needed to allow to switch between multiple collection views to
offer multiple ready-made searches.

Created attachment 306404 [details] [review]
collection-toolbar: Add collection view switcher

Add a Gtk.StackSwitcher as the collection toolbar's custom title widget,
allowing to switch between the window's multiple collection views.

This is needed to allow to switch between multiple collection views to
offer multiple ready-made searches.

Created attachment 306405 [details] [review]
collection: Add CollectionFilterFunc delegate

Add the CollectionFilterFunc delegate, allowing to filter a single
collection element.

This will be used in next commits to filter the visible elements of a
collection and is needed to offer multiple ready-made searches.

Created attachment 306406 [details] [review]
collection-view: Add 'item_visible_func' prop

Add the 'item_visible_func' delegate allowing to filter visible items of
the collection view.

This is needed to filter the visible elements of a collection and to
offer multiple ready-made searches.

Created attachment 306407 [details] [review]
machine: Add is_local()

Add the abstract is_local() mthod to the Machine class and its
subclasses.

This will be needed in the next commit to filter local and remote
machines.

Created attachment 306408 [details] [review]
app-window: Add local and remote pages

Add collection pages showing only the local boxes and the remote boxes.

This is needed to quickly filter local or remote boxes.

Review of attachment 306407 [details] [review]:

Description makes it sound like we are adding abstract method to subclasses too. "its subclasses" -> "implement it in subclasses".

::: src/libvirt-machine.vala
@@ +360,3 @@
     }
 
+    public override bool is_local () {

Just FYI override is only needed for virtual methods/properties.

@@ +362,3 @@
+    public override bool is_local () {
+        // If the URI is prefixed by "qemu" then the machine is virtual and hence local.
+        return /^qemu/i.match (source.uri);

That's not correct I think. The usual remote URI is prefixed "qemu+ssh" IIRC. You should check for '^qemu(\+unix)*://(system)|(session)

::: src/machine.vala
@@ +379,3 @@
     }
 
+    public abstract bool is_local ();

This should be property. is_on() and friends should be properties too but we can fix that later and not a change for this patch anyway.

::: src/ovirt-machine.vala
@@ +78,3 @@
 
+    public override bool is_local () {
+        return true;

ovirt machines could be local too. I think you want to make the property virtual instead in Machine and move the implementation from RemoteMachine to base Machine class. That way LibvirtMachine can call base implementation when URI doesn't match the usual local URIs.

::: src/remote-machine.vala
@@ +100,3 @@
+    public override bool is_local () {
+        // If the URI is in the 127.0.0.0 address block, then it is local
+        return /^spice:\/\/127\./i.match (source.uri);

it could also be using "localhost".

Review of attachment 306403 [details] [review]:

Didn't we decide on simply using filters? We're already using filters for searches so we probably want to keep using them here as well. We can then re-use same code for bug#736972.

Review of attachment 306403 [details] [review]:

Didn't we decide on simply using filters? We're already using filters for searches so we probably want to keep using them here as well. We can then re-use same code for bug#736972.

Created attachment 306484 [details] [review]
collection: Add getter to CollectionFilter.text

This allow notifications on the CollectionFilter.text prop and will be
needed in next commits to move the use of CollectionFilter from App to
CoolectionView.

Created attachment 306485 [details] [review]
app: Move collection filter to CollectionView

This is needed by next commits to be able to have one collection filter
per collection view, and hence,to be able to filter each collection view
individually.

Created attachment 306486 [details] [review]
app-window: Put collection view in stack

Move the window's collection view in a Gtk.Stack.

This is needed to allow to switch between multiple collection views to
offer multiple ready-made searches.

Created attachment 306487 [details] [review]
collection-toolbar: Add collection view switcher

Add a Gtk.StackSwitcher as the collection toolbar's custom title widget,
allowing to switch between the window's multiple collection views.

This is needed to allow to switch between multiple collection views to
offer multiple ready-made searches.

Created attachment 306488 [details] [review]
collection: Add CollectionFilterFunc delegate

Add the CollectionFilterFunc delegate, allowing to filter a single
collection element.

This will be used in next commits to filter the visible elements of a
collection and is needed to offer multiple ready-made searches.

Created attachment 306489 [details] [review]
collection-filter: Add 'filter_func' prop

Add the 'filter_func' delegate allowing to customize a collection filter

This is needed to filter the visible elements of a collection with more
precision and to offer multiple ready-made searches.

Created attachment 306490 [details] [review]
machine: Add 'is_local' prop

This will be needed in the next commit to filter local and remote
machines.

Created attachment 306491 [details] [review]
app-window: Add local and remote pages

Add collection pages showing only the local boxes and the remote boxes.

This is needed to quickly filter local or remote boxes.

Review of attachment 306484 [details] [review]:

ack

::: src/collection.vala
@@ +62,2 @@
     public string text {
+        // The getter is necessary for notifications to work.

This would be true for other props too but since we'll not have this comment to all getters, let's not have it here either. :)

Review of attachment 306485 [details] [review]:

* How about "App.filter -> CollectionView" as shortlog?

* Description should mention (first) that we'll have multiple collection views. Why are going to have multiple views again? Can't we just apply different filters to existing view, like it is done right now?

Review of attachment 306485 [details] [review]:

To switch between "filters" the correct widget to use is a Gtk.StackSwitcher, which switch between the pages of a stack as its buttons are pressed.

So to use this stack switcher, the best way is to have a stack of the different views we want, and hence, we need different views, each one filtering the collection in its own way.

Review of attachment 306485 [details] [review]:

That's a bit of circular argument IMO. The stackswitcher is for switching between stacks so if you don't have a stack, it's not the appropriate widget to use. You have GtkButtonBox and GtkToggleButton. I think it's best to re-use existing filtering code and not complicate things with multiple views just to be able to use the most conventient widget for switching.

Review of attachment 306485 [details] [review]:

I didn't know about GtkButtonBox, I'll test that.

Created attachment 306597 [details] [review]
collection: Add getter to CollectionFilter.text

This allow notifications on the CollectionFilter.text prop and will be
needed in next commits to move the use of CollectionFilter from App to
CollectionView.

Created attachment 306598 [details] [review]
App.filter -> CollectionView

This is needed to make CollectionView more self contained and hence make
the code easier to understand.

Created attachment 306599 [details] [review]
collection: Add CollectionFilterFunc delegate

Add the CollectionFilterFunc delegate, allowing to filter a single
collection element.

This will be used in next commits to make CollectionFilter more flexible
ans more powerful and is needed to offer multiple ready-made searches.

Created attachment 306600 [details] [review]
collection-filter: Add 'filter_func' prop

Add the 'filter_func' delegate allowing to customize a collection
filter.

This will be used to filter the visible elements of a collection with
more precision and is needed to offer multiple ready-made searches.

Created attachment 306601 [details] [review]
Add Switcher class

Add Switcher class, a widget offering a box of linked toggle buttons
allowing the user to choose one option out of several.

This will be used in the next commit as a base widget for a collection
filter switcher and is needed to offer multiple ready-made searches.

Created attachment 306602 [details] [review]
Add CollectionFilterSwitcher class

Add CollectionFilterSwitcher class, a widget offering a box of linked
toggle buttons allowing to customize a CollectionFilter by letting the
user switch between multiple filtering functions.

This will be used in the next commit as header for the main window and
is needed to offer multiple ready-made searches.

Created attachment 306603 [details] [review]
collection-toolbar: Add collection filter switcher

Add a CollectionFilterSwitcher as the collection toolbar's custom title
widget, allowing to switch between multiple filtering functions.

This is needed to allow to switch between multiple ready-made searches.

Created attachment 306604 [details] [review]
machine: Add 'is_local' prop

This will used in the next commit to filter local and remote machines
and is needed to offer multiple ready-made searches.

Created attachment 306605 [details] [review]
collection-toolbar: Add local and remote filters

Add collection filters showing only the local boxes and the remote
boxes.

This is needed to quickly filter local or remote boxes as ready-made
searches.

Review of attachment 306597 [details] [review]:

allow -> allows

Review of attachment 306598 [details] [review]:

ack

Review of attachment 306599 [details] [review]:

* "ans" -> "and"
* Don't really see a single new delegate addition as a logical change. Maybe merge with following patch?

Review of attachment 306600 [details] [review]:

::: src/collection.vala
@@ +58,2 @@
 private class Boxes.CollectionFilter: GLib.Object {
+    // This is necessary to notify that filter_func has changed as delegates currently can't send notifications

Link to bug would be nice.

@@ +85,3 @@
+    construct {
+        notify["filter-func"].connect_after (() => {
+            print ("MAJ FILTER FUNC\n");

Excusez-moi? :)

@@ +96,3 @@
         }
+
+        return filter_func != null ? filter_func (item) : true;

This completely ignores the code above it to match against name.

Review of attachment 306601 [details] [review]:

::: data/ui/switcher.ui
@@ +1,3 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<interface>
+  <template class="BoxesSwitcher" parent="GtkButtonBox">

* For all new UI code, I'd very much prefer defining the UI in the UI file. You can attach buttons to states/numbers etc from implementation file.

OR

* if we are not going to really define the widget here, I'd rather we don't use template at all. I think you can use widgets in UI files w/o having to have them declared in a UI file.

::: src/switcher.vala
@@ +1,3 @@
+// This file is part of GNOME Boxes. License: LGPLv2+
+
+[GtkTemplate (ui = "/org/gnome/Boxes/ui/switcher.ui")]

I don't think we'll be using this class for any other purpose so I'd make it specific to use case in hand and define enums for different states.

@@ +8,3 @@
+    private HashTable<Gtk.ToggleButton, int> button_numbers;
+
+    public signal void toggled (int button);

signals before (especially) private fields/props.

Review of attachment 306600 [details] [review]:

::: src/collection.vala
@@ +58,2 @@
 private class Boxes.CollectionFilter: GLib.Object {
+    // This is necessary to notify that filter_func has changed as delegates currently can't send notifications

I chatted with lethalman and apparently this is not a bug but by design, because delegates are hidden complex values. I'll update the comment to make it more clear.

@@ +96,3 @@
         }
+
+        return filter_func != null ? filter_func (item) : true;

I'm not sure to understand what you mean. The function checks whether the item match at the same time the searched terms (if any) and the custom filtering function (if any).

If the item doesn't correspond to the terms, it is hidden, else if there is no custom search function, it is visible, and otherwise it depends on the custom filtering function.

Would you prefer something like the following to replace the last line?

| if (filter_func != null)
|     return filter_func (item);
| 
| return true;

Created attachment 307797 [details] [review]
collection-filter: Add 'filter_func' prop

Add the 'filter_func' delegate allowing to customize a collection
filter.

This will be used to filter the visible elements of a collection with
more precision and is needed to offer multiple ready-made searches.

Created attachment 307798 [details] [review]
machine: Add 'is_local' prop

This will used in the next commit to filter local and remote machines
and is needed to offer multiple ready-made searches.

Created attachment 307799 [details] [review]
Add CollectionFilterSwitcher widget

Add CollectionFilterSwitcher class, a widget offering a box of linked
toggle buttons allowing to customize a CollectionFilter by letting the
user switch between multiple filtering functions.

This will be used in the next commit as header for the main window and
is needed to offer multiple ready-made searches.

Created attachment 307800 [details] [review]
collection-toolbar: Add collection filter switcher

Add a CollectionFilterSwitcher as the collection toolbar's custom title
widget, allowing to switch between multiple filtering functions.

This allows to switch between multiple ready-made searches.

Review of attachment 306600 [details] [review]:

::: src/collection.vala
@@ +96,3 @@
         }
+
+        return filter_func != null ? filter_func (item) : true;

Nah, i probably misunderstood. Having said that, you want to put the condition in brackets.

Review of attachment 306600 [details] [review]:

::: src/collection.vala
@@ +96,3 @@
         }
+
+        return filter_func != null ? filter_func (item) : true;

I still did the change in the last patches I attached, it's easier to read. =)

Review of attachment 307797 [details] [review]:

looks good

::: src/collection.vala
@@ +58,3 @@
 private class Boxes.CollectionFilter: GLib.Object {
+    // This is necessary to notify that filter_func has changed as delegates are, by design, not registered as
+    // properties because they are hidden complex values (they come with a target value), and hence can't be notified.

No need for such long explaination, i think we do the same in a few other places too and probably would do in others too. Just say "delegate properties can't have notify signals" so something like that.

@@ +93,3 @@
+        if (filter_func != null)
+            return filter_func (item);
+

Hmm.. when you asked me if we want to go down the inheritance route or delegate, I was thinking about the model and didn't think about this CollectionFilter class. :( So in retrospect, inheritance would have been a more elegent solution here. I won't ask you to change this now but if you would, that would be awesome. :)

Review of attachment 307798 [details] [review]:

::: src/machine.vala
@@ +63,3 @@
+
+            // If the URI'domain is localhost, then it is local
+            if (/^spice:\/\/localhost/i.match (source.uri))

* I don't think Machine class should know anything about spice. I think you want this definition in RemoteMachine and also check for vnc.

* You can put them in the same regex or if that's complex than at least in the same if condition.

Review of attachment 307799 [details] [review]:

"allowing to customize a CollectionFilter by letting the
user switch between multiple filtering functions" -> "allowing user to switch different filtering functions on a provided CollectionFilter"

" and is needed to offer" -> " to implement quick ready-made search toggles"?

::: src/collection-filter-switcher.vala
@@ +13,3 @@
+    private Gtk.ToggleButton remote_button;
+
+    public CollectionFilter filter { get; construct set; }

if it's construct, i'd expect it to be set by a construction method but i don't see one.

@@ +23,3 @@
+        });
+
+        if (filter != null)

filter should never be null.

@@ +50,3 @@
+    [GtkCallback]
+    private void update (Gtk.ToggleButton button) {
+        if (updating)

This all seems synchronous, why do we need this updating variable?

@@ +56,3 @@
+            button.active = true;
+            return;
+        }

This whole code related to update (what's updating?) is hard to follow. I especially don't get why there is going to be an active_button but active_button.active won't be set?

Review of attachment 307800 [details] [review]:

Looks good.

::: src/collection-toolbar.vala
@@ +33,3 @@
         App.app.notify["main-window"].connect (ui_state_changed);
+
+        filter_switcher.filter = window.view.filter;

Ah now I understand why you don't have a construction method defined in FilterSwitcher class. setup_ui() pattern was excactly invented by me for consistent handing of issue of not able to pass params to constructor to objects created from UI files. i-e I think you should instead add a setup_ui() method to FilterSwitcher class.

Created attachment 309041 [details] [review]
machine: Add 'is_local' prop

This will used in the next commit to filter local and remote machines
and is needed to offer multiple ready-made searches.

Created attachment 309042 [details] [review]
Add CollectionFilterSwitcher widget

Add CollectionFilterSwitcher class, a widget offering a box of linked
toggle buttons allowing user to switch different filtering functions on
a provided CollectionFilter.

This will be used in the next commit as header for the main window to
implement quick ready-made search toggles.

Created attachment 309043 [details] [review]
collection-toolbar: Add collection filter switcher

Add a CollectionFilterSwitcher as the collection toolbar's custom title
widget, allowing to switch between multiple filtering functions.

This allows to switch between multiple ready-made searches.

Created attachment 309102 [details] [review]
machine: Add 'is_local' prop

This will used in the next commit to filter local and remote machines
and is needed to offer multiple ready-made searches.

Created attachment 309103 [details] [review]
Add CollectionFilterSwitcher widget

Add CollectionFilterSwitcher class, a widget offering a box of linked
toggle buttons allowing user to switch different filtering functions on
a provided CollectionFilter.

This will be used in the next commit as header for the main window to
implement quick ready-made search toggles.

Created attachment 309104 [details] [review]
collection-toolbar: Add collection filter switcher

Add a CollectionFilterSwitcher as the collection toolbar's custom title
widget, allowing to switch between multiple filtering functions.

This allows to switch between multiple ready-made searches.

Created attachment 309134 [details] [review]
machine: Add 'is_local' prop

This will used in the next commit to filter local and remote machines
and is needed to offer multiple ready-made searches.

Created attachment 309135 [details] [review]
Add CollectionFilterSwitcher widget

Add CollectionFilterSwitcher class, a widget offering a box of linked
toggle buttons allowing user to switch different filtering functions on
a provided CollectionFilter.

This will be used in the next commit as header for the main window to
implement quick ready-made search toggles.

Created attachment 309136 [details] [review]
collection-toolbar: Add collection filter switcher

Add a CollectionFilterSwitcher as the collection toolbar's custom title
widget, allowing to switch between multiple filtering functions.

This allows to switch between multiple ready-made searches.

Review of attachment 309134 [details] [review]:

ack

Created attachment 309173 [details] [review]
Add CollectionFilterSwitcher widget

Add CollectionFilterSwitcher class, a widget offering a box of linked
toggle buttons allowing user to switch different filtering functions on
a provided CollectionFilter.

This will be used in the next commit as header for the main window to
implement quick ready-made search toggles.

Created attachment 309174 [details] [review]
collection-toolbar: Add collection filter switcher

Add a CollectionFilterSwitcher as the collection toolbar's custom title
widget, allowing to switch between multiple filtering functions.

This allows to switch between multiple ready-made searches.

Review of attachment 309173 [details] [review]:

::: src/collection-filter-switcher.vala
@@ +27,3 @@
+
+        if (filter == null)
+            warning ("CollectionFilterSwitcher: the 'filter' prop must be set at construction time");

warnings are for runtime issue and this would be programmer error. Why doesn't setup_ui simply take a non-nullable filter and ensures that it's set instead of emitting a warning?

@@ +52,3 @@
+
+    [GtkCallback]
+    private void update (Gtk.ToggleButton button) {

its still not clear at first sight what this function does exactly and what's the 'button' arg about. Seems like to me that this function would be better called "activate_button"?

Review of attachment 309174 [details] [review]:

ack

Review of attachment 309173 [details] [review]:

::: src/collection-filter-switcher.vala
@@ +27,3 @@
+
+        if (filter == null)
+            warning ("CollectionFilterSwitcher: the 'filter' prop must be set at construction time");

Yes, that would be better.

@@ +52,3 @@
+
+    [GtkCallback]
+    private void update (Gtk.ToggleButton button) {

Yes, "activate_button" would be better.

Pushed all with some changes.

Attachment 306597 [details] pushed as 5236681 - collection: Add getter to CollectionFilter.text
Attachment 306598 [details] pushed as 7e0652e - App.filter -> CollectionView
Attachment 307797 [details] pushed as 0a7a135 - collection-filter: Add 'filter_func' prop
Attachment 309134 [details] pushed as 5bae0d7 - machine: Add 'is_local' prop
Attachment 309173 [details] pushed as bcede69 - Add CollectionFilterSwitcher widget
Attachment 309174 [details] pushed as 9bd112b - collection-toolbar: Add collection filter switcher

*** Bug 738554 has been marked as a duplicate of this bug. ***