GNOME Bugzilla – Bug 733252
Offer list view
Last modified: 2016-09-20 08:16:03 UTC
If boxes are headless machines, thumbnails in collection view won't be very useful so maybe it'd be more appropriate to have a list-view option.
I agree. For a tentative design: Notes and Files are doing that in a different way from each other. - Notes has one button to toggle the view - Files has two buttons to activate the appropriate view while one is active. Let's consult the designers for what's better and if we can standardize that among gnome apps. (I have this on my TODO ;)) I think we should have these views as some generic objects internally so baedert can use it for showing snapshots too (or the other way around), assuming he uses this kind of list view.
*** Bug 733990 has been marked as a duplicate of this bug. ***
Created attachment 298824 [details] List view prototype I started working on list view, it's working well so far but it's far from perfect yet. Do you have an idea of what other columns should be displayed in list view? I thought of: - favs (heart icon) - OS icon - local or remote - system monitor - URI - IP I still need to correct some runtime warnings that I don't understand yet, to remove the heart emblem from the miniatures in list mode and to make Boxes remember the last used mode.
(In reply to Adrien Plazas from comment #3) > Created attachment 298824 [details] > List view prototype Cool, Really good start. > Do you have an idea of what other columns should be displayed in list view? > I thought of: > - favs (heart icon) > - OS icon > - local or remote > - system monitor > - URI > - IP I'd go for these only for now: > - system monitor > - IP > - OS icon While OS icon should probably only be shown if machine is shutoff and in place of thumbnail. I think we should do that in icon view too. > to remove the heart emblem from the miniatures in list mode I don't think we want that. Even if we decide to show favorite status here, you'll want a separate column for that. > and to make > Boxes remember the last used mode. That would be nice but I don't think its needed in the first iteration.
First mockup: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/boxes/wires/list-view.png
Created attachment 307760 [details] [review] collection-view: Extract ICollectionView interface The ICollectionView interface is extracted from CollectionView so it is easier to handle different types of collection views. This is needed as the CollectionListView widget will be added by a subsequent commit.
Created attachment 307761 [details] [review] collection: Make populate() take ICollectionView param With this, any collection view can easily be populated. This is needed as the CollectionListView widget will be added by a subsequent commit and will need to be populated.
Created attachment 307762 [details] [review] app-window: Add 'current_view' prop This allow to easily access the current collection view of the window. This is needed as a CollectionListView widget will be added alongside the current CollectionView 'view' widget in a subsequent commit.
Created attachment 307763 [details] [review] Add MachineListThumbnail widget Allow to display a thumbnail for a list view from a machine. This is needed by CollectionItemListView which will be added in next commit and utimately this is needed to offer a list view.
Created attachment 307764 [details] [review] Add CollectionItemListView widget Represent a row of a list view from a machine. This is needed by CollectionListView which will be added in next commit and utimately this is needed to offer a list view.
Created attachment 307765 [details] [review] Add CollectionListView widget This will be used in next commit by the window, and it is utimately needed to offer a list view.
Created attachment 307766 [details] [review] app-window: Add list_view Add a CollectionListView alongside the current CollectionView. This is needed to offer a list view.
Created attachment 307767 [details] [review] app-window: Add 'view_type' prop Allow to switch the view type of a window This is needed to offer a list view.
Created attachment 307768 [details] [review] collection-toolbar: Add list view buttons Add buttons to the collection toolbar to switch between the collection view's list and icon modes. This is needed to offer a list view.
Review of attachment 307760 [details] [review]: Patch itself is good. * "Extract" -> "Add". * Don't use past tense for changes in the patch. * "This is needed" contradicts with previous para where you say it only makes things easier. As a homework to become even better commit writer and be able to think out this simple pattern of "This is needed cause..", I want you do try to combine the two paragraphs. HINT: in first para you already justify the change. :) ::: src/i-collection-view.vala @@ +4,3 @@ + public abstract void add_item (CollectionItem item); + public abstract List<CollectionItem> get_selected_items (); + public abstract void remove_item (CollectionItem item); nitpick: I'd put remove_item just below add_item.
Review of attachment 307761 [details] [review]: I think this should be squashed with previous patch.
Review of attachment 307762 [details] [review]: ::: src/app-window.vala @@ +94,3 @@ + public ICollectionView current_view { + get { return view; } + } I don't think there is much value in addition of 'current_' to name. Let's change the name of 'view' to 'collection_view' and make it private and expose this new prop as 'view' so nothing changes for other classes.
Review of attachment 307763 [details] [review]: ::: src/machine-list-thumbnail.vala @@ +2,3 @@ + +[GtkTemplate (ui = "/org/gnome/Boxes/ui/machine-list-thumbnail.ui")] +private class Boxes.MachineListThumbnail: Gtk.DrawingArea { A completely new class with lots of code? Looking at the mockup, I don't see enough differences between listview and collection thumbnail. @@ +36,3 @@ + + public MachineListThumbnail (Machine machine) { + Object (machine: machine); Convention is to use 'this.machine = machine' syntax.
Review of attachment 307764 [details] [review]: * Description sounds weird ("from a machine?"). * Isn't CollectionListView *is* the list view? Last para makes it sound like they are not quite the same. ::: src/collection-item-list-view.vala @@ +46,3 @@ + + public CollectionItemListView (CollectionItem item) { + Object (item: item); this.item = item; @@ +63,3 @@ + update_info (); + update_state (); + } I don't like having both construction method and 'construct' unless it's really needed. Can we not do all this from construction method above?
Review of attachment 307763 [details] [review]: ::: src/machine-list-thumbnail.vala @@ +2,3 @@ + +[GtkTemplate (ui = "/org/gnome/Boxes/ui/machine-list-thumbnail.ui")] +private class Boxes.MachineListThumbnail: Gtk.DrawingArea { The big difference is that MachineThumbnailer is a pixbuf factory (because we can't do better with Gd.MainView), while MachineListThumbnail is a widget (with all the things a widget have, like a style which can be defined in CSS, a state...), for example allowing us to have a special style when the window is unfocused. That's why I suffixed the first with -er (it produces thumbnails) but not the second (it _is_ the thumbnail). In the end, the classes are way more different than they look at first glance, and I'm not sure trying to mix both would end up nicely. I hoped that once the list view would be merged, we could really easily add a new icon view, maybe adding a sister MachineIconThumbnail widget and removing MachineThumbnailer. What do you think?
Review of attachment 307762 [details] [review]: ::: src/app-window.vala @@ +94,3 @@ + public ICollectionView current_view { + get { return view; } + } The problem is that sometimes the other classes have to access the current view, and sometimes they have to access all the views (which obviously was the same when there was only one view). I'll just add some foreach_view() to AppWindow to treat the second case.
Created attachment 308663 [details] [review] collection-view: Add ICollectionView interface Extract the ICollectionView interface from CollectionView so it is easier to handle the current collection view and the CollectionListView which will be added by a subsequent commit.
Created attachment 308664 [details] [review] app-window: Add foreach_view() Add foreach_view(), allowing to easily access all the views of the window. This is needed as a CollectionListView widget will be added alongside the 'view' widget in a subsequent commit.
Created attachment 308665 [details] [review] app-window: Add 'view' property Rename the 'view' attribute to 'collection_view', making it private in the process, and add the 'view' property, allowing to easily access the current collection view of the window. This is needed as a CollectionListView widget will be added alongside the 'collection_view' widget in a subsequent commit.
Created attachment 308666 [details] [review] Add MachineListThumbnail widget Allow to display a thumbnail for a list view from a machine. This is needed by CollectionItemListView which will be added in next commit and utimately this is needed to offer a list view.
Created attachment 308667 [details] [review] Add CollectionItemListView widget Represent a row of a list view from a machine. This is needed by CollectionListView which will be added in next commit and utimately this is needed to offer a list view.
Created attachment 308668 [details] [review] Add CollectionListView widget This will be used in next commit by the window, and it is utimately needed to offer a list view.
Created attachment 308669 [details] [review] app-window: Add list_view Add a CollectionListView alongside the current CollectionView. This is needed to offer a list view.
Created attachment 308670 [details] [review] app-window: Add 'view_type' prop Allow to switch the view type of a window This is needed to offer a list view.
Created attachment 308671 [details] [review] collection-toolbar: Add list view buttons Add buttons to the collection toolbar to switch between the collection view's list and icon modes. This is needed to offer a list view.
Review of attachment 307763 [details] [review]: ::: src/machine-list-thumbnail.vala @@ +2,3 @@ + +[GtkTemplate (ui = "/org/gnome/Boxes/ui/machine-list-thumbnail.ui")] +private class Boxes.MachineListThumbnail: Gtk.DrawingArea { I understand the difference between this thumnail widget and thumbnailer. Thing is that you just introduced that class and knowing that summer (of code) is ending soon and we are approaching 3.18 very fast, I'm afraid that we'll be stuck with this inconsistency for much longer than I'd want to. I don't think you necessarily need to handle the thumbnail redrawing, just like you don't with icon view case. I'd recommend going forward with Thumbnailer approach for now and if you need this class, how about just derive from Gtk.Image and let that handle redrawing. As for not enough commonality betwween this and Thumbnailer, I don't see what you mean since you are drawing the same image depending on the state of the machine. Even your method names here are the same.
Created attachment 308916 [details] [review] collection-view: Add ICollectionView interface Extract the ICollectionView interface from CollectionView so it is easier to handle the current collection view and the CollectionListView which will be added by a subsequent commit.
Created attachment 308917 [details] [review] app-window: Add foreach_view() Add foreach_view(), allowing to easily access all the views of the window. This is needed as a CollectionListView widget will be added alongside the 'view' widget in a subsequent commit.
Created attachment 308918 [details] [review] app-window: Add 'view' property Rename the 'view' attribute to 'collection_view', making it private in the process, and add the 'view' property, allowing to easily access the current collection view of the window. This is needed as a CollectionListView widget will be added alongside the 'collection_view' widget in a subsequent commit.
Created attachment 308919 [details] [review] Add MachineThumbnailDrawer Allows to draw a machine's thumbnails with a Cairo.Context. Also makes MachineThumbnailer use this instead of its own methods. With this, a machine's thumbnail can be drawn on a pixbuf or on a widget with the same code. This is needed to add the MachineListThumbnail widget, using the power of the widget's state and CSS styling without having to duplicate the code drawing the thumbnail.
Created attachment 308920 [details] [review] Add MachineListThumbnail widget Allow to display a thumbnail for a list view from a machine. This is needed by CollectionItemListView which will be added in next commit and utimately this is needed to offer a list view.
Created attachment 308921 [details] [review] Add CollectionItemListView widget Represent a row of a list view from a machine. This is needed by CollectionListView which will be added in next commit and utimately this is needed to offer a list view.
Created attachment 308922 [details] [review] Add CollectionListView widget This will be used in next commit by the window, and it is utimately needed to offer a list view.
Created attachment 308923 [details] [review] app-window: Add list_view Add a CollectionListView alongside the current CollectionView. This is needed to offer a list view.
Created attachment 308924 [details] [review] app-window: Add 'view_type' prop Allow to switch the view type of a window This is needed to offer a list view.
Created attachment 308925 [details] [review] collection-toolbar: Add list view buttons Add buttons to the collection toolbar to switch between the collection view's list and icon modes. This is needed to offer a list view.
Review of attachment 308916 [details] [review]: ack
Created attachment 309108 [details] [review] machine-thumbnailer: Draw using CSS Makes the thumbnailer to use Cairo and CSS to draw the thumbnails. This will allow to have different styles for the thumbnails of the icon view and for the ones of the list view which will be added in subsequent commits.
Created attachment 309109 [details] [review] Add CollectionItemListView widget Represent a row of a list view from a machine. This is needed by CollectionListView which will be added in next commit and utimately this is needed to offer a list view.
Created attachment 309110 [details] [review] Add CollectionListView widget This will be used in next commit by the window, and it is utimately needed to offer a list view.
Created attachment 309111 [details] [review] collection-view: Add ICollectionView interface Extract the ICollectionView interface from CollectionView so it is easier to handle the current collection view and the CollectionListView which will be added by a subsequent commit.
Created attachment 309112 [details] [review] app-window: Add foreach_view() Add foreach_view(), allowing to easily access all the views of the window. This is needed as a CollectionListView widget will be added alongside the 'view' widget in a subsequent commit.
Created attachment 309113 [details] [review] app-window: Add 'view' property Rename the 'view' attribute to 'collection_view', making it private in the process, and add the 'view' property, allowing to easily access the current collection view of the window. This is needed as a CollectionListView widget will be added alongside the 'collection_view' widget in a subsequent commit.
Created attachment 309114 [details] [review] app-window: Add list_view Add a CollectionListView alongside the current CollectionView. This is needed to offer a list view.
Created attachment 309115 [details] [review] app-window: Add 'view_type' prop Allow to switch the view type of a window This is needed to offer a list view.
Created attachment 309116 [details] [review] collection-toolbar: Add list view buttons Add buttons to the collection toolbar to switch between the collection view's list and icon modes. This is needed to offer a list view.
Created attachment 309175 [details] [review] machine-thumbnailer: Make style customizable Makes the colors and sizes of the produced thumbnail customizable. This allows to have a different style for the list view's thumbnails which will be added in the next commit.
Created attachment 309176 [details] [review] Add CollectionItemListView widget Represent a row of a list view from a machine. This is needed by CollectionListView which will be added in next commit and utimately this is needed to offer a list view.
Created attachment 309177 [details] [review] Add CollectionListView widget This will be used in next commit by the window, and it is utimately needed to offer a list view.
Created attachment 309178 [details] [review] collection-view: Add ICollectionView interface Extract the ICollectionView interface from CollectionView so it is easier to handle the current collection view and the CollectionListView which will be added by a subsequent commit.
Created attachment 309179 [details] [review] app-window: Add foreach_view() Add foreach_view(), allowing to easily access all the views of the window. This is needed as a CollectionListView widget will be added alongside the 'view' widget in a subsequent commit.
Created attachment 309180 [details] [review] app-window: Add 'view' property Rename the 'view' attribute to 'collection_view', making it private in the process, and add the 'view' property, allowing to easily access the current collection view of the window. This is needed as a CollectionListView widget will be added alongside the 'collection_view' widget in a subsequent commit.
Created attachment 309181 [details] [review] app-window: Add list_view Add a CollectionListView alongside the current CollectionView. This is needed to offer a list view.
Created attachment 309182 [details] [review] app-window: Add 'view_type' prop Allow to switch the view type of a window This is needed to offer a list view.
Created attachment 309183 [details] [review] collection-toolbar: Add list view buttons Add buttons to the collection toolbar to switch between the collection view's list and icon modes. This is needed to offer a list view.
Created attachment 309461 [details] [review] machine-thumbnailer: Make style customizable Makes the colors and sizes of the produced thumbnail customizable. This allows to have a different style for the list view's thumbnails which will be added in the next commit.
Created attachment 309462 [details] [review] Add CollectionItemListView widget Represent a row of a list view from a machine. This is needed by CollectionListView which will be added in next commit and utimately this is needed to offer a list view.
Created attachment 309463 [details] [review] Add CollectionListView widget This will be used in next commit by the window, and it is utimately needed to offer a list view.
Created attachment 309464 [details] [review] collection-view: Add ICollectionView interface Extract the ICollectionView interface from CollectionView so it is easier to handle the current collection view and the CollectionListView which will be added by a subsequent commit.
Created attachment 309465 [details] [review] app-window: Add foreach_view() Add foreach_view(), allowing to easily access all the views of the window. This is needed as a CollectionListView widget will be added alongside the 'view' widget in a subsequent commit.
Created attachment 309466 [details] [review] app-window: Add 'view' property Rename the 'view' attribute to 'collection_view', making it private in the process, and add the 'view' property, allowing to easily access the current collection view of the window. This is needed as a CollectionListView widget will be added alongside the 'collection_view' widget in a subsequent commit.
Created attachment 309467 [details] [review] app-window: Add list_view Add a CollectionListView alongside the current CollectionView. This is needed to offer a list view.
Created attachment 309468 [details] [review] app-window: Add 'view_type' prop Allow to switch the view type of a window This is needed to offer a list view.
Created attachment 309469 [details] [review] collection-toolbar: Add list view buttons Add buttons to the collection toolbar to switch between the collection view's list and icon modes. This is needed to offer a list view.
Review of attachment 309461 [details] [review]: ack otherwise. ::: src/machine-thumbnailer.vala @@ +10,3 @@ public const int EMBLEM_PADDING = 8; + public weak Machine machine { get; private set; } Is this particular change really related to this patch?
Review of attachment 309461 [details] [review]: ::: src/machine-thumbnailer.vala @@ +10,3 @@ public const int EMBLEM_PADDING = 8; + public weak Machine machine { get; private set; } Yes, as I replace the GObject style construction by "this.machine = machine;" in the constructor: "construct" becomes useless and a setter is needed for this property.
Review of attachment 309462 [details] [review]: You missed my comments on description from previous review: https://bugzilla.gnome.org/review?bug=733252&attachment=307764 . ::: data/gtk-style.css @@ +211,3 @@ +.slow-spinner { + animation-duration: 2s; +} Could you please open a bug on removing most of these custom styles (and perhaps moving into standard theme) too look into after this is merged? You'd want to talk to Lapo and jimmac about it. ::: src/collection-item-list-view.vala @@ +50,3 @@ + private Gtk.Label machine_name; + [GtkChild] + private Gtk.Label info; i'd call it info_label for clarity. @@ +52,3 @@ + private Gtk.Label info; + [GtkChild] + private Gtk.Label state; state_label @@ +132,3 @@ + state.label = _("running"); + state.sensitive = true; + return; newlines before return, please. @@ +138,3 @@ + state.label = _("paused"); + state.sensitive = false; + return; same here. @@ +147,3 @@ + public override void show_all () { + show (); + } what? show() is implied in show_all().
Review of attachment 309462 [details] [review]: Oh and two things about name: 1. "Collection" is used for the current icon view so it shouldn't be part of the name. 2. "Item" is not very visible in the name, being in the middle of 3 other components of the name and name is confusing with CollectionListView. Probably better name would be: ListViewRow (that would be consistent with SnapshotListRow too).
Review of attachment 309463 [details] [review]: * Similarly I think name should be simply "ListView" here. * Not completely clear what you mean by "the window" here. You probably want to write "the app window". * I think the comment you missed from prev. review of last patch, actually applies to this one: "utimately needed" sounds like there is a long way towards it. ::: src/collection-list-view.vala @@ +8,3 @@ + [GtkChild] + private Gtk.ListBox list_box; + redundant empty line. @@ +124,3 @@ + } + + // FIXME Dirty hack to fix this bug in GTK+: https://bugzilla.gnome.org/show_bug.cgi?id=752615 * colon after FIXME please. * fix -> workaround? @@ +136,3 @@ + pos_b++; + + return ((pos_a > pos_b) ? 1 : 0) - ((pos_a < pos_b) ? 1 : 0); Don't get it. Why don't you simply compare box_a and box_b? @@ +143,3 @@ + + // FIXME Dirty hack to fix this bug in GTK+: https://bugzilla.gnome.org/show_bug.cgi?id=752615 + list_box.set_sort_func (model_sort); Why are you setting sort func twice? @@ +147,3 @@ + } + + public void select (SelectionCriteria selection) { * select -> select_by_criteria * selection -> criteria @@ +154,3 @@ + case SelectionCriteria.ALL: + list_box.select_all (); + break; Empty line before break pls. @@ +222,3 @@ + list_box.row_activated.connect ((row) => { + if (window.selection_mode) + return; in case of single-statment blocks, you want an emptyline after 'return'. @@ +225,3 @@ + var item = get_item_for_row (row); + if (item is LibvirtMachine && (item as LibvirtMachine).importing) + return; same here.
Review of attachment 309464 [details] [review]: I think the order of patches has gone wrong.
Review of attachment 309465 [details] [review]: IIRC, we agreed that adding of foreach before the second view isn't the best thing.
Review of attachment 309462 [details] [review]: ::: src/collection-item-list-view.vala @@ +147,3 @@ + public override void show_all () { + show (); + } I added it to prevent show_all() from prpagating to the children, as we want to control their visibility. I'll add a comment in the code.
Review of attachment 309462 [details] [review]: ::: src/collection-item-list-view.vala @@ +147,3 @@ + public override void show_all () { + show (); + } oh, that seems like a hack. Why do you need to call show_all() on it if that's not what you want?
Review of attachment 309463 [details] [review]: ::: src/collection-list-view.vala @@ +147,3 @@ + } + + public void select (SelectionCriteria selection) { I can't change it in this commit as CollectionView have this method and it will be part of ICollectionView, maybe I should add a commit between this one and the one adding ICollectionView to renameCollectionView.select()?
Review of attachment 309462 [details] [review]: ::: src/collection-item-list-view.vala @@ +147,3 @@ + public override void show_all () { + show (); + } It may be called on a parent (like the window) and propagate down to it. I personally don't like using use show_all() because of that, but I prefer to block it just in case.
Review of attachment 309463 [details] [review]: ::: src/collection-list-view.vala @@ +147,3 @@ + } + + public void select (SelectionCriteria selection) { oh ok. if you can quickly do that, good. Otherwise, leave it, as is.
Review of attachment 309462 [details] [review]: ::: src/collection-item-list-view.vala @@ +147,3 @@ + public override void show_all () { + show (); + } i don't think gtk+ calls show_all() on anything so it must be us/you who is doing it in Boxes and hence we should not do that instead of changing the semantics of show_all().
Created attachment 309491 [details] [review] machine-thumbnailer: Make style customizable Makes the colors and sizes of the produced thumbnail customizable. This allows to have a different style for the list view's thumbnails which will be added in the next commit.
Created attachment 309492 [details] [review] Add ListViewRow widget Represent a row of a list. This is needed by ListView which will be added in next commit to offer a list view.
Created attachment 309493 [details] [review] Add ListView widget This will be used in next commit by the app window to offer a list view.
Created attachment 309494 [details] [review] collection-view: select() -> select_by_criteria() This makes the method's name more understandable.
Created attachment 309495 [details] [review] collection-view: Add ICollectionView interface Extract the ICollectionView interface from CollectionView so it is easier to handle the current collection view and the CollectionListView which will be added by a subsequent commit.
Created attachment 309496 [details] [review] app-window: Add 'view' property Rename the 'view' attribute to 'collection_view', making it private in the process, and add the 'view' property, allowing to easily access the current collection view of the window. This is needed as a CollectionListView widget will be added alongside the 'collection_view' widget in a subsequent commit.
Created attachment 309497 [details] [review] app-window: Add foreach_view() Add foreach_view(), allowing to easily access all the views of the window. This is needed as a CollectionListView widget will be added alongside the 'view' widget in a subsequent commit.
Created attachment 309498 [details] [review] app-window: Add list_view Add a CollectionListView alongside the current CollectionView. This is needed to offer a list view.
Created attachment 309499 [details] [review] app-window: Add 'view_type' prop Allow to switch the view type of a window This is needed to offer a list view.
Created attachment 309500 [details] [review] collection-toolbar: Add list view buttons Add buttons to the collection toolbar to switch between the collection view's list and icon modes. This is needed to offer a list view.
Review of attachment 309493 [details] [review]: description says list view is added in the next commit but it's not the case. Wrong order?
Review of attachment 309493 [details] [review]: ::: src/list-view.vala @@ +51,3 @@ + } + + private HashTable<CollectionItem, ItemConnections> items_connections; pretty sure this is not private to 'contruct' below so you want to declare it above, with other fields/props. @@ +145,3 @@ + // FIXME: Dirty hack to workaround this bug in GTK+: https://bugzilla.gnome.org/show_bug.cgi?id=752615 + // Set the regulat sorting and filtering functions back to reset the ordering and the filtering after deleting + // an element. I don't think there is any need to repeat the whole thing here. You can just write the explaination above and in here say "FIXME: Workaround for bug#752615 (see above). @@ +395,3 @@ + } + + private void update_row_selected (Gtk.ListBoxRow? row) { row is nullable but you are not checking for null before using it. @@ +408,3 @@ + } + + private void view_selection_updated (ListViewRow view) { view -> row
Review of attachment 309493 [details] [review]: The order changed at some time and I forgot to update the log. ::: src/list-view.vala @@ +408,3 @@ + } + + private void view_selection_updated (ListViewRow view) { I changed for: - view -> view_row - row -> box_row So we can know when if deal with a ListViewRow or a ListBoxRow.
Created attachment 309549 [details] [review] Add ListView widget This will be used in a subsequent commit by the app window to offer a list view.
Created attachment 309550 [details] [review] collection-view: select() -> select_by_criteria() This makes the method's name more understandable.
Created attachment 309551 [details] [review] collection-view: Add ICollectionView interface Extract the ICollectionView interface from CollectionView so it is easier to handle the current collection view and the ListView which will be added by a subsequent commit.
Created attachment 309552 [details] [review] app-window: Add 'view' property Rename the 'view' attribute to 'collection_view', making it private in the process, and add the 'view' property, allowing to easily access the current collection view of the window. This is needed as a ListView widget will be added alongside the 'collection_view' widget in a subsequent commit.
Created attachment 309553 [details] [review] app-window: Add foreach_view() Add foreach_view(), allowing to easily access all the views of the window. This is needed as a ListView widget will be added alongside the 'collection_view' widget in a subsequent commit.
Created attachment 309554 [details] [review] app-window: Add list_view Add a ListView alongside the current CollectionView. This is needed to offer a list view.
Created attachment 309555 [details] [review] app-window: Add 'view_type' prop Allow to switch the view type of a window This is needed to offer a list view.
Created attachment 309556 [details] [review] collection-toolbar: Add list view buttons Add buttons to the collection toolbar to switch between the collection view's list and icon modes. This is needed to offer a list view.
Created attachment 309586 [details] [review] machine-thumbnailer: Make style customizable Makes the colors and sizes of the produced thumbnail customizable. This allows to have a different style for the list view's thumbnails which will be added in a subsequent commit.
Created attachment 309587 [details] [review] collection-view: select() -> select_by_criteria() This makes the method's name more understandable.
Created attachment 309588 [details] [review] collection-view: Add ICollectionView interface Extract the ICollectionView interface from CollectionView so it is easier to handle the current collection view and the ListView which will be added by a subsequent commit.
Created attachment 309589 [details] [review] Add ListViewRow widget Represent a row of a list. This is needed by ListView which will be added in next commit to offer a list view.
Created attachment 309590 [details] [review] Add ListView widget This will be used in a subsequent commit by the app window to offer a list view.
Created attachment 309591 [details] [review] app-window: Add 'view' property Rename the 'view' attribute to 'collection_view', making it private in the process, and add the 'view' property, allowing to easily access the current collection view of the window. This is needed as a ListView widget will be added alongside the 'collection_view' widget in a subsequent commit.
Created attachment 309592 [details] [review] app-window: Add foreach_view() Add foreach_view(), allowing to easily access all the views of the window. This is needed as a ListView widget will be added alongside the 'collection_view' widget in a subsequent commit.
Created attachment 309593 [details] [review] app-window: Add list_view Add a ListView alongside the current CollectionView. This is needed to offer a list view.
Created attachment 309594 [details] [review] app-window: Add 'view_type' prop Allow to switch the view type of a window This is needed to offer a list view.
Created attachment 309595 [details] [review] collection-toolbar: Add list view buttons Add buttons to the collection toolbar to switch between the collection view's list and icon modes. This is needed to offer a list view.
Created attachment 309627 [details] [review] machine-thumbnailer: 'machine' setter now private Make 'machine' property setter private.
Created attachment 309628 [details] [review] machine-thumbnailer: Drop redundant construct block There is no reason to have both construct block and construction method, let's do all initialization in construction method and drop construct block.
Created attachment 309629 [details] [review] machine-thumbnailer: Make style customizable Makes the colors and sizes of the produced thumbnail customizable. This allows to have a different style for the list view's thumbnails which will be added in a subsequent commit.
Created attachment 309630 [details] [review] collection-view: select() -> select_by_criteria() This makes the method's function clearer.
Created attachment 309631 [details] [review] collection-view: Add ICollectionView interface Extract the ICollectionView interface from CollectionView so it is easier to handle the current collection view and the ListView which will be added by a subsequent commit.
Created attachment 309632 [details] [review] Add ListViewRow widget In the following commit ListView will be added and it'll use this class/widget to show each machine as a row.
Created attachment 309634 [details] [review] Add ListView widget This will be used in a subsequent commit by the app window to offer a list view.
Created attachment 309635 [details] [review] app-window: Add 'collection_view' property Rename the 'view' attribute to 'collection_view', making it private in the process, and add a new generic 'view' property that always points to the current collection view of the window. For now this isn't very useful since we only have a single view but a list view will be added in a following patch.
Created attachment 309636 [details] [review] app-window: Add foreach_view() Add foreach_view(), allowing to easily iterate over all the views of the window. For now this isn't very useful since we only have a single view but a list view will be added in a following patch.
Created attachment 309638 [details] [review] app-window: Add list view Add a list view alongside the current icon view.
Created attachment 309639 [details] [review] app-window: Add 'view_type' prop Allow to switch the view type of a window
Created attachment 309640 [details] [review] collection-toolbar: Add list view buttons Add buttons to the collection toolbar to switch between the collection view's list and icon modes.
Created attachment 309694 [details] [review] machine-thumbnailer: 'machine' setter now private Make 'machine' property setter private.
Created attachment 309695 [details] [review] machine-thumbnailer: Drop redundant construct block There is no reason to have both construct block and construction method, let's do all initialization in construction method and drop construct block.
Created attachment 309696 [details] [review] machine-thumbnailer: Make style customizable Makes the colors and sizes of the produced thumbnail customizable. This allows to have a different style for the list view's thumbnails which will be added in a subsequent commit.
Created attachment 309697 [details] [review] collection-view: select() -> select_by_criteria() This makes the method's function clearer.
Created attachment 309698 [details] [review] collection-view: Add ICollectionView interface Extract the ICollectionView interface from CollectionView so it is easier to handle the current collection view and the ListView which will be added by a subsequent commit.
Created attachment 309699 [details] [review] Add ListViewRow widget In the following commit ListView will be added and it'll use this class/widget to show each machine as a row.
Created attachment 309700 [details] [review] Add ListView widget This will be used in a subsequent commit by the app window to offer a list view.
Created attachment 309701 [details] [review] app-window: Add 'collection_view' property Rename the 'view' attribute to 'collection_view', making it private in the process, and add a new generic 'view' property that always points to the current collection view of the window. For now this isn't very useful since we only have a single view but a list view will be added in a following patch.
Created attachment 309702 [details] [review] app-window: Add foreach_view() Add foreach_view(), allowing to easily iterate over all the views of the window. For now this isn't very useful since we only have a single view but a list view will be added in a following patch.
Created attachment 309703 [details] [review] app-window: Add list view Add a list view alongside the current icon view.
Created attachment 309704 [details] [review] app-window: Add 'view_type' prop Allow to switch the view type of a window
Created attachment 309705 [details] [review] collection-toolbar: Add list view buttons Add buttons to the collection toolbar to switch between the collection view's list and icon modes.
Review of attachment 309700 [details] [review]: ::: src/list-view.vala @@ +367,3 @@ + private void update_selection_mode () { + foreach_row ((box_row) => { + if (box_row == null) forearch shouldn't pass us nulls. @@ +384,3 @@ + private void propagate_view_row_selection (ListViewRow view_row) { + var box_row = view_row.parent as Gtk.ListBoxRow; + if (box_row == null) same here. @@ +394,3 @@ + + private void toggle_row_selected (Gtk.ListBoxRow box_row) { + if (box_row == null) and here. @@ +409,3 @@ + + private void select_row (Gtk.ListBoxRow box_row) { + if (box_row == null) box_row is declared non-nullable and I'm sure that is correct. @@ +414,3 @@ + var view_row = box_row.get_child () as ListViewRow; + + if (view_row == null) can a row be empty? you have the same null checks in other row operating functions too. @@ +425,3 @@ + + private void unselect_row (Gtk.ListBoxRow box_row) { + if (box_row == null) here as well. @@ +430,3 @@ + var view_row = box_row.get_child () as ListViewRow; + + if (view_row == null) and here.
Review of attachment 309700 [details] [review]: ::: src/list-view.vala @@ +367,3 @@ + private void update_selection_mode () { + foreach_row ((box_row) => { + if (box_row == null) Your're right, these checks are useless, I'll remove them. @@ +414,3 @@ + var view_row = box_row.get_child () as ListViewRow; + + if (view_row == null) There is next to no chance for this to happen, but the cast can return it null if the child happen not to be a ListViewRow. Perhapse we should use "return if fail" to have a warning if it happens?
Review of attachment 309700 [details] [review]: ::: src/list-view.vala @@ +414,3 @@ + var view_row = box_row.get_child () as ListViewRow; + + if (view_row == null) The main question is, do you put anything other than one particular type in the row? If not, this check is pretty much useless.
Review of attachment 309700 [details] [review]: ::: src/list-view.vala @@ +414,3 @@ + var view_row = box_row.get_child () as ListViewRow; + + if (view_row == null) I think I remember you wanting to add checks when downcasting CollectionItem to Machine, even though the collection's items are all machines for now, so I thought it was a good idea to check all downcasts. I'll remove it if you think it's useless. =)
Created attachment 309720 [details] [review] Add ListView widget This will be used in a subsequent commit by the app window to offer a list view.
Created attachment 309721 [details] [review] app-window: Add 'collection_view' property Rename the 'view' attribute to 'collection_view', making it private in the process, and add a new generic 'view' property that always points to the current collection view of the window. For now this isn't very useful since we only have a single view but a list view will be added in a following patch.
Created attachment 309722 [details] [review] app-window: Add foreach_view() Add foreach_view(), allowing to easily iterate over all the views of the window. For now this isn't very useful since we only have a single view but a list view will be added in a following patch.
Created attachment 309723 [details] [review] app-window: Add list view Add a list view alongside the current icon view.
Created attachment 309724 [details] [review] app-window: Add 'view_type' prop Allow to switch the view type of a window
Created attachment 309725 [details] [review] collection-toolbar: Add list view buttons Add buttons to the collection toolbar to switch between the collection view's list and icon modes.
Review of attachment 309700 [details] [review]: ::: src/list-view.vala @@ +414,3 @@ + var view_row = box_row.get_child () as ListViewRow; + + if (view_row == null) That's different. There could very easily be non-Machine items and I've had one in my private branch for years this item that represents a group of machines (aka folder) and I think that possiblity was the idea behind keeping Machine and Item separate.
Review of attachment 309700 [details] [review]: ::: src/list-view.vala @@ +414,3 @@ + var view_row = box_row.get_child () as ListViewRow; + + if (view_row == null) Interesting, thanks for the explanation.
Created attachment 309770 [details] Screenshot of the feature
Attachment 309694 [details] pushed as c2c491e - machine-thumbnailer: 'machine' setter now private Attachment 309695 [details] pushed as 2548029 - machine-thumbnailer: Drop redundant construct block Attachment 309696 [details] pushed as 667108d - machine-thumbnailer: Make style customizable Attachment 309697 [details] pushed as a65543f - collection-view: select() -> select_by_criteria() Attachment 309698 [details] pushed as 5288897 - collection-view: Add ICollectionView interface Attachment 309699 [details] pushed as 2913edf - Add ListViewRow widget Attachment 309720 [details] pushed as 45206f5 - Add ListView widget Attachment 309721 [details] pushed as 9fa66f3 - app-window: Add 'collection_view' property Attachment 309722 [details] pushed as 83857a1 - app-window: Add foreach_view() Attachment 309723 [details] pushed as 364a7b0 - app-window: Add list view Attachment 309724 [details] pushed as fae592d - app-window: Add 'view_type' prop Attachment 309725 [details] pushed as b8a71e0 - collection-toolbar: Add list view buttons