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 732098 - Get rid of AppWindow singleton
Get rid of AppWindow singleton
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: general
unspecified
Other Linux
: Normal normal
: 3.22
Assigned To: GNOME Boxes maintainer(s)
GNOME Boxes maintainer(s)
Depends on:
Blocks: 710304 734486
 
 
Reported: 2014-06-23 12:18 UTC by Zeeshan Ali
Modified: 2016-03-31 13:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
topbar: Remove App and AppWindow dependencies (5.24 KB, patch)
2014-06-24 11:00 UTC, Adrien Plazas
needs-work Details | Review
searchbar: Remove App and AppWindow dependencies (3.99 KB, patch)
2014-06-24 11:00 UTC, Adrien Plazas
needs-work Details | Review
properties-toolbar: Remove App dependencies (2.75 KB, patch)
2014-06-24 11:00 UTC, Adrien Plazas
needs-work Details | Review
selection-toolbar: Remove App and AppWindow dependencies (5.41 KB, patch)
2014-06-24 11:00 UTC, Adrien Plazas
needs-work Details | Review
collection-toolbar: Remove App dependencies (8.12 KB, patch)
2014-06-24 11:00 UTC, Adrien Plazas
needs-work Details | Review
wizard: Replace buttons with toolbar (7.56 KB, patch)
2014-07-04 07:15 UTC, Adrien Plazas
needs-work Details | Review
wizard: Notify failed installation (2.93 KB, patch)
2014-07-04 07:15 UTC, Adrien Plazas
needs-work Details | Review
wizard: Notify successful installation (1.61 KB, patch)
2014-07-04 07:15 UTC, Adrien Plazas
needs-work Details | Review
app-window: Edit sidebar's wizard page (1.20 KB, patch)
2014-07-04 07:15 UTC, Adrien Plazas
needs-work Details | Review
wizard: Bind page to toolbar's page (1.70 KB, patch)
2014-07-04 07:15 UTC, Adrien Plazas
reviewed Details | Review
vm-creator: Notify failed installation (1.87 KB, patch)
2014-07-04 07:15 UTC, Adrien Plazas
needs-work Details | Review
wizard: Use WizardToolbar (7.68 KB, patch)
2014-07-07 08:06 UTC, Adrien Plazas
needs-work Details | Review
wizard: Notify failed installation (3.08 KB, patch)
2014-07-07 08:06 UTC, Adrien Plazas
reviewed Details | Review
wizard: Notify successful installation (1.76 KB, patch)
2014-07-07 08:06 UTC, Adrien Plazas
needs-work Details | Review
app-window: Set sidebar's wizard page (1.27 KB, patch)
2014-07-07 08:06 UTC, Adrien Plazas
reviewed Details | Review
wizard: Bind page to toolbar's page (1.83 KB, patch)
2014-07-07 08:06 UTC, Adrien Plazas
reviewed Details | Review
vm-creator: Notify failed installation (2.02 KB, patch)
2014-07-07 08:06 UTC, Adrien Plazas
reviewed Details | Review
wizard-toolbar: Set buttons depending on its page (6.54 KB, patch)
2014-07-07 08:06 UTC, Adrien Plazas
needs-work Details | Review
wizard: Notify successful installation (1.71 KB, patch)
2014-07-07 09:02 UTC, Adrien Plazas
reviewed Details | Review
topbar: Notify clicked cancel (3.40 KB, patch)
2014-07-07 12:49 UTC, Adrien Plazas
needs-work Details | Review
topbar: Check its own ui state (1.14 KB, patch)
2014-07-07 12:49 UTC, Adrien Plazas
reviewed Details | Review
app-window: Bind current item to properties title (2.91 KB, patch)
2014-07-07 12:49 UTC, Adrien Plazas
reviewed Details | Review
topbar: Set selection mode (2.00 KB, patch)
2014-07-07 12:49 UTC, Adrien Plazas
needs-work Details | Review
topbar: Set display title (1.99 KB, patch)
2014-07-07 12:49 UTC, Adrien Plazas
needs-work Details | Review
searchbar: Add window property (3.47 KB, patch)
2014-07-08 06:47 UTC, Adrien Plazas
none Details | Review
searchbar: Notify on changed search (1.46 KB, patch)
2014-07-08 06:47 UTC, Adrien Plazas
none Details | Review
searchbar: Notify on activated search (1.43 KB, patch)
2014-07-08 06:47 UTC, Adrien Plazas
none Details | Review
searchbar: Add window property (3.47 KB, patch)
2014-07-08 07:09 UTC, Adrien Plazas
none Details | Review
searchbar: Notify on changed search (1.46 KB, patch)
2014-07-08 07:09 UTC, Adrien Plazas
none Details | Review
searchbar: Notify on activated search (1.43 KB, patch)
2014-07-08 07:09 UTC, Adrien Plazas
none Details | Review
properties-toolbar: Notify on back clicked (3.40 KB, patch)
2014-07-08 07:09 UTC, Adrien Plazas
none Details | Review
searchbar: Add window property (3.47 KB, patch)
2014-07-08 07:23 UTC, Adrien Plazas
needs-work Details | Review
searchbar: Notify on changed search (1.65 KB, patch)
2014-07-08 07:23 UTC, Adrien Plazas
reviewed Details | Review
searchbar: Notify on activated search (1.41 KB, patch)
2014-07-08 07:23 UTC, Adrien Plazas
needs-work Details | Review
properties-toolbar: Notify on back clicked (3.40 KB, patch)
2014-07-08 07:23 UTC, Adrien Plazas
needs-work Details | Review
wizard-toolbar: Set buttons depending on its page (6.54 KB, patch)
2014-07-08 14:39 UTC, Adrien Plazas
none Details | Review
wizard-toolbar: Set buttons depending on its page (6.33 KB, patch)
2014-07-08 14:45 UTC, Adrien Plazas
none Details | Review
wizard: Use WizardToolbar (7.68 KB, patch)
2014-07-09 11:08 UTC, Adrien Plazas
needs-work Details | Review
wizard: Notify failed installation (3.08 KB, patch)
2014-07-09 11:08 UTC, Adrien Plazas
reviewed Details | Review
wizard: Notify successful installation (1.71 KB, patch)
2014-07-09 11:08 UTC, Adrien Plazas
reviewed Details | Review
app-window: Set sidebar's wizard page (1.27 KB, patch)
2014-07-09 11:08 UTC, Adrien Plazas
reviewed Details | Review
wizard: Bind page to toolbar's page (1.83 KB, patch)
2014-07-09 11:08 UTC, Adrien Plazas
reviewed Details | Review
vm-creator: Notify failed installation (2.02 KB, patch)
2014-07-09 11:08 UTC, Adrien Plazas
reviewed Details | Review
wizard-toolbar: Set buttons depending on its page (6.32 KB, patch)
2014-07-09 11:08 UTC, Adrien Plazas
needs-work Details | Review
topbar: Notify clicked cancel (3.37 KB, patch)
2014-07-09 11:09 UTC, Adrien Plazas
reviewed Details | Review
topbar: Check its own ui state (1.14 KB, patch)
2014-07-09 11:09 UTC, Adrien Plazas
reviewed Details | Review
topbar: Set title from machine name (3.54 KB, patch)
2014-07-09 11:09 UTC, Adrien Plazas
reviewed Details | Review
app-window: Set topbar's selection mode (1.64 KB, patch)
2014-07-09 11:09 UTC, Adrien Plazas
reviewed Details | Review
topbar: Set display subtitle (1.78 KB, patch)
2014-07-09 11:09 UTC, Adrien Plazas
needs-work Details | Review
searchbar: Add window property (4.18 KB, patch)
2014-07-09 11:09 UTC, Adrien Plazas
needs-work Details | Review
searchbar: Notify on changed search (1.65 KB, patch)
2014-07-09 11:09 UTC, Adrien Plazas
needs-work Details | Review
searchbar: Notify on activated search (1.41 KB, patch)
2014-07-09 11:09 UTC, Adrien Plazas
reviewed Details | Review
properties-toolbar: Notify on back clicked (3.49 KB, patch)
2014-07-09 11:09 UTC, Adrien Plazas
reviewed Details | Review
wizard: Add toolbar prop (7.59 KB, patch)
2014-07-10 07:23 UTC, Adrien Plazas
needs-work Details | Review
app: Set state to wizard when opening URI (1.60 KB, patch)
2014-07-10 07:23 UTC, Adrien Plazas
reviewed Details | Review
wizard: Notify failed installation (3.13 KB, patch)
2014-07-10 07:23 UTC, Adrien Plazas
none Details | Review
wizard: Notify successful installation (1.78 KB, patch)
2014-07-10 07:23 UTC, Adrien Plazas
none Details | Review
app-window: Set sidebar's wizard page (1.40 KB, patch)
2014-07-10 07:23 UTC, Adrien Plazas
none Details | Review
wizard-toolbar: Add page prop (1.88 KB, patch)
2014-07-10 07:23 UTC, Adrien Plazas
none Details | Review
wizard-toolbar: page prop set its state (6.30 KB, patch)
2014-07-10 07:24 UTC, Adrien Plazas
none Details | Review
vm-creator: Notify failed installation (2.07 KB, patch)
2014-07-10 07:24 UTC, Adrien Plazas
none Details | Review
topbar: Notify clicked cancel (3.39 KB, patch)
2014-07-10 07:24 UTC, Adrien Plazas
none Details | Review
topbar: Check its own ui state (1.21 KB, patch)
2014-07-10 07:24 UTC, Adrien Plazas
none Details | Review
topbar: Set title from machine name (3.55 KB, patch)
2014-07-10 07:24 UTC, Adrien Plazas
none Details | Review
topbar: Set display subtitle (1.88 KB, patch)
2014-07-10 07:24 UTC, Adrien Plazas
none Details | Review
app-window: Set topbar's selection mode (1.68 KB, patch)
2014-07-10 07:24 UTC, Adrien Plazas
none Details | Review
searchbar: Add window property (3.00 KB, patch)
2014-07-10 07:24 UTC, Adrien Plazas
none Details | Review
searchbar: Transmit key handler blocked to windows (2.71 KB, patch)
2014-07-10 07:24 UTC, Adrien Plazas
none Details | Review
searchbar: Notify on changed search (1.71 KB, patch)
2014-07-10 07:25 UTC, Adrien Plazas
none Details | Review
searchbar: Notify on activated search (1.51 KB, patch)
2014-07-10 07:25 UTC, Adrien Plazas
none Details | Review
properties-toolbar: Notify on back clicked (3.55 KB, patch)
2014-07-10 07:25 UTC, Adrien Plazas
none Details | Review
wizard: Add toolbar prop (7.86 KB, patch)
2014-07-10 08:52 UTC, Adrien Plazas
needs-work Details | Review
wizard: Notify failed installation (3.13 KB, patch)
2014-07-10 08:52 UTC, Adrien Plazas
none Details | Review
wizard: Notify successful installation (1.78 KB, patch)
2014-07-10 08:53 UTC, Adrien Plazas
none Details | Review
app-window: Set sidebar's wizard page (1.40 KB, patch)
2014-07-10 08:53 UTC, Adrien Plazas
none Details | Review
wizard-toolbar: Add page prop (1.88 KB, patch)
2014-07-10 08:53 UTC, Adrien Plazas
none Details | Review
wizard-toolbar: page prop set its state (6.30 KB, patch)
2014-07-10 08:53 UTC, Adrien Plazas
none Details | Review
vm-creator: Notify failed installation (2.07 KB, patch)
2014-07-10 08:53 UTC, Adrien Plazas
none Details | Review
topbar: Notify clicked cancel (3.39 KB, patch)
2014-07-10 08:53 UTC, Adrien Plazas
none Details | Review
topbar: Check its own ui state (1.21 KB, patch)
2014-07-10 08:53 UTC, Adrien Plazas
none Details | Review
topbar: Set title from machine name (3.55 KB, patch)
2014-07-10 08:53 UTC, Adrien Plazas
none Details | Review
topbar: Set display subtitle (1.88 KB, patch)
2014-07-10 08:54 UTC, Adrien Plazas
none Details | Review
app-window: Set topbar's selection mode (1.68 KB, patch)
2014-07-10 08:54 UTC, Adrien Plazas
none Details | Review
searchbar: Add window property (3.00 KB, patch)
2014-07-10 08:54 UTC, Adrien Plazas
none Details | Review
searchbar: Transmit key handler blocked to windows (2.71 KB, patch)
2014-07-10 08:54 UTC, Adrien Plazas
none Details | Review
searchbar: Notify on changed search (1.71 KB, patch)
2014-07-10 08:54 UTC, Adrien Plazas
none Details | Review
searchbar: Notify on activated search (1.51 KB, patch)
2014-07-10 08:54 UTC, Adrien Plazas
none Details | Review
properties-toolbar: Notify on back clicked (3.55 KB, patch)
2014-07-10 08:54 UTC, Adrien Plazas
none Details | Review
wizard: Add toolbar prop (6.97 KB, patch)
2014-07-10 10:22 UTC, Adrien Plazas
reviewed Details | Review
app: Set state to wizard when opening URI (1.69 KB, patch)
2014-07-10 10:22 UTC, Adrien Plazas
reviewed Details | Review
wizard: Notify failed installation (3.13 KB, patch)
2014-07-10 10:22 UTC, Adrien Plazas
reviewed Details | Review
wizard: Notify successful installation (1.78 KB, patch)
2014-07-10 10:22 UTC, Adrien Plazas
reviewed Details | Review
app-window: Set sidebar's wizard page (1.40 KB, patch)
2014-07-10 10:22 UTC, Adrien Plazas
needs-work Details | Review
wizard-toolbar: Add page prop (1.88 KB, patch)
2014-07-10 10:22 UTC, Adrien Plazas
reviewed Details | Review
wizard-toolbar: page prop set its state (6.30 KB, patch)
2014-07-10 10:22 UTC, Adrien Plazas
needs-work Details | Review
vm-creator: Notify failed installation (2.07 KB, patch)
2014-07-10 10:23 UTC, Adrien Plazas
reviewed Details | Review
topbar: Notify clicked cancel (3.39 KB, patch)
2014-07-10 10:23 UTC, Adrien Plazas
reviewed Details | Review
topbar: Check its own ui state (1.21 KB, patch)
2014-07-10 10:23 UTC, Adrien Plazas
reviewed Details | Review
topbar: Set title from machine name (3.55 KB, patch)
2014-07-10 10:23 UTC, Adrien Plazas
reviewed Details | Review
topbar: Set display subtitle (1.88 KB, patch)
2014-07-10 10:23 UTC, Adrien Plazas
needs-work Details | Review
app-window: Set topbar's selection mode (1.68 KB, patch)
2014-07-10 10:23 UTC, Adrien Plazas
needs-work Details | Review
searchbar: Add window property (3.00 KB, patch)
2014-07-10 10:23 UTC, Adrien Plazas
reviewed Details | Review
searchbar: Transmit key handler blocked to windows (2.71 KB, patch)
2014-07-10 10:24 UTC, Adrien Plazas
needs-work Details | Review
searchbar: Notify on changed search (1.71 KB, patch)
2014-07-10 10:24 UTC, Adrien Plazas
needs-work Details | Review
searchbar: Notify on activated search (1.51 KB, patch)
2014-07-10 10:24 UTC, Adrien Plazas
reviewed Details | Review
properties-toolbar: Notify on back clicked (3.55 KB, patch)
2014-07-10 10:24 UTC, Adrien Plazas
needs-work Details | Review
wizard: Add toolbar prop (6.95 KB, patch)
2014-07-10 14:17 UTC, Adrien Plazas
needs-work Details | Review
app: Set state to wizard when opening URI (1.69 KB, patch)
2014-07-10 14:17 UTC, Adrien Plazas
none Details | Review
wizard: Notify failed installation (3.13 KB, patch)
2014-07-10 14:17 UTC, Adrien Plazas
none Details | Review
wizard: Notify successful installation (1.78 KB, patch)
2014-07-10 14:17 UTC, Adrien Plazas
none Details | Review
app-window: Set sidebar's wizard page (1.40 KB, patch)
2014-07-10 14:18 UTC, Adrien Plazas
none Details | Review
wizard-toolbar: Add page prop (1.90 KB, patch)
2014-07-10 14:18 UTC, Adrien Plazas
needs-work Details | Review
wizard-toolbar: page prop set its state (6.32 KB, patch)
2014-07-10 14:18 UTC, Adrien Plazas
none Details | Review
vm-creator: Notify failed installation (2.07 KB, patch)
2014-07-10 14:18 UTC, Adrien Plazas
none Details | Review
topbar: Notify clicked cancel (3.39 KB, patch)
2014-07-10 14:18 UTC, Adrien Plazas
none Details | Review
topbar: Check its own ui state (1.21 KB, patch)
2014-07-10 14:18 UTC, Adrien Plazas
none Details | Review
topbar: Set title from machine name (3.55 KB, patch)
2014-07-10 14:18 UTC, Adrien Plazas
needs-work Details | Review
topbar: Set display subtitle (1.80 KB, patch)
2014-07-10 14:19 UTC, Adrien Plazas
none Details | Review
app-window: Set topbar's selection mode (1.65 KB, patch)
2014-07-10 14:19 UTC, Adrien Plazas
none Details | Review
searchbar: Add window property (2.98 KB, patch)
2014-07-10 14:19 UTC, Adrien Plazas
none Details | Review
searchbar: Transmit key handler blocked to windows (2.66 KB, patch)
2014-07-10 14:19 UTC, Adrien Plazas
none Details | Review
searchbar: Notify on changed search (1.37 KB, patch)
2014-07-10 14:19 UTC, Adrien Plazas
none Details | Review
searchbar: Notify on activated search (1.47 KB, patch)
2014-07-10 14:19 UTC, Adrien Plazas
none Details | Review
properties-toolbar: Notify on back clicked (3.55 KB, patch)
2014-07-10 14:19 UTC, Adrien Plazas
none Details | Review
wizard-toolbar: Add page prop (1.90 KB, patch)
2014-07-11 09:03 UTC, Adrien Plazas
none Details | Review
wizard-toolbar: page prop set its state (6.32 KB, patch)
2014-07-11 09:04 UTC, Adrien Plazas
none Details | Review
vm-creator: Notify failed installation (2.07 KB, patch)
2014-07-11 09:04 UTC, Adrien Plazas
none Details | Review
topbar: Notify clicked cancel (3.39 KB, patch)
2014-07-11 09:04 UTC, Adrien Plazas
none Details | Review
topbar: Check its own ui state (1.21 KB, patch)
2014-07-11 09:04 UTC, Adrien Plazas
none Details | Review
topbar: Set title from machine name (3.55 KB, patch)
2014-07-11 09:04 UTC, Adrien Plazas
none Details | Review
topbar: Set display subtitle (1.80 KB, patch)
2014-07-11 09:04 UTC, Adrien Plazas
none Details | Review
app-window: Set topbar's selection mode (1.65 KB, patch)
2014-07-11 09:05 UTC, Adrien Plazas
none Details | Review
searchbar: Add window property (2.98 KB, patch)
2014-07-11 09:05 UTC, Adrien Plazas
none Details | Review
searchbar: Transmit key handler blocked to windows (2.59 KB, patch)
2014-07-11 09:05 UTC, Adrien Plazas
none Details | Review
searchbar: Notify on changed search (1.37 KB, patch)
2014-07-11 09:05 UTC, Adrien Plazas
none Details | Review
searchbar: Notify on activated search (1.47 KB, patch)
2014-07-11 09:05 UTC, Adrien Plazas
none Details | Review
properties-toolbar: Notify on back clicked (3.97 KB, patch)
2014-07-11 09:05 UTC, Adrien Plazas
none Details | Review
app-window: Add current-item props (1.80 KB, patch)
2014-07-16 11:11 UTC, Adrien Plazas
none Details | Review
app-window: Add show_properties method (2.32 KB, patch)
2014-07-16 11:12 UTC, Adrien Plazas
none Details | Review
app-window: Add connect_to method (3.90 KB, patch)
2014-07-16 11:12 UTC, Adrien Plazas
none Details | Review
app-window: Add select_item method (5.20 KB, patch)
2014-07-16 11:12 UTC, Adrien Plazas
none Details | Review
app-window: Cancel machine connection on collection state (2.47 KB, patch)
2014-07-16 11:12 UTC, Adrien Plazas
none Details | Review
app: Remove current_item attribute (8.46 KB, patch)
2014-07-16 11:12 UTC, Adrien Plazas
none Details | Review
app-window: Add selection-mode props (1.93 KB, patch)
2014-07-16 11:12 UTC, Adrien Plazas
none Details | Review
collection-view: Add setup_ui method (1.34 KB, patch)
2014-07-16 11:13 UTC, Adrien Plazas
none Details | Review
selectionbar: Add setup_ui method (1.49 KB, patch)
2014-07-16 11:13 UTC, Adrien Plazas
none Details | Review
Use AppWindow's selection-mode (6.09 KB, patch)
2014-07-16 11:13 UTC, Adrien Plazas
none Details | Review
app: Remove selection-mode props (1.07 KB, patch)
2014-07-16 11:13 UTC, Adrien Plazas
none Details | Review
app: Remove UI state (15.69 KB, patch)
2014-07-16 11:13 UTC, Adrien Plazas
none Details | Review
AppWindow: Remove use of App.window (1.43 KB, patch)
2014-07-16 11:13 UTC, Adrien Plazas
none Details | Review
app-window: Add current-item props (1.80 KB, patch)
2014-07-16 11:41 UTC, Adrien Plazas
needs-work Details | Review
app-window: Add show_properties method (2.32 KB, patch)
2014-07-16 11:41 UTC, Adrien Plazas
needs-work Details | Review
app-window: Add connect_to method (3.90 KB, patch)
2014-07-16 11:42 UTC, Adrien Plazas
needs-work Details | Review
app-window: Add select_item method (5.20 KB, patch)
2014-07-16 11:42 UTC, Adrien Plazas
needs-work Details | Review
app-window: Cancel machine connection on collection state (2.47 KB, patch)
2014-07-16 11:42 UTC, Adrien Plazas
needs-work Details | Review
app: Remove current_item attribute (8.46 KB, patch)
2014-07-16 11:42 UTC, Adrien Plazas
needs-work Details | Review
app-window: Add selection-mode props (1.93 KB, patch)
2014-07-16 11:42 UTC, Adrien Plazas
none Details | Review
collection-view: Add setup_ui method (1.34 KB, patch)
2014-07-16 11:42 UTC, Adrien Plazas
none Details | Review
selectionbar: Add setup_ui method (1.49 KB, patch)
2014-07-16 11:43 UTC, Adrien Plazas
none Details | Review
Use AppWindow's selection-mode (6.09 KB, patch)
2014-07-16 11:43 UTC, Adrien Plazas
none Details | Review
app: Remove selection-mode props (1.07 KB, patch)
2014-07-16 11:43 UTC, Adrien Plazas
none Details | Review
collection-toolbar: Move UI state connction to setup_ui (1.37 KB, patch)
2014-07-16 11:43 UTC, Adrien Plazas
none Details | Review
app: Remove UI state (15.60 KB, patch)
2014-07-16 11:43 UTC, Adrien Plazas
none Details | Review
AppWindow: Remove use of App.window (1.43 KB, patch)
2014-07-16 11:43 UTC, Adrien Plazas
none Details | Review
collection-toolbar: Move UI state connection to setup_ui (1.37 KB, patch)
2014-07-16 11:48 UTC, Adrien Plazas
none Details | Review
app: Remove UI state (15.60 KB, patch)
2014-07-16 11:49 UTC, Adrien Plazas
none Details | Review
AppWindow: Remove use of App.window (1.43 KB, patch)
2014-07-16 11:49 UTC, Adrien Plazas
none Details | Review
app-window: Add current-item prop (1.80 KB, patch)
2014-07-16 15:27 UTC, Adrien Plazas
needs-work Details | Review
app-window: Add show_properties() (2.32 KB, patch)
2014-07-16 15:27 UTC, Adrien Plazas
needs-work Details | Review
app-window: Add connect_to() (3.91 KB, patch)
2014-07-16 15:27 UTC, Adrien Plazas
needs-work Details | Review
app-window: Add select_item() (5.20 KB, patch)
2014-07-16 15:27 UTC, Adrien Plazas
needs-work Details | Review
app-window: Cancel machine connection on collection state (2.52 KB, patch)
2014-07-16 15:28 UTC, Adrien Plazas
none Details | Review
app: Move current_item to AppWindow (8.46 KB, patch)
2014-07-16 15:28 UTC, Adrien Plazas
none Details | Review
app-window: Add selection-mode prop (1.93 KB, patch)
2014-07-16 15:28 UTC, Adrien Plazas
none Details | Review
collection-view: Add setup_ui() (1.34 KB, patch)
2014-07-16 15:28 UTC, Adrien Plazas
none Details | Review
selectionbar: Add setup_ui() (1.50 KB, patch)
2014-07-16 15:28 UTC, Adrien Plazas
none Details | Review
Use AppWindow's selection-mode (6.09 KB, patch)
2014-07-16 15:28 UTC, Adrien Plazas
none Details | Review
app: Move selection-mode prop to AppWindow (1.08 KB, patch)
2014-07-16 15:29 UTC, Adrien Plazas
none Details | Review
collection-toolbar: Move UI state connection to setup_ui (1.37 KB, patch)
2014-07-16 15:29 UTC, Adrien Plazas
none Details | Review
app: Remove UI state (15.60 KB, patch)
2014-07-16 15:29 UTC, Adrien Plazas
none Details | Review
AppWindow: Remove use of App.window (1.43 KB, patch)
2014-07-16 15:29 UTC, Adrien Plazas
none Details | Review
app-window: Add current-item prop (1.75 KB, patch)
2014-07-17 05:07 UTC, Adrien Plazas
needs-work Details | Review
app-window: Add show_properties() (2.27 KB, patch)
2014-07-17 05:07 UTC, Adrien Plazas
needs-work Details | Review
app-window: Add connect_to() (3.87 KB, patch)
2014-07-17 05:07 UTC, Adrien Plazas
needs-work Details | Review
app-window: Add select_item() (5.16 KB, patch)
2014-07-17 05:07 UTC, Adrien Plazas
needs-work Details | Review
app-window: Cancel machine connection on collection state (2.52 KB, patch)
2014-07-17 05:08 UTC, Adrien Plazas
needs-work Details | Review
app: Move current_item to AppWindow (8.46 KB, patch)
2014-07-17 05:08 UTC, Adrien Plazas
needs-work Details | Review
app-window: Add selection-mode prop (1.88 KB, patch)
2014-07-17 05:08 UTC, Adrien Plazas
none Details | Review
collection-view: Add setup_ui() (1.30 KB, patch)
2014-07-17 05:08 UTC, Adrien Plazas
none Details | Review
selectionbar: Add setup_ui() (1.46 KB, patch)
2014-07-17 05:08 UTC, Adrien Plazas
none Details | Review
Use AppWindow's selection-mode (6.09 KB, patch)
2014-07-17 05:08 UTC, Adrien Plazas
none Details | Review
app: Move selection-mode prop to AppWindow (1.08 KB, patch)
2014-07-17 05:08 UTC, Adrien Plazas
none Details | Review
collection-toolbar: Move UI state connection to setup_ui (1.37 KB, patch)
2014-07-17 05:09 UTC, Adrien Plazas
none Details | Review
app: Remove UI state (15.60 KB, patch)
2014-07-17 05:09 UTC, Adrien Plazas
needs-work Details | Review
AppWindow: Remove use of App.window (1.43 KB, patch)
2014-07-17 05:09 UTC, Adrien Plazas
needs-work Details | Review
app-window: Add current_item prop (1.88 KB, patch)
2014-07-17 09:47 UTC, Adrien Plazas
none Details | Review
app-window: Move show_properties() from App (2.45 KB, patch)
2014-07-17 09:48 UTC, Adrien Plazas
none Details | Review
app-window: Move connect_to() from App (3.92 KB, patch)
2014-07-17 09:48 UTC, Adrien Plazas
none Details | Review
app-window: Move select_item() from App (5.18 KB, patch)
2014-07-17 09:48 UTC, Adrien Plazas
none Details | Review
app-window: Cancel machine connection on collection state (2.57 KB, patch)
2014-07-17 09:48 UTC, Adrien Plazas
none Details | Review
app: Remove current_item (8.58 KB, patch)
2014-07-17 09:48 UTC, Adrien Plazas
none Details | Review
app-window: Add selection_mode prop (1.96 KB, patch)
2014-07-17 09:49 UTC, Adrien Plazas
none Details | Review
collection-view: Add setup_ui() (1.44 KB, patch)
2014-07-17 09:49 UTC, Adrien Plazas
none Details | Review
selectionbar: Add setup_ui() (1.60 KB, patch)
2014-07-17 09:49 UTC, Adrien Plazas
none Details | Review
Use AppWindow's selection_mode (6.09 KB, patch)
2014-07-17 09:49 UTC, Adrien Plazas
none Details | Review
app: Remove selection_mode prop (1.08 KB, patch)
2014-07-17 09:49 UTC, Adrien Plazas
none Details | Review
collection-toolbar: Move UI state connection to setup_ui (1.51 KB, patch)
2014-07-17 09:50 UTC, Adrien Plazas
none Details | Review
app: Remove UI state (15.66 KB, patch)
2014-07-17 09:50 UTC, Adrien Plazas
none Details | Review
AppWindow: Remove use of App.window (1.44 KB, patch)
2014-07-17 09:50 UTC, Adrien Plazas
none Details | Review
app-window: Add 'current-item' prop (1.89 KB, patch)
2014-07-17 10:51 UTC, Adrien Plazas
none Details | Review
app-window: Move show_properties() from App (2.39 KB, patch)
2014-07-17 10:51 UTC, Adrien Plazas
none Details | Review
app-window: Move connect_to() from App (3.93 KB, patch)
2014-07-17 10:51 UTC, Adrien Plazas
none Details | Review
app-window: Move select_item() from App (5.19 KB, patch)
2014-07-17 10:52 UTC, Adrien Plazas
none Details | Review
app-window: Cancel machine connection on collection state (2.57 KB, patch)
2014-07-17 10:52 UTC, Adrien Plazas
none Details | Review
app: Remove 'current-item' (8.58 KB, patch)
2014-07-17 10:52 UTC, Adrien Plazas
none Details | Review
app-window: Add 'selection-mode' prop (1.96 KB, patch)
2014-07-17 10:52 UTC, Adrien Plazas
none Details | Review
collection-view: Add setup_ui() (1.46 KB, patch)
2014-07-17 10:52 UTC, Adrien Plazas
none Details | Review
selectionbar: Add setup_ui() (1.62 KB, patch)
2014-07-17 10:53 UTC, Adrien Plazas
none Details | Review
Use AppWindow's 'selection-mode' prop instead of App's (6.06 KB, patch)
2014-07-17 10:53 UTC, Adrien Plazas
none Details | Review
app: Remove 'selection-mode' prop (1.03 KB, patch)
2014-07-17 10:53 UTC, Adrien Plazas
none Details | Review
collection-toolbar: Move 'ui-state' connection to setup_ui() (1.49 KB, patch)
2014-07-17 10:53 UTC, Adrien Plazas
none Details | Review
app: No more implements UIState (15.64 KB, patch)
2014-07-17 10:53 UTC, Adrien Plazas
none Details | Review
app-window: Remove use of App.window (1.44 KB, patch)
2014-07-17 10:54 UTC, Adrien Plazas
none Details | Review
app-window: Add 'current-item' prop (1.93 KB, patch)
2014-07-17 12:59 UTC, Adrien Plazas
none Details | Review
app-window: Move show_properties() from App (2.43 KB, patch)
2014-07-17 12:59 UTC, Adrien Plazas
none Details | Review
app-window: Move connect_to() from App (3.97 KB, patch)
2014-07-17 12:59 UTC, Adrien Plazas
none Details | Review
app-window: Add 'current-item' prop (1.99 KB, patch)
2014-07-17 13:16 UTC, Adrien Plazas
reviewed Details | Review
app-window: Move show_properties() from App (2.49 KB, patch)
2014-07-17 13:17 UTC, Adrien Plazas
needs-work Details | Review
app-window: Move connect_to() from App (4.03 KB, patch)
2014-07-17 13:17 UTC, Adrien Plazas
reviewed Details | Review
app-window: Move select_item() from App (5.29 KB, patch)
2014-07-17 13:17 UTC, Adrien Plazas
needs-work Details | Review
app-window: Cancel machine connection on collection state (2.67 KB, patch)
2014-07-17 13:17 UTC, Adrien Plazas
needs-work Details | Review
app: Remove 'current-item' (8.68 KB, patch)
2014-07-17 13:17 UTC, Adrien Plazas
needs-work Details | Review
app-window: Add 'selection-mode' prop (2.06 KB, patch)
2014-07-17 13:18 UTC, Adrien Plazas
reviewed Details | Review
collection-view: Add setup_ui() (1.56 KB, patch)
2014-07-17 13:18 UTC, Adrien Plazas
needs-work Details | Review
selectionbar: Add setup_ui() (1.72 KB, patch)
2014-07-17 13:18 UTC, Adrien Plazas
needs-work Details | Review
Use AppWindow's 'selection-mode' prop instead of App's (6.16 KB, patch)
2014-07-17 13:18 UTC, Adrien Plazas
needs-work Details | Review
app: Remove 'selection-mode' prop (1.13 KB, patch)
2014-07-17 13:18 UTC, Adrien Plazas
needs-work Details | Review
collection-toolbar: Move 'ui-state' connection to setup_ui() (1.59 KB, patch)
2014-07-17 13:19 UTC, Adrien Plazas
needs-work Details | Review
app: No more implements UIState (15.75 KB, patch)
2014-07-17 13:19 UTC, Adrien Plazas
needs-work Details | Review
app-window: Remove use of App.window (1.54 KB, patch)
2014-07-17 13:19 UTC, Adrien Plazas
reviewed Details | Review
app-window: Add 'current-item' prop (1.99 KB, patch)
2014-07-17 18:11 UTC, Adrien Plazas
reviewed Details | Review
app-window: Move show_properties() from App (2.50 KB, patch)
2014-07-17 18:11 UTC, Adrien Plazas
reviewed Details | Review
app-window: Move connect_to() from App (4.03 KB, patch)
2014-07-17 18:11 UTC, Adrien Plazas
reviewed Details | Review
app-window: Move select_item() from App (5.29 KB, patch)
2014-07-17 18:12 UTC, Adrien Plazas
reviewed Details | Review
app-window: Disconnect machine on collection page (2.73 KB, patch)
2014-07-17 18:12 UTC, Adrien Plazas
reviewed Details | Review
app: Remove 'current-item' prop (8.69 KB, patch)
2014-07-17 18:12 UTC, Adrien Plazas
reviewed Details | Review
app-window: Add 'selection-mode' prop (2.06 KB, patch)
2014-07-17 18:12 UTC, Adrien Plazas
needs-work Details | Review
collection-view: Add setup_ui() (1.56 KB, patch)
2014-07-17 18:12 UTC, Adrien Plazas
reviewed Details | Review
selectionbar: Add setup_ui() (2.07 KB, patch)
2014-07-17 18:13 UTC, Adrien Plazas
reviewed Details | Review
Use AppWindow's 'selection-mode' prop instead of App's (5.82 KB, patch)
2014-07-17 18:13 UTC, Adrien Plazas
reviewed Details | Review
app: Remove 'selection-mode' prop (1.16 KB, patch)
2014-07-17 18:13 UTC, Adrien Plazas
reviewed Details | Review
collection-toolbar: Move 'ui-state' connection to setup_ui() (1.59 KB, patch)
2014-07-17 18:13 UTC, Adrien Plazas
reviewed Details | Review
app: No more implements UIState (15.75 KB, patch)
2014-07-17 18:14 UTC, Adrien Plazas
reviewed Details | Review
app-window: Remove use of App.window (1.54 KB, patch)
2014-07-17 18:14 UTC, Adrien Plazas
needs-work Details | Review
topbar: Add AppWindow param to setup_ui() (3.78 KB, patch)
2014-07-18 15:57 UTC, Adrien Plazas
needs-work Details | Review
collection-toolbar: Add AppWindow param to setup_ui() (3.34 KB, patch)
2014-07-18 15:57 UTC, Adrien Plazas
needs-work Details | Review
selection-toolbar: Add AppWindow param to setup_ui() (2.33 KB, patch)
2014-07-18 15:57 UTC, Adrien Plazas
needs-work Details | Review
wizard: Add AppWindow param to setup_ui() (5.36 KB, patch)
2014-07-18 15:58 UTC, Adrien Plazas
needs-work Details | Review
wizard-toolbar: Add 'wizard' prop (1.65 KB, patch)
2014-07-18 15:58 UTC, Adrien Plazas
reviewed Details | Review
display-page: Add AppWindow param to setup_ui() (3.67 KB, patch)
2014-07-18 15:58 UTC, Adrien Plazas
needs-work Details | Review
display-toolbar: Add setup_ui() (3.51 KB, patch)
2014-07-18 15:58 UTC, Adrien Plazas
none Details | Review
collection-view: Add AppWindow param to setup_ui() (3.18 KB, patch)
2014-07-18 15:58 UTC, Adrien Plazas
none Details | Review
selectionbar: Add AppWindow param to setup_ui() (2.84 KB, patch)
2014-07-18 15:59 UTC, Adrien Plazas
none Details | Review
auth-notfication: Add Searchbar param to constructor (3.42 KB, patch)
2014-07-18 15:59 UTC, Adrien Plazas
none Details | Review
searchbar: Add setup_ui() (2.67 KB, patch)
2014-07-18 15:59 UTC, Adrien Plazas
none Details | Review
sidebar: Add setup_ui() (2.31 KB, patch)
2014-07-18 15:59 UTC, Adrien Plazas
none Details | Review
properties: Add AppWindow param to setup_ui() (6.15 KB, patch)
2014-07-18 16:00 UTC, Adrien Plazas
none Details | Review
properties-toolbar: Add setup_ui() (2.04 KB, patch)
2014-07-18 16:00 UTC, Adrien Plazas
none Details | Review
empty-boxes: Add setup_ui() (1.87 KB, patch)
2014-07-18 16:00 UTC, Adrien Plazas
none Details | Review
wizard-source: Add setup_ui() (2.30 KB, patch)
2014-07-18 16:00 UTC, Adrien Plazas
none Details | Review
app: Add get_main_window() (4.17 KB, patch)
2014-07-18 16:00 UTC, Adrien Plazas
none Details | Review
machine: Add 'window' prop (9.28 KB, patch)
2014-07-18 16:01 UTC, Adrien Plazas
none Details | Review
libvirt-machine: Remove usage of App.window singleton (1.71 KB, patch)
2014-07-18 16:01 UTC, Adrien Plazas
none Details | Review
libvirt-machine-properties: Remove usage of App.window singleton (3.68 KB, patch)
2014-07-18 16:01 UTC, Adrien Plazas
none Details | Review
spice-display: Add 'machine' prop (4.51 KB, patch)
2014-07-18 16:01 UTC, Adrien Plazas
none Details | Review
app-window: Remove usage of App.window singleton (1.55 KB, patch)
2014-07-18 16:01 UTC, Adrien Plazas
none Details | Review
app: Remove window singleton (869 bytes, patch)
2014-07-18 16:02 UTC, Adrien Plazas
none Details | Review
app-window: Add 'current-item' prop (1.99 KB, patch)
2014-08-03 07:40 UTC, Adrien Plazas
committed Details | Review
app-window: Move show_properties() from App (2.50 KB, patch)
2014-08-03 07:40 UTC, Adrien Plazas
committed Details | Review
app-window: Move connect_to() from App (4.03 KB, patch)
2014-08-03 07:41 UTC, Adrien Plazas
committed Details | Review
app-window: Move select_item() from App (5.29 KB, patch)
2014-08-03 07:41 UTC, Adrien Plazas
committed Details | Review
app-window: Disconnect machine on collection page (2.73 KB, patch)
2014-08-03 07:41 UTC, Adrien Plazas
committed Details | Review
app: Remove 'current-item' prop (8.69 KB, patch)
2014-08-03 07:41 UTC, Adrien Plazas
committed Details | Review
app-window: Add 'selection-mode' prop (2.06 KB, patch)
2014-08-03 07:42 UTC, Adrien Plazas
committed Details | Review
collection-view: Add setup_ui() (1.56 KB, patch)
2014-08-03 07:42 UTC, Adrien Plazas
needs-work Details | Review
selectionbar: Add setup_ui() (2.07 KB, patch)
2014-08-03 07:42 UTC, Adrien Plazas
needs-work Details | Review
Use AppWindow's 'selection-mode' prop instead of App's (5.82 KB, patch)
2014-08-03 07:42 UTC, Adrien Plazas
needs-work Details | Review
app: Remove 'selection-mode' prop (1.16 KB, patch)
2014-08-03 07:43 UTC, Adrien Plazas
rejected Details | Review
collection-toolbar: Move 'ui-state' connection to setup_ui() (1.59 KB, patch)
2014-08-03 07:43 UTC, Adrien Plazas
none Details | Review
app: No more implements UIState (15.89 KB, patch)
2014-08-03 07:43 UTC, Adrien Plazas
none Details | Review
topbar: Add AppWindow param to setup_ui() (3.74 KB, patch)
2014-08-03 07:43 UTC, Adrien Plazas
none Details | Review
collection-toolbar: Add AppWindow param to setup_ui() (3.26 KB, patch)
2014-08-03 07:44 UTC, Adrien Plazas
none Details | Review
selection-toolbar: Add AppWindow param to setup_ui() (2.24 KB, patch)
2014-08-03 07:44 UTC, Adrien Plazas
none Details | Review
wizard: Add AppWindow param to setup_ui() (6.33 KB, patch)
2014-08-03 07:44 UTC, Adrien Plazas
none Details | Review
display-page: Add AppWindow param to setup_ui() (3.64 KB, patch)
2014-08-03 07:44 UTC, Adrien Plazas
none Details | Review
display-toolbar: Add setup_ui() (3.47 KB, patch)
2014-08-03 07:45 UTC, Adrien Plazas
none Details | Review
collection-view: Add AppWindow param to setup_ui() (3.15 KB, patch)
2014-08-03 07:45 UTC, Adrien Plazas
none Details | Review
selectionbar: Add AppWindow param to setup_ui() (2.80 KB, patch)
2014-08-03 07:45 UTC, Adrien Plazas
none Details | Review
auth-notfication: Add Searchbar param to constructor (3.42 KB, patch)
2014-08-03 07:45 UTC, Adrien Plazas
none Details | Review
searchbar: Add setup_ui() (2.63 KB, patch)
2014-08-03 07:46 UTC, Adrien Plazas
none Details | Review
sidebar: Add setup_ui() (2.28 KB, patch)
2014-08-03 07:46 UTC, Adrien Plazas
none Details | Review
properties: Add AppWindow param to setup_ui() (6.02 KB, patch)
2014-08-03 07:46 UTC, Adrien Plazas
none Details | Review
properties-toolbar: Add setup_ui() (2.01 KB, patch)
2014-08-03 07:46 UTC, Adrien Plazas
none Details | Review
empty-boxes: Add setup_ui() (1.78 KB, patch)
2014-08-03 07:47 UTC, Adrien Plazas
none Details | Review
wizard-source: Add setup_ui() (6.04 KB, patch)
2014-08-03 07:47 UTC, Adrien Plazas
none Details | Review
machine: Add 'window' prop (9.28 KB, patch)
2014-08-03 07:47 UTC, Adrien Plazas
none Details | Review
libvirt-machine: Remove usage of App.window singleton (1.71 KB, patch)
2014-08-03 07:47 UTC, Adrien Plazas
none Details | Review
libvirt-machine-properties: Remove usage of App.window singleton (3.68 KB, patch)
2014-08-03 07:48 UTC, Adrien Plazas
none Details | Review
spice-display: Add 'machine' prop (4.51 KB, patch)
2014-08-03 07:48 UTC, Adrien Plazas
none Details | Review
app-window: Remove usage of App.window singleton (1.55 KB, patch)
2014-08-03 07:48 UTC, Adrien Plazas
none Details | Review
app: Remove window singleton (869 bytes, patch)
2014-08-03 07:48 UTC, Adrien Plazas
none Details | Review
wizard-source: Add setup_ui() (2.33 KB, patch)
2014-08-03 08:19 UTC, Adrien Plazas
none Details | Review
app: Add get_main_window() (4.23 KB, patch)
2014-08-03 08:20 UTC, Adrien Plazas
none Details | Review
machine: Add 'window' prop (9.28 KB, patch)
2014-08-03 08:20 UTC, Adrien Plazas
none Details | Review
libvirt-machine: Remove usage of App.window singleton (1.71 KB, patch)
2014-08-03 08:20 UTC, Adrien Plazas
none Details | Review
libvirt-machine-properties: Remove usage of App.window singleton (3.68 KB, patch)
2014-08-03 08:20 UTC, Adrien Plazas
none Details | Review
spice-display: Add 'machine' prop (4.51 KB, patch)
2014-08-03 08:20 UTC, Adrien Plazas
none Details | Review
app-window: Remove usage of App.window singleton (1.55 KB, patch)
2014-08-03 08:21 UTC, Adrien Plazas
none Details | Review
app: Remove window singleton (869 bytes, patch)
2014-08-03 08:21 UTC, Adrien Plazas
none Details | Review
app: Replace window singleton by windows list (6.97 KB, patch)
2014-08-03 12:06 UTC, Adrien Plazas
none Details | Review
app: Add WindowsCollection class (1.90 KB, patch)
2014-08-04 15:27 UTC, Adrien Plazas
none Details | Review
app: Replace AppWindow singleton by WindowsCollection (6.92 KB, patch)
2014-08-04 15:27 UTC, Adrien Plazas
none Details | Review
app: Remove get_main_window() (9.31 KB, patch)
2014-08-04 15:28 UTC, Adrien Plazas
none Details | Review
app-window: Closing window removes it from collection (1.64 KB, patch)
2014-08-04 15:28 UTC, Adrien Plazas
none Details | Review
app-window: Present when machine selected (1.03 KB, patch)
2014-08-04 15:28 UTC, Adrien Plazas
none Details | Review
app-window: Add 'main' prop (2.39 KB, patch)
2014-08-04 15:28 UTC, Adrien Plazas
none Details | Review
app: Populate new main window's collection (1.40 KB, patch)
2014-08-04 15:29 UTC, Adrien Plazas
none Details | Review
collection-view: Setup UI once window is constructed (1.58 KB, patch)
2014-08-06 12:12 UTC, Adrien Plazas
committed Details | Review
selectionbar: Setup UI once window is constructed (2.09 KB, patch)
2014-08-06 12:12 UTC, Adrien Plazas
committed Details | Review
Use AppWindow's 'selection-mode' prop instead of App's (6.39 KB, patch)
2014-08-06 12:13 UTC, Adrien Plazas
committed Details | Review
collection-toolbar: Move 'ui-state' connection to setup_ui() (1.59 KB, patch)
2014-08-06 12:13 UTC, Adrien Plazas
needs-work Details | Review
app: No more implements UIState (15.89 KB, patch)
2014-08-06 12:13 UTC, Adrien Plazas
accepted-commit_now Details | Review
topbar: Add AppWindow param to setup_ui() (3.74 KB, patch)
2014-08-06 12:14 UTC, Adrien Plazas
needs-work Details | Review
collection-toolbar: Add AppWindow param to setup_ui() (3.26 KB, patch)
2014-08-06 12:14 UTC, Adrien Plazas
needs-work Details | Review
selection-toolbar: Add AppWindow param to setup_ui() (2.24 KB, patch)
2014-08-06 12:14 UTC, Adrien Plazas
needs-work Details | Review
wizard: Add AppWindow param to setup_ui() (6.33 KB, patch)
2014-08-06 12:14 UTC, Adrien Plazas
needs-work Details | Review
display-page: Add AppWindow param to setup_ui() (3.64 KB, patch)
2014-08-06 12:15 UTC, Adrien Plazas
needs-work Details | Review
display-toolbar: Add setup_ui() (3.47 KB, patch)
2014-08-06 12:15 UTC, Adrien Plazas
needs-work Details | Review
collection-view: Add AppWindow param to setup_ui() (3.15 KB, patch)
2014-08-06 12:15 UTC, Adrien Plazas
needs-work Details | Review
selectionbar: Add AppWindow param to setup_ui() (2.80 KB, patch)
2014-08-06 12:15 UTC, Adrien Plazas
needs-work Details | Review
auth-notfication: Add Searchbar param to constructor (3.42 KB, patch)
2014-08-06 12:16 UTC, Adrien Plazas
needs-work Details | Review
searchbar: Add setup_ui() (2.63 KB, patch)
2014-08-06 12:16 UTC, Adrien Plazas
needs-work Details | Review
sidebar: Add setup_ui() (2.28 KB, patch)
2014-08-06 12:16 UTC, Adrien Plazas
needs-work Details | Review
properties: Add AppWindow param to setup_ui() (6.02 KB, patch)
2014-08-06 12:17 UTC, Adrien Plazas
needs-work Details | Review
properties-toolbar: Add setup_ui() (2.01 KB, patch)
2014-08-06 12:17 UTC, Adrien Plazas
needs-work Details | Review
empty-boxes: Add setup_ui() (1.78 KB, patch)
2014-08-06 12:17 UTC, Adrien Plazas
needs-work Details | Review
wizard-source: Add setup_ui() (2.33 KB, patch)
2014-08-06 12:17 UTC, Adrien Plazas
needs-work Details | Review
app: Add get_main_window() (4.23 KB, patch)
2014-08-06 12:18 UTC, Adrien Plazas
needs-work Details | Review
machine: Add 'window' prop (9.28 KB, patch)
2014-08-06 12:18 UTC, Adrien Plazas
needs-work Details | Review
libvirt-machine: Remove usage of App.window singleton (1.71 KB, patch)
2014-08-06 12:18 UTC, Adrien Plazas
accepted-commit_now Details | Review
libvirt-machine-properties: Remove usage of App.window singleton (3.68 KB, patch)
2014-08-06 12:18 UTC, Adrien Plazas
needs-work Details | Review
spice-display: Add 'machine' prop (4.51 KB, patch)
2014-08-06 12:19 UTC, Adrien Plazas
needs-work Details | Review
app-window: Remove usage of App.window singleton (1.55 KB, patch)
2014-08-06 12:19 UTC, Adrien Plazas
none Details | Review
app: Add WindowsCollection class (1.90 KB, patch)
2014-08-06 12:19 UTC, Adrien Plazas
none Details | Review
app: Replace AppWindow singleton by WindowsCollection (6.92 KB, patch)
2014-08-06 12:19 UTC, Adrien Plazas
none Details | Review
app: Remove get_main_window() (9.31 KB, patch)
2014-08-06 12:20 UTC, Adrien Plazas
none Details | Review
app-window: Closing window removes it from collection (1.64 KB, patch)
2014-08-06 12:20 UTC, Adrien Plazas
none Details | Review
app-window: Present when machine selected (1.03 KB, patch)
2014-08-06 12:20 UTC, Adrien Plazas
none Details | Review
app-window: Add 'main' prop (2.39 KB, patch)
2014-08-06 12:21 UTC, Adrien Plazas
none Details | Review
app: Populate new main window's collection (1.40 KB, patch)
2014-08-06 12:21 UTC, Adrien Plazas
none Details | Review
collection-toolbar: Move 'ui-state' connection to setup_ui() (1.60 KB, patch)
2014-08-07 11:52 UTC, Adrien Plazas
committed Details | Review
app: No more implements UIState (15.89 KB, patch)
2014-08-07 11:52 UTC, Adrien Plazas
committed Details | Review
topbar: Drop use of AppWindow singleton (3.82 KB, patch)
2014-08-07 11:52 UTC, Adrien Plazas
committed Details | Review
collection-toolbar: Drop use of AppWindow singleton (3.33 KB, patch)
2014-08-07 11:53 UTC, Adrien Plazas
committed Details | Review
selection-toolbar: Drop use of AppWindow singleton (2.32 KB, patch)
2014-08-07 11:53 UTC, Adrien Plazas
committed Details | Review
wizard: Drop use of AppWindow singleton (6.41 KB, patch)
2014-08-07 11:53 UTC, Adrien Plazas
committed Details | Review
display-page: Drop use of AppWindow singleton (3.71 KB, patch)
2014-08-07 11:54 UTC, Adrien Plazas
committed Details | Review
display-toolbar: Drop use of AppWindow singleton (3.57 KB, patch)
2014-08-07 11:54 UTC, Adrien Plazas
committed Details | Review
collection-view: Drop use of AppWindow singleton (3.23 KB, patch)
2014-08-07 11:54 UTC, Adrien Plazas
committed Details | Review
selectionbar: Drop use of AppWindow singleton (2.88 KB, patch)
2014-08-07 11:54 UTC, Adrien Plazas
committed Details | Review
auth-notfication: Drop use of AppWindow singleton (3.42 KB, patch)
2014-08-07 11:55 UTC, Adrien Plazas
committed Details | Review
searchbar: Drop use of AppWindow singleton (2.73 KB, patch)
2014-08-07 11:55 UTC, Adrien Plazas
committed Details | Review
sidebar: Drop use of AppWindow singleton (2.37 KB, patch)
2014-08-07 11:55 UTC, Adrien Plazas
committed Details | Review
properties: Drop use of AppWindow singleton (6.02 KB, patch)
2014-08-07 11:56 UTC, Adrien Plazas
committed Details | Review
properties-toolbar: Drop use of AppWindow singleton (2.02 KB, patch)
2014-08-07 11:56 UTC, Adrien Plazas
committed Details | Review
empty-boxes: Drop use of AppWindow singleton (1.88 KB, patch)
2014-08-07 11:56 UTC, Adrien Plazas
committed Details | Review
wizard-source: Drop use of AppWindow singleton (2.43 KB, patch)
2014-08-07 11:57 UTC, Adrien Plazas
committed Details | Review
app: Add 'main-window' prop (4.20 KB, patch)
2014-08-07 11:57 UTC, Adrien Plazas
rejected Details | Review
machine: Drop use of AppWindow singleton (9.34 KB, patch)
2014-08-07 11:57 UTC, Adrien Plazas
none Details | Review
libvirt-machine: Drop use of AppWindow singleton (1.70 KB, patch)
2014-08-07 11:57 UTC, Adrien Plazas
none Details | Review
libvirt-machine-properties: Drop use of AppWindow singleton (3.68 KB, patch)
2014-08-07 11:58 UTC, Adrien Plazas
none Details | Review
spice-display: Add 'machine' prop (4.60 KB, patch)
2014-08-07 11:58 UTC, Adrien Plazas
none Details | Review
app-window: Remove usage of App.window singleton (1.55 KB, patch)
2014-08-07 11:58 UTC, Adrien Plazas
none Details | Review
app: Add WindowsCollection class (1.89 KB, patch)
2014-08-07 11:58 UTC, Adrien Plazas
rejected Details | Review
app: Replace AppWindow singleton by WindowsCollection (6.81 KB, patch)
2014-08-07 11:59 UTC, Adrien Plazas
none Details | Review
app: Remove 'main-window' prop (9.01 KB, patch)
2014-08-07 11:59 UTC, Adrien Plazas
none Details | Review
app-window: Add 'main' prop (2.38 KB, patch)
2014-08-07 11:59 UTC, Adrien Plazas
none Details | Review
app-window: Closing window removes it from collection (1.60 KB, patch)
2014-08-07 12:00 UTC, Adrien Plazas
needs-work Details | Review
app-window: Present when machine selected (1.03 KB, patch)
2014-08-07 12:00 UTC, Adrien Plazas
needs-work Details | Review
app: Populate new main window's collection (1.40 KB, patch)
2014-08-07 12:00 UTC, Adrien Plazas
needs-work Details | Review
app: Add 'main-window' prop (10.98 KB, patch)
2014-08-08 07:44 UTC, Adrien Plazas
needs-work Details | Review
machine: Drop use of AppWindow singleton (9.34 KB, patch)
2014-08-08 07:45 UTC, Adrien Plazas
none Details | Review
libvirt-machine: Drop use of AppWindow singleton (1.70 KB, patch)
2014-08-08 07:45 UTC, Adrien Plazas
none Details | Review
libvirt-machine-properties: Drop use of AppWindow singleton (3.68 KB, patch)
2014-08-08 07:45 UTC, Adrien Plazas
none Details | Review
spice-display: Add 'machine' prop (4.60 KB, patch)
2014-08-08 07:46 UTC, Adrien Plazas
none Details | Review
app-window: Remove usage of App.window singleton (1.55 KB, patch)
2014-08-08 07:46 UTC, Adrien Plazas
none Details | Review
app: Replace AppWindow singleton by List<Boxes.AppWindow> (2.69 KB, patch)
2014-08-08 07:46 UTC, Adrien Plazas
none Details | Review
app-window: Closing window removes it from collection (1.56 KB, patch)
2014-08-08 07:46 UTC, Adrien Plazas
none Details | Review
Set toolbar UI depending on if window is the main one (1.77 KB, patch)
2014-08-08 07:47 UTC, Adrien Plazas
none Details | Review
app-window: Present window when machine selected (1.48 KB, patch)
2014-08-08 07:47 UTC, Adrien Plazas
none Details | Review
app: Populate new main window's collection (1.66 KB, patch)
2014-08-08 07:47 UTC, Adrien Plazas
none Details | Review
app: Add 'main-window' prop (11.09 KB, patch)
2014-08-08 15:36 UTC, Adrien Plazas
committed Details | Review
machine: Drop use of AppWindow singleton (9.34 KB, patch)
2014-08-08 15:36 UTC, Adrien Plazas
committed Details | Review
libvirt-machine: Drop use of AppWindow singleton (1.70 KB, patch)
2014-08-08 15:36 UTC, Adrien Plazas
committed Details | Review
libvirt-machine-properties: Drop use of AppWindow singleton (3.68 KB, patch)
2014-08-08 15:37 UTC, Adrien Plazas
committed Details | Review
spice-display: Add 'machine' prop (4.60 KB, patch)
2014-08-08 15:37 UTC, Adrien Plazas
committed Details | Review
app-window: Remove usage of App.window singleton (1.55 KB, patch)
2014-08-08 15:37 UTC, Adrien Plazas
committed Details | Review

Description Zeeshan Ali 2014-06-23 12:18:16 UTC
Its about implementing this mockup:

https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/boxes/wires/multi-window.png

This will require quite a bit of changes in code layout.
Comment 1 Adrien Plazas 2014-06-24 11:00:05 UTC
Created attachment 279105 [details] [review]
topbar: Remove App and AppWindow dependencies

Replace handling of parent singletons in Topbar with signals and properties,
let the parents handle the consequences.

https://bugzilla.gnome.org/show_bug.cgi?id=710304
Comment 2 Adrien Plazas 2014-06-24 11:00:08 UTC
Created attachment 279106 [details] [review]
searchbar: Remove App and AppWindow dependencies

Replace handling of parent singletons in Searchbar with signals and properties,
let the parents handle the consequences.

https://bugzilla.gnome.org/show_bug.cgi?id=710304
Comment 3 Adrien Plazas 2014-06-24 11:00:11 UTC
Created attachment 279107 [details] [review]
properties-toolbar: Remove App dependencies

Replace handling of App singleton in PropertiesToolbar with a signal,
let the parents handle the consequences.

https://bugzilla.gnome.org/show_bug.cgi?id=710304
Comment 4 Adrien Plazas 2014-06-24 11:00:15 UTC
Created attachment 279108 [details] [review]
selection-toolbar: Remove App and AppWindow dependencies

Replace handling of parent singletons in SelectionToolbar with signals and properties,
let the parents handle the consequences.

https://bugzilla.gnome.org/show_bug.cgi?id=710304
Comment 5 Adrien Plazas 2014-06-24 11:00:18 UTC
Created attachment 279109 [details] [review]
collection-toolbar: Remove App dependencies

Replace handling of App singleton in CollectionToolbar with a signals and properties,
let the parents handle the consequences.

https://bugzilla.gnome.org/show_bug.cgi?id=710304
Comment 6 Adrien Plazas 2014-06-24 11:02:00 UTC
I am not sure if the long description are too short or even just useless.
Also don't hesitate to share any problem you have with my coding style or whatever.
Comment 7 Lasse Schuirmann 2014-06-24 11:06:49 UTC
Review of attachment 279105 [details] [review]:

- Did you notice you have two bugzilla lines in there? I think its better to refer to this bug and link this bug via "See also" to the other.
- I would omit "singletons" in all the logs
Comment 8 Adrien Plazas 2014-06-24 11:11:00 UTC
Lasse: Woops, I forgot the chage the bug's reference. THanks.

Also I am not sure if we should keep the implementation of an object (widgets in that case) private and explicitly transmit the signals and property changes of its members, or let them be public to be accessed directly.
Comment 9 Lasse Schuirmann 2014-06-25 08:49:45 UTC
(In reply to comment #8)
> Lasse: Woops, I forgot the chage the bug's reference. THanks.

I review bigger patches of mine in BZ myself because its way easier to get the overview than the diff. (Its sometimes a bit bad when I spot errors there and people are getting six useless mails or so but I think its better than not spotting the errors.)

> Also I am not sure if we should keep the implementation of an object (widgets
> in that case) private and explicitly transmit the signals and property changes
> of its members, or let them be public to be accessed directly.

For me signals should add some abstraction over the function its connected to. So if you want to have a changable property for one object, go ahead with a property. If you want to change the property of an object and choose that dependend of the context you might want to add a signal and reroute it on context change (example could be this click back button thing ;))
Comment 10 Lasse Schuirmann 2014-06-25 10:38:54 UTC
Review of attachment 279105 [details] [review]:

I like it so far.

::: src/topbar.vala
@@ +35,3 @@
     // Clicks the appropriate back button depending on the ui state.
     public void click_back_button () {
+        switch (ui_state) {

You can put these things into an own patch with summary Codestyle changes and in the description you provide a list with every type of change you made. I just did this with
https://bug729026.bugzilla-attachments.gnome.org/attachment.cgi?id=279100

@@ +54,3 @@
     // Clicks the appropriate cancel button dependent on the ui state.
     public void click_cancel_button () {
+        switch (ui_state) {

Same here.

@@ +57,2 @@
         case UIState.COLLECTION:
+            selection_cancel_clicked ();

If you have a signal for selection cancellation you should also have some for the wizard cancel button.

So out of consistency and because it makes more sense for me: how about changing this line to
selection_toolbar.cancel_btn.clicked ();

You could also refactor this line and line 61 to be consistent with lines 41, 44 and 51 in an own patch. (Meaning: do another patch that introduces selection_toolbar.click_cancel_button (), the same for the wizard_toolbar.)
Comment 11 Lasse Schuirmann 2014-06-25 10:48:51 UTC
Review of attachment 279105 [details] [review]:

::: src/topbar.vala
@@ +57,2 @@
         case UIState.COLLECTION:
+            selection_cancel_clicked ();

For good modularity (and we need that for multiple windows) we basically need to reroute all click events from the topbar out so your signal approach is the right way. We just have to do this with everything. So topbar could get a buch of signals
- selection_cancel () // its an abstraction, its not necessarily a click action
- wizard_cancel ()
- wizard_back ()
- wizard_forward ()
...

But I'd introduce all these signals in one own patch and stick to selection_toolbar.cancel_btn.clicked (); in this one.
Comment 12 Lasse Schuirmann 2014-06-25 11:31:53 UTC
Review of attachment 279105 [details] [review]:

::: src/topbar.vala
@@ +35,3 @@
     // Clicks the appropriate back button depending on the ui state.
     public void click_back_button () {
+        switch (ui_state) {

ignore this stupid comment.

@@ +54,3 @@
     // Clicks the appropriate cancel button dependent on the ui state.
     public void click_cancel_button () {
+        switch (ui_state) {

and this too.
Comment 13 Zeeshan Ali 2014-06-26 15:50:25 UTC
To avoid confusions, I'll postpone my reviews until Lasse is done with his. Give me a shout Lasse once you are satisfied and thanks for reviewing.
Comment 14 Lasse Schuirmann 2014-06-27 10:41:00 UTC
Review of attachment 279106 [details] [review]:

Looks good. The comment in the last patch about the commit message applies to all patches.

::: src/searchbar.vala
@@ +36,3 @@
         search_mode_enabled = false;
 
+        notify["enable_key_handler"].connect (update_key_handler);

It seems cleaner for me to put an
  update_key_handler ();
invokation into the setter of enable_key_handler. Theres no need to be able to reroute such things here and it's easier to just look at the prop setter to see whats happening instead of having to scan the whole code for signal connections.

What do you think?
Comment 15 Lasse Schuirmann 2014-06-27 10:51:37 UTC
Review of attachment 279107 [details] [review]:

Looks good for me except the one comment and the commit message.

::: src/topbar.vala
@@ +97,3 @@
         transition_type = Gtk.StackTransitionType.CROSSFADE; // FIXME: Why this won't work from .ui file?
+
+        props_toolbar.back_clicked.connect (on_child_back_clicked);

on_child_back_clicked is a weird name. Also readability increases for me if you put the back_clicked () in a lambda here because you dont have to jump down for finding only this one line.
Comment 16 Lasse Schuirmann 2014-06-27 11:49:47 UTC
Review of attachment 279108 [details] [review]:

Another thing about all commit logs: the singletons aren't the parents of the things you modularize. Mother of SelectionToolbar is HeaderBar (father probably ran away.)

::: src/selection-toolbar.vala
@@ +39,3 @@
 
+    public bool search_active { set; get; }
+    public uint items { set; get; default = 0; }

Name suggests you hold the actual items in there. How about selected_items_count ?

@@ +45,2 @@
+    construct {
+

This \n doesnt increase readability for me.

@@ +66,2 @@
     private void update_search_btn () {
+        search_btn.sensitive = collection != null && collection.items.length != 0;

Anything against some brackets for readability?

::: src/topbar.vala
@@ +99,3 @@
 
+    public bool search_active { set; get; }
+    public uint items { set; get; default = 0; }

see comment on the other items member in selection-toolbar
Comment 17 Lasse Schuirmann 2014-06-27 13:16:09 UTC
Review of attachment 279109 [details] [review]:

I like the direction where these patches are leading much!

::: src/app-window.vala
@@ +132,3 @@
+                break;
+            }
+        });

I dislike long lambdas. IMHO this belongs in an own function.

::: src/collection-toolbar.vala
@@ +77,2 @@
     private void update_search_btn () {
+        search_btn.sensitive = collection != null && collection.items.length != 0;

IMHO there could be four more brackets in this line

@@ +81,2 @@
     private void update_select_btn () {
+        select_btn.sensitive = collection != null && collection.items.length != 0;

here too

@@ +87,3 @@
     }
 
+    public void set_creds_state (bool creds_state) {

I liked the switch version better. How about not accepting a bool parameter here but a UIState (dont remember the exact type name)? Its way easier to find out that the second state is indeed the COLLECTION state and not everything else. It is also easier to invoke, you don't need to put it in the switch in topbar.vala line 150/155

@@ +94,3 @@
             search_btn.hide ();
+        }
+        else {

this else shouldnt have an own line
Comment 18 Adrien Plazas 2014-07-04 07:15:03 UTC
Created attachment 279879 [details] [review]
wizard: Replace buttons with toolbar
Comment 19 Adrien Plazas 2014-07-04 07:15:07 UTC
Created attachment 279880 [details] [review]
wizard: Notify failed installation
Comment 20 Adrien Plazas 2014-07-04 07:15:11 UTC
Created attachment 279881 [details] [review]
wizard: Notify successful installation
Comment 21 Adrien Plazas 2014-07-04 07:15:15 UTC
Created attachment 279882 [details] [review]
app-window: Edit sidebar's wizard page
Comment 22 Adrien Plazas 2014-07-04 07:15:19 UTC
Created attachment 279883 [details] [review]
wizard: Bind page to toolbar's page
Comment 23 Adrien Plazas 2014-07-04 07:15:24 UTC
Created attachment 279884 [details] [review]
vm-creator: Notify failed installation
Comment 24 Adrien Plazas 2014-07-04 07:25:45 UTC
I started a new series of lighter commits, they should be easier to review.
Comment 25 Lasse Schuirmann 2014-07-04 08:13:03 UTC
Review of attachment 279879 [details] [review]:

Log: you say you replace the buttons with a toolbar? Sounds weird...

You could also refer to commit e75349 since that's when the wizard toolbar is introduced.

How about:
---
wizard: Use wizard toolbar

Make use of wizard toolbar introduced in e75349 to make objects more
independent from each other.

This is necessary to make multiple windows possible.
---
It also is not perfect but I'm not the right guy for commit messages I presume ;)Maybe you can come up with a better summary line. Zeenix will probably have some comments about this later.

Some general things about the comments below: all these buttons are members of the wizard toolbar (and the next_button should be too). So their properties (e.g. visibility) should be handled by the WizardToolbar.

::: src/wizard.vala
@@ +57,3 @@
             case WizardPage.INTRODUCTION:
+                toolbar.create_btn.visible = false;
+                toolbar.continue_btn.visible = true;

Couldn't the wizard toolbar just have a page setting too? So you could just set this to value in the beginning or so. Would make this horrible function a very little bit friendlier.

@@ +61,2 @@
                 next_button.sensitive = true;
                 next_button.grab_focus ();

I think this next_button and all the sensitive and focus things also belong to the wizard-toolbar.
Comment 26 Lasse Schuirmann 2014-07-04 08:36:45 UTC
Review of attachment 279880 [details] [review]:

Looks good. Thing about the log: its not that you introduce the message to the user, you rather convert it to a signal. Can you change log to reflect that?
Comment 27 Lasse Schuirmann 2014-07-04 08:40:53 UTC
Review of attachment 279881 [details] [review]:

Same as last patch
Comment 28 Lasse Schuirmann 2014-07-04 08:46:03 UTC
Review of attachment 279882 [details] [review]:

I dislike the commit message but I don't have a better one :/ Else: looks good for me.
Comment 29 Lasse Schuirmann 2014-07-04 09:03:06 UTC
Review of attachment 279883 [details] [review]:

Log: how about:
wizard-toolbar: Remove App dependency

or so.
Comment 30 Lasse Schuirmann 2014-07-04 09:04:01 UTC
Review of attachment 279884 [details] [review]:

same as with the other notify patches
Comment 31 Zeeshan Ali 2014-07-04 12:24:46 UTC
Review of attachment 279882 [details] [review]:

Edit -> Set

Some rationale in the logs would be nice. Same goes for other patches.
Comment 32 Lasse Schuirmann 2014-07-04 12:49:40 UTC
Review of attachment 279882 [details] [review]:

Yes, we already discussed about the rationale on IRC. Good that you're jumping in. It feels weird having "This is necessary to make multiple windows possible." below so many commit messages (thats what I suggested for another patch.) How would you do it?
Comment 33 Zeeshan Ali 2014-07-04 13:00:55 UTC
Review of attachment 279882 [details] [review]:

I haven't looked at the other patches so I don't know yet but "This is necessary to make multiple windows possible" is not good enough on its own cause it doesn't indicate anything about the connection between the change and multiple windows.
Comment 34 Lasse Schuirmann 2014-07-04 13:01:57 UTC
Review of attachment 279882 [details] [review]:

We'll figure something out.
Comment 35 Adrien Plazas 2014-07-07 05:39:15 UTC
(In reply to comment #25)
> Log: you say you replace the buttons with a toolbar? Sounds weird...

I have o idea to make it sound better while still being concise. =/

> You could also refer to commit e75349 since that's when the wizard toolbar is
> introduced.

I am not sure it is useful, is it?

> It also is not perfect but I'm not the right guy for commit messages I presume
> ;)Maybe you can come up with a better summary line. Zeenix will probably have
> some comments about this later.

> Some general things about the comments below: all these buttons are members of
> the wizard toolbar (and the next_button should be too). So their properties
> (e.g. visibility) should be handled by the WizardToolbar.

Correct, I'll add this in another commit.
Comment 36 Adrien Plazas 2014-07-07 05:42:40 UTC
(In reply to comment #26)
> Looks good. Thing about the log: its not that you introduce the message to the
> user, you rather convert it to a signal. Can you change log to reflect that?

A signal is used by a class to notify other parts of the code, I am not sure using another word would make the distinction from notifying the user more clear.
Comment 37 Lasse Schuirmann 2014-07-07 06:11:40 UTC
(In reply to comment #35)
> (In reply to comment #25)
> > Log: you say you replace the buttons with a toolbar? Sounds weird...
> 
> I have o idea to make it sound better while still being concise. =/
> 
> > You could also refer to commit e75349 since that's when the wizard toolbar is
> > introduced.
> 
> I am not sure it is useful, is it?

As you mentioned below this patch will be splitted anyway, maybe we can come up with a better commit message for the net patches then because smaller changes are easier to describe.

> > Some general things about the comments below: all these buttons are members of
> > the wizard toolbar (and the next_button should be too). So their properties
> > (e.g. visibility) should be handled by the WizardToolbar.
> 
> Correct, I'll add this in another commit.

:)

A thing about the rationale for the patches, you probably have seen that I discussed this with zeenix. (https://bugzilla.gnome.org/review?bug=732098&attachment=279882) A sentence like "This is necessary to make multiple windows work" is not enought. So, I'm sorry and I know that's difficult, you'll have to come up with a more detailed reasoning sentence for each patch individually. Maybe something like this will do:
"wizard: Use wizard toolbar

Make use of wizard toolbar introduced in e75349 to make the wizard
more independent from App and AppWindow singletons preventing the
implementation of the multi monitor feature."

No pressure to get it right the first time, you can approach zeenix if you need more help on this, he usually has good ideas on this.

A thing about bugzilla: it would be way easier to follow if you reply in the review view. If you don't know what I mean ask me on irc :)
Comment 38 Lasse Schuirmann 2014-07-07 06:15:09 UTC
(In reply to comment #36)
> (In reply to comment #26)
> > Looks good. Thing about the log: its not that you introduce the message to the
> > user, you rather convert it to a signal. Can you change log to reflect that?
> 
> A signal is used by a class to notify other parts of the code, I am not sure
> using another word would make the distinction from notifying the user more
> clear.

"Introduce failed_installation signal"? Plus some (short) description.
Comment 39 Adrien Plazas 2014-07-07 08:06:30 UTC
Created attachment 280027 [details] [review]
wizard: Use WizardToolbar

Use the buttons from a configurable WizardToolbar instead of the ones from
App.window.topbar.wizard_toolbar to allow multiple windows.
Comment 40 Adrien Plazas 2014-07-07 08:06:35 UTC
Created attachment 280028 [details] [review]
wizard: Notify failed installation

Add a signal to the Wizard class to notify on failed installations.
Allows to display the notification in another way than with
App.window.notificationbar.
Comment 41 Adrien Plazas 2014-07-07 08:06:40 UTC
Created attachment 280029 [details] [review]
wizard: Notify successful installation

Add a signal to the Wizard class to notify on successful installations.
Help to untie the Wizard class from the App's UI state to allow multiple
UI states.
Comment 42 Adrien Plazas 2014-07-07 08:06:45 UTC
Created attachment 280030 [details] [review]
app-window: Set sidebar's wizard page

Set the sidebar's wizard page from the AppWindow class for more modularity.
Comment 43 Adrien Plazas 2014-07-07 08:06:49 UTC
Created attachment 280031 [details] [review]
wizard: Bind page to toolbar's page

Add a WizardPage to the wizard toolbar and bind it to a wizard's page so it
can check other pages than App.window.wizard.page.
Comment 44 Adrien Plazas 2014-07-07 08:06:54 UTC
Created attachment 280032 [details] [review]
vm-creator: Notify failed installation

Add a signal to the VMCreator class to notify on failed installations.
Allows to display the notification in another way than with
App.window.notificationbar.
Comment 45 Adrien Plazas 2014-07-07 08:06:59 UTC
Created attachment 280033 [details] [review]
wizard-toolbar: Set buttons depending on its page

The wizard toolbar now set its buttons depending on its page for a better
separation from the Wizard class.
Comment 46 Lasse Schuirmann 2014-07-07 08:19:10 UTC
Review of attachment 280029 [details] [review]:

"Help to untie the Wizard class from the App's UI state to allow multiple
UI states." <- I don't really understand this sentence.

::: src/wizard.vala
@@ +210,2 @@
     public signal void installation_failed (string message);
+    public signal void creation_success ();

How about creation_finished as name for this signal?
Comment 47 Lasse Schuirmann 2014-07-07 08:19:10 UTC
Review of attachment 280029 [details] [review]:

"Help to untie the Wizard class from the App's UI state to allow multiple
UI states." <- I don't really understand this sentence.

::: src/wizard.vala
@@ +210,2 @@
     public signal void installation_failed (string message);
+    public signal void creation_success ();

How about creation_finished as name for this signal?
Comment 48 Lasse Schuirmann 2014-07-07 08:21:46 UTC
Review of attachment 280028 [details] [review]:

Minor thing for the commit log: you might want to mention that you, at least partially, remove the App.window dependency.
Comment 49 Lasse Schuirmann 2014-07-07 08:25:43 UTC
Review of attachment 280027 [details] [review]:

We probably want to first introduce the buttons to the toolbar (https://bugzilla.gnome.org/review?bug=732098&attachment=280033) and then apply this patch (which will be smaller then!), do you agree?
Comment 50 Lasse Schuirmann 2014-07-07 08:26:21 UTC
Review of attachment 280029 [details] [review]:

.
Comment 51 Lasse Schuirmann 2014-07-07 08:29:09 UTC
Review of attachment 280030 [details] [review]:

looks good for me. Lets wait for the other patches to be ready so zeenix gets a clean patch series for review.
Comment 52 Adrien Plazas 2014-07-07 08:38:00 UTC
(In reply to comment #46)
> How about creation_finished as name for this signal?

I can be finished without being successful: if it fails it is also finished.

(In reply to comment #48)
> Minor thing for the commit log: you might want to mention that you, at least
> partially, remove the App.window dependency.

Isn't it clear enough when I say it doesn't use App.window.notificationbar anymore?

(In reply to comment #49)

> We probably want to first introduce the buttons to the toolbar
> (https://bugzilla.gnome.org/review?bug=732098&attachment=280033) and then apply
> this patch (which will be smaller then!), do you agree?

We may do that but is it useful to introduce the code in the WizardToolbar without using it?
Comment 53 Adrien Plazas 2014-07-07 09:02:40 UTC
Created attachment 280038 [details] [review]
wizard: Notify successful installation

Add a signal to the Wizard class to notify on successful installations
to untie it from its consequences.
Comment 54 Adrien Plazas 2014-07-07 12:11:14 UTC
Review of attachment 280033 [details] [review]:

I n this patch I add code to WizardToolbar and remove code from Wizard, but I don't think I can split it more: as the page property is already used, if I add code depending on the page in WizardToolbar, I have to remove the code doing the same thing from Wizard or both will be run.
Comment 55 Adrien Plazas 2014-07-07 12:49:18 UTC
Created attachment 280056 [details] [review]
topbar: Notify clicked cancel

Add a signal to the Topbar class to notify clicks on a cancel button.
Allows to let the window handle the selection mode and the UI state
for more modularity.
Comment 56 Adrien Plazas 2014-07-07 12:49:23 UTC
Created attachment 280057 [details] [review]
topbar: Check its own ui state

The Topbar class now checks its own UI state to untie it
from the App's one.
Comment 57 Adrien Plazas 2014-07-07 12:49:28 UTC
Created attachment 280058 [details] [review]
app-window: Bind current item to properties title

Bind the current item's name to the topbar's properties title.
Help to untie the Topbar class from App.app.current_item.
Comment 58 Adrien Plazas 2014-07-07 12:49:33 UTC
Created attachment 280059 [details] [review]
topbar: Set selection mode

Add a way to set the Topbar's selection mode
to untie it from App.app.selection_mode.
Comment 59 Adrien Plazas 2014-07-07 12:49:38 UTC
Created attachment 280060 [details] [review]
topbar: Set display title

Bind the display Toolbar's title to another configurable one
to untie it from App.window.display_page.toolbar.
Comment 60 Adrien Plazas 2014-07-08 06:47:42 UTC
Created attachment 280108 [details] [review]
searchbar: Add window property

Add a window property to the Searchbar class to untie it from App.window.
Comment 61 Adrien Plazas 2014-07-08 06:47:48 UTC
Created attachment 280109 [details] [review]
searchbar: Notify on changed search

Add a signal to the Searchbar class to notify on changed search.
Allow to untie the consequences from App.app and App.window.
Comment 62 Adrien Plazas 2014-07-08 06:47:54 UTC
Created attachment 280110 [details] [review]
searchbar: Notify on activated search

Add a signal to	the Searchbar class to notify on activated search.
Allow to untie the consequences	from App.window.
Comment 63 Adrien Plazas 2014-07-08 07:09:17 UTC
Created attachment 280112 [details] [review]
searchbar: Add window property

Add a window property to the Searchbar class to untie it from App.window.
Comment 64 Adrien Plazas 2014-07-08 07:09:23 UTC
Created attachment 280113 [details] [review]
searchbar: Notify on changed search

Add a signal to the Searchbar class to notify on changed search.
Allow to untie the consequences from App.app and App.window.
Comment 65 Adrien Plazas 2014-07-08 07:09:28 UTC
Created attachment 280114 [details] [review]
searchbar: Notify on activated search

Add a signal to the Searchbar class to notify on activated search.
Allow to untie the consequences	from App.window.
Comment 66 Adrien Plazas 2014-07-08 07:09:33 UTC
Created attachment 280115 [details] [review]
properties-toolbar: Notify on back clicked

Add a signal to the Topbar class to notify on back clicked
on the properties toolbar.
Allow to untie the consequences from App.app.
Comment 67 Adrien Plazas 2014-07-08 07:23:25 UTC
Created attachment 280116 [details] [review]
searchbar: Add window property

Add a window property to the Searchbar class to untie it from App.window.
Comment 68 Adrien Plazas 2014-07-08 07:23:30 UTC
Created attachment 280117 [details] [review]
searchbar: Notify on changed search

Notify the text property of the Searchbar class when on changed search.
Allow to untie the consequences from App.app and App.window.
Comment 69 Adrien Plazas 2014-07-08 07:23:36 UTC
Created attachment 280118 [details] [review]
searchbar: Notify on activated search

Add a signal to the Searchbar class to notify on activated search.
Allow to untie the consequences	from App.window.
Comment 70 Adrien Plazas 2014-07-08 07:23:41 UTC
Created attachment 280119 [details] [review]
properties-toolbar: Notify on back clicked

Add a signal to the Topbar class to notify on back clicked
on the properties toolbar.
Allow to untie the consequences from App.app.
Comment 71 Lasse Schuirmann 2014-07-08 08:25:30 UTC
Review of attachment 280031 [details] [review]:

Looks good. Zeenix will probably have something against every commit message, but I can't think of better ones so I'll leave it at least for some patches.
Comment 72 Lasse Schuirmann 2014-07-08 08:57:48 UTC
Review of attachment 280032 [details] [review]:

Looks good.
Comment 73 Lasse Schuirmann 2014-07-08 09:47:47 UTC
Review of attachment 280033 [details] [review]:

Patch size is ok for me. This is a good change.

::: src/wizard-toolbar.vala
@@ +32,3 @@
+    construct {
+        update_next_button ();
+        notify["creation-ready"].connect (update_next_button);

Why don't you do that in the property setter? (I searched almost half an hour for this statement :D)

public bool creation_ready { get; set {
    page = WizardPage.REVIEW;
    next_button.sensitive = true;
    next_button.grab_focus ();
} }

or so? Maybe update/remove the update_next_button function appropriately.

@@ +68,3 @@
+            next_button.sensitive = false;
+            break;
+        }

how about:
next_button = (page == WizardPage.SETUP)
            ||(page == WizardPage.INTRODUCTION)
            ||(page == WizardPage.REVIEW && creation_ready)

its way shorter. (Copy it into an appropriate editor for proper alignment ;))

::: src/wizard.vala
@@ +84,1 @@
                         toolbar.create_btn.grab_focus ();

toolbar should do this when creation_ready is set to true. (and now we need our nice new abstraction :))
Comment 74 Lasse Schuirmann 2014-07-08 09:48:21 UTC
Review of attachment 280038 [details] [review]:

looks good for me
Comment 75 Lasse Schuirmann 2014-07-08 09:50:56 UTC
Review of attachment 280056 [details] [review]:

::: src/topbar.vala
@@ +89,3 @@
     }
 
+    public signal void cancel_clicked ();

I think the signal should just be named "cancel ()".
Comment 76 Lasse Schuirmann 2014-07-08 09:53:01 UTC
Review of attachment 280057 [details] [review]:

ack from me
Comment 77 Lasse Schuirmann 2014-07-08 10:21:18 UTC
Review of attachment 280058 [details] [review]:

Looks good.

::: src/topbar.vala
@@ +73,3 @@
         set {
             // Translators: The %s will be replaced with the name of the VM
+            props_toolbar.title = _("%s - Properties").printf (value);

Yes, that makes way more sense than before :)
Comment 78 Lasse Schuirmann 2014-07-08 10:23:44 UTC
Review of attachment 280059 [details] [review]:

::: src/topbar.vala
@@ +116,3 @@
+        page = selection_mode ? TopbarPage.SELECTION :
+                                TopbarPage.COLLECTION;
+    }

Why don't you set the page property directly?
Comment 79 Lasse Schuirmann 2014-07-08 10:26:50 UTC
Review of attachment 280060 [details] [review]:

::: src/topbar.vala
@@ +115,3 @@
+        toolbar.bind_property ("title", display_toolbar, "title", BindingFlags.SYNC_CREATE);
+        toolbar.bind_property ("subtitle", display_toolbar, "subtitle", BindingFlags.SYNC_CREATE);
+    }

maybe a DisplayToolbar prop with a setter would be clearer. What do you think?
Comment 80 Lasse Schuirmann 2014-07-08 10:41:04 UTC
Review of attachment 280116 [details] [review]:

::: src/app-window.vala
@@ +110,3 @@
+    private void setup_searchbar_ui () {
+        searchbar.window = this;
+    }

I like that you put this into an own function. Maybe zeenix doesn't because its a one-time-used one-liner. Lets leave it this way for now :)

::: src/searchbar.vala
@@ +34,3 @@
         search_mode_enabled = false;
 
+        notify["enable_key_handler"].connect (update_key_handler);

...I think I already said that...
https://bugzilla.gnome.org/review?bug=732098&attachment=279106

@@ +77,3 @@
+            blocked = true;
+        }
+    }

I think you don't need that function anymore after applying the comment above. Instead of calling this you can then invoke:
enable_key_handler = enable_key_handler
plus some comment that thats for updating purposes.
Comment 81 Lasse Schuirmann 2014-07-08 10:48:57 UTC
Review of attachment 280118 [details] [review]:

You have a \t in the log. Looks good apart from that.
Comment 82 Lasse Schuirmann 2014-07-08 10:54:25 UTC
Review of attachment 280119 [details] [review]:

Looks good. You know that 74 characters per line are allowed for commit messages? Also in log: s/Allow/Allows/
Comment 83 Lasse Schuirmann 2014-07-08 10:58:38 UTC
Review of attachment 280117 [details] [review]:

looks good
Comment 84 Adrien Plazas 2014-07-08 12:07:50 UTC
Review of attachment 280033 [details] [review]:

::: src/wizard-toolbar.vala
@@ +32,3 @@
+    construct {
+        update_next_button ();
+        notify["creation-ready"].connect (update_next_button);

The way I saw it, the fact that the creation is ready doesn't imply that you want the review page, just that the creation button is clickable.
So the next button needs to be updated without ever setting the page: that's why the update_next_button method exists, but maybe another design would be better.

@@ +68,3 @@
+            next_button.sensitive = false;
+            break;
+        }

I don't think it is easier to read, eventually something like this:

next_button.sensitive = (creation_ready && page == WizardPage.REVIEW) ||
                        page in new WizardPage[] {
                            WizardPage.INTRODUCTION,
                            WizardPage.SETUP
                        };

But I'm not very fond of it either.

::: src/wizard.vala
@@ +84,1 @@
                         toolbar.create_btn.grab_focus ();

Not necessarily as creation_ready doesn't imply the current page.
Except maybe when both the page is REVIEW and the creation is ready. What do you think about it?
Comment 85 Lasse Schuirmann 2014-07-08 12:23:58 UTC
Review of attachment 280033 [details] [review]:

::: src/wizard-toolbar.vala
@@ +32,3 @@
+    construct {
+        update_next_button ();
+        notify["creation-ready"].connect (update_next_button);

> The way I saw it, the fact that the creation is ready doesn't imply that you want the review page, just that the creation 
button is clickable.

If you are ready for creation you have to be on the review page, don't you? Being ready for creation from the outside does not care about the button it's just an abstract state. However my proposed setter is of course a mess since it assumes (value == true) it is merely a scetch how I'd do it.

> So the next button needs to be updated without ever setting the page: that's why the update_next_button method exists, but maybe another design would be better.

I'd really like it better if we have a setter instead of binding a property making the program more decentral and complex. If you have a better idea feel free to propose it. :)

@@ +68,3 @@
+            next_button.sensitive = false;
+            break;
+        }

I dislike creating a new WizardPage[] object, that makes the whole thing more complex (and not easier) for me. I still like my short version but maybe its a matter of preference. We can keep it now and mention the alternative when zeenix reviews it later.

::: src/wizard.vala
@@ +84,1 @@
                         toolbar.create_btn.grab_focus ();

What sense does creation_ready make when we're on another page than REVIEW?
Comment 86 Lasse Schuirmann 2014-07-08 13:48:06 UTC
Review of attachment 280033 [details] [review]:

::: src/wizard-toolbar.vala
@@ +32,3 @@
+    construct {
+        update_next_button ();
+        notify["creation-ready"].connect (update_next_button);

how about:

public bool creation_ready { get; set
requires (!value || page == WizardPage.REVIEW)
{
next_button.sensitive = true;
next_button.grab_focus ();
} }

And you should also set it to false when changing to a page which is not REVIEW.
Comment 87 Adrien Plazas 2014-07-08 14:22:14 UTC
Review of attachment 280033 [details] [review]:

After a chat on IRC, we settled that the creation_ready could either:
- simply set the creation button's sensitivity
- also set the page to REVIEW as if the creation is ready we should review it

We decided that changing the page wasn't a property's job but that the implication could be implemented as a method similar to this:
// The create button is insensitive when the page is set.
public void review_finished () {
    assert (page == WizardPage.REVIEW);
    create_btn.sensitive = true;
    create_btn.grab_focus ();
}
Comment 88 Adrien Plazas 2014-07-08 14:39:31 UTC
Created attachment 280165 [details] [review]
wizard-toolbar: Set buttons depending on its page

The wizard toolbar now set its buttons depending on its page for a better
separation from the Wizard class.
Comment 89 Adrien Plazas 2014-07-08 14:45:56 UTC
Created attachment 280166 [details] [review]
wizard-toolbar: Set buttons depending on its page

The wizard toolbar now set its buttons depending on its page for a better
separation from the Wizard class.
Comment 90 Lasse Schuirmann 2014-07-08 14:56:50 UTC
Review of attachment 280033 [details] [review]:

Thanks for writing this down. A small thing: you don't want to do an assert but rather make a precondition out of it:
public void review_finished () requires (page == WizardPage.REVIEW) {
//...
}
Comment 91 Adrien Plazas 2014-07-09 06:42:42 UTC
Review of attachment 280060 [details] [review]:

::: src/topbar.vala
@@ +115,3 @@
+        toolbar.bind_property ("title", display_toolbar, "title", BindingFlags.SYNC_CREATE);
+        toolbar.bind_property ("subtitle", display_toolbar, "subtitle", BindingFlags.SYNC_CREATE);
+    }

My version is bad but I'm not sure yours is good enough either. Lets find an even better one.
Comment 92 Adrien Plazas 2014-07-09 11:08:18 UTC
Created attachment 280228 [details] [review]
wizard: Use WizardToolbar

Use the buttons from a configurable WizardToolbar instead of the ones from
App.window.topbar.wizard_toolbar to allow multiple windows.
Comment 93 Adrien Plazas 2014-07-09 11:08:25 UTC
Created attachment 280229 [details] [review]
wizard: Notify failed installation

Add a signal to the Wizard class to notify on failed installations.
Allows to display the notification in another way than with
App.window.notificationbar.
Comment 94 Adrien Plazas 2014-07-09 11:08:31 UTC
Created attachment 280230 [details] [review]
wizard: Notify successful installation

Add a signal to the Wizard class to notify on successful installations
to untie it from its consequences.
Comment 95 Adrien Plazas 2014-07-09 11:08:37 UTC
Created attachment 280231 [details] [review]
app-window: Set sidebar's wizard page

Set the sidebar's wizard page from the AppWindow class for more modularity.
Comment 96 Adrien Plazas 2014-07-09 11:08:43 UTC
Created attachment 280232 [details] [review]
wizard: Bind page to toolbar's page

Add a WizardPage to the wizard toolbar and bind it to a wizard's page so it
can check other pages than App.window.wizard.page.
Comment 97 Adrien Plazas 2014-07-09 11:08:49 UTC
Created attachment 280233 [details] [review]
vm-creator: Notify failed installation

Add a signal to the VMCreator class to notify on failed installations.
Allows to display the notification in another way than with
App.window.notificationbar.
Comment 98 Adrien Plazas 2014-07-09 11:08:55 UTC
Created attachment 280234 [details] [review]
wizard-toolbar: Set buttons depending on its page

The wizard toolbar now set its buttons depending on its page for a better
separation from the Wizard class.
Comment 99 Adrien Plazas 2014-07-09 11:09:01 UTC
Created attachment 280235 [details] [review]
topbar: Notify clicked cancel

Add a signal to the Topbar class to notify clicks on a cancel button.
Allows to let the window handle the selection mode and the UI state
for more modularity.
Comment 100 Adrien Plazas 2014-07-09 11:09:07 UTC
Created attachment 280236 [details] [review]
topbar: Check its own ui state

The Topbar class now checks its own UI state to untie it
from the App's one.
Comment 101 Adrien Plazas 2014-07-09 11:09:13 UTC
Created attachment 280237 [details] [review]
topbar: Set title from machine name

Set the title of the display and properties toolbars form the machine's name.
Help to untie the Topbar class from App.app.current_item and
App.window.display_page.toolbar.
Comment 102 Adrien Plazas 2014-07-09 11:09:19 UTC
Created attachment 280238 [details] [review]
app-window: Set topbar's selection mode

Set the page of the topbar according to the selection mode's value
to untie it from App.app.selection_mode.
Comment 103 Adrien Plazas 2014-07-09 11:09:25 UTC
Created attachment 280239 [details] [review]
topbar: Set display subtitle

Bind the display toolbar's title to the topbar's one
to untie it from App.window.display_page.toolbar.
Comment 104 Adrien Plazas 2014-07-09 11:09:31 UTC
Created attachment 280240 [details] [review]
searchbar: Add window property

Add a window property to the Searchbar class to untie it from App.window.
Comment 105 Adrien Plazas 2014-07-09 11:09:37 UTC
Created attachment 280241 [details] [review]
searchbar: Notify on changed search

Notify the text property of the Searchbar class when on changed search.
Allow to untie the consequences from App.app and App.window.
Comment 106 Adrien Plazas 2014-07-09 11:09:44 UTC
Created attachment 280242 [details] [review]
searchbar: Notify on activated search

Add a signal to the Searchbar class to notify on activated search.
Allow to untie the consequences from App.window.
Comment 107 Adrien Plazas 2014-07-09 11:09:50 UTC
Created attachment 280243 [details] [review]
properties-toolbar: Notify on back clicked

Add a signal to the Topbar class to notify on back clicked on the
properties toolbar.
Allows to untie the consequences from App.app.
Comment 108 Lasse Schuirmann 2014-07-09 11:50:05 UTC
Review of attachment 280228 [details] [review]:

Log: if I read it I'm don't really know what the patch does. I think one could mention in the shortlog that you introduce this member in the wizard somehow...  I don't know... logs are the most difficult thing here...

::: src/app.vala
@@ +250,1 @@
                 else

Look through the whole wizard.vala. You'll see that whereever there is an else and a } before it they are in the same line. I'd say stick to what's existent concerning coding style.

This may also apply to other patches.

::: src/wizard.vala
@@ +198,3 @@
+                    page = page + 1;
+                });
+                tb_create_id = _toolbar.create_btn.clicked.connect (() => {

what about placing an assertion here that page == WizardPage.REVIEW?

@@ +206,3 @@
+            return _toolbar;
+        }
+    }

* I dislike long setter methods. They tend to grow. Thats what happened with the page setter above I assume. If you have any idea how to shorten this I'd appreciate that.

* How about: if (value == _toolbar) return; in the first line in the setter?

@@ -558,2 @@
     public void open_with_uri (string uri, bool skip_review_for_live = true) {
-        App.app.set_state (UIState.WIZARD);

I think moving this line to app.vala is an own patch. I don't see how that has anything to do with using the wizard toolbar for some things.
Comment 109 Lasse Schuirmann 2014-07-09 11:53:06 UTC
Review of attachment 280229 [details] [review]:

Assuming patch applies cleanly and compiles. I'll recheck that for all patches in the next iteration. ACK
Comment 110 Lasse Schuirmann 2014-07-09 11:55:26 UTC
Review of attachment 280230 [details] [review]:

same here.
Comment 111 Lasse Schuirmann 2014-07-09 11:58:54 UTC
Review of attachment 280231 [details] [review]:

I think log could be a bit more verbose.
Comment 112 Lasse Schuirmann 2014-07-09 12:01:01 UTC
Review of attachment 280232 [details] [review]:

ack
Comment 113 Lasse Schuirmann 2014-07-09 12:10:35 UTC
Review of attachment 280233 [details] [review]:

ack
Comment 114 Lasse Schuirmann 2014-07-09 12:14:36 UTC
Review of attachment 280235 [details] [review]:

ack

::: src/topbar.vala
@@ +101,3 @@
+        wizard_toolbar.cancel_btn.clicked.connect (() => {
+            canceled ();
+        });

To be fully modular you'd do a signal in selection_toolbar and wizard_toolbar and route both signals to canceled () here. But I don't think that this is necessary here.
Comment 115 Lasse Schuirmann 2014-07-09 12:25:16 UTC
Review of attachment 280241 [details] [review]:

::: src/searchbar.vala
@@ +52,3 @@
+        set {
+            entry.set_text (value);
+            notify_property ("text");

IIUC you don't need to notify the text explicitly, it should happen automatically when you invoke the setter.
Comment 116 Lasse Schuirmann 2014-07-09 12:25:52 UTC
Review of attachment 280242 [details] [review]:

ack
Comment 117 Lasse Schuirmann 2014-07-09 12:26:50 UTC
Review of attachment 280243 [details] [review]:

ack
Comment 118 Lasse Schuirmann 2014-07-09 12:43:04 UTC
Review of attachment 280234 [details] [review]:

I dislike the shortlog.
How about:
wizard-toolbar: introduce page prop

...

::: src/wizard-toolbar.vala
@@ +33,3 @@
+    }
+
+    public void review_finished () ensures (page == WizardPage.REVIEW) {

you'd want to use requires instead of ensures here

@@ +36,3 @@
+        next_button.sensitive = true;
+        next_button.grab_focus ();
+    }

If I do now update_next_button () the next button will not be sensitive anymore even though I would expect that the next button gets updated when necessary and if I update it again it shouldnt change. YOu know what I mean?

Maybe update_next_button shouldn't take care of the sensitivity of the next_button?

@@ +54,3 @@
+            next_button = continue_btn;
+        }
+        else {

same here with the } else

::: src/wizard.vala
@@ +49,3 @@
         get { return _page; }
         private set {
+            if (toolbar != null) toolbar.page = value;

Did you do some tests that this works? Previously there were some conditional returns and this weird skip_page invokation which make it an incredible complex task to determine which the next page is.
Comment 119 Lasse Schuirmann 2014-07-09 12:45:26 UTC
Review of attachment 280236 [details] [review]:

nice small patch
Comment 120 Lasse Schuirmann 2014-07-09 12:55:56 UTC
Review of attachment 280238 [details] [review]:

ok
Comment 121 Lasse Schuirmann 2014-07-09 12:57:11 UTC
Review of attachment 280239 [details] [review]:

Can you switch this commit with its parent? So we have title and subtitle patch together.
Comment 122 Lasse Schuirmann 2014-07-09 12:58:54 UTC
Review of attachment 280237 [details] [review]:

ack
Comment 123 Lasse Schuirmann 2014-07-09 13:04:52 UTC
Review of attachment 280240 [details] [review]:

In the log you only talk about the window. I think you should explain not only to me but to the whole world what you did to the key handler ;)

::: src/searchbar.vala
@@ +61,3 @@
     }
+
+    public void set_key_handler (bool enabled) {

I think I really would put this into the setter. It does do the same and just that you're modifying internally is no reason to put it in an extra method.

@@ +68,3 @@
+            key_handler_blocked = false;
+        }
+        else if (!enabled && ! key_handler_blocked) {

theres the bracket problem again
Comment 124 Adrien Plazas 2014-07-09 14:44:26 UTC
Review of attachment 280234 [details] [review]:

::: src/wizard-toolbar.vala
@@ +36,3 @@
+        next_button.sensitive = true;
+        next_button.grab_focus ();
+    }

I know what you mean but I don't understand how it is problematic as the only moment update_next_button is called is on construction and when you change the page.

::: src/wizard.vala
@@ +49,3 @@
         get { return _page; }
         private set {
+            if (toolbar != null) toolbar.page = value;

I don't understand what you mean, I ran the code and tested different cases, everything worked great.
What do you think can be a problem here?
Comment 125 Lasse Schuirmann 2014-07-09 14:52:18 UTC
Review of attachment 280234 [details] [review]:

::: src/wizard-toolbar.vala
@@ +36,3 @@
+        next_button.sensitive = true;
+        next_button.grab_focus ();
+    }

Lets say I write:
  mylittlebar.review_finished ();
  mylittlebar.page = mylittlebar.page;

I'd expect this code snippet to have an equal result to:
  mylittlebar.review_finished ();

Which isnt true. In the former case the page setter will invoke update_next_button which will render the create button insensitive.

If you access the sensitivity from several distinct places in the program that makes it more complex to the user. So while this is not a major problem it is generally better to reduce usages of variables to a central point. This increases readability and maintainability.

::: src/wizard.vala
@@ +49,3 @@
         get { return _page; }
         private set {
+            if (toolbar != null) toolbar.page = value;

I'm just saying here that the page setter is a pretty complex thing. And I believe you if you tested that it works (I also tested it) but I'm not sure if there are corner cases where this doesnt work. (E.g. some things related to preparation. I told you about the bug I had with this because of the async recursive calls of this page setter.)
Comment 126 Adrien Plazas 2014-07-09 15:06:57 UTC
Review of attachment 280234 [details] [review]:

::: src/wizard-toolbar.vala
@@ +36,3 @@
+        next_button.sensitive = true;
+        next_button.grab_focus ();
+    }

If you do:
mylittlebar.review_finished ();
mylittlebar.page = myotherbar.page;
Would you expect it do do something in most cases but nothing if both pages are equal?

Also how would you reset the review state while staying on the same page?

I don't think it's a problem: if you set the page then it haves to be in its default state, and it's up to the user of the class to check if the value are equal before setting them.

::: src/wizard.vala
@@ +49,3 @@
         get { return _page; }
         private set {
+            if (toolbar != null) toolbar.page = value;

I'm not totally sure about it either but I don't know how to test these corner cases like failed preparation or installation.
Comment 127 Lasse Schuirmann 2014-07-09 15:10:56 UTC
Review of attachment 280234 [details] [review]:

::: src/wizard-toolbar.vala
@@ +36,3 @@
+        next_button.sensitive = true;
+        next_button.grab_focus ();
+    }

ok, leave it that way then.

::: src/wizard.vala
@@ +49,3 @@
         get { return _page; }
         private set {
+            if (toolbar != null) toolbar.page = value;

But we should _really_ be sure before we're applying this on master shouldnt we? You can try out failed preparation if you just insert return false; into the prepare () function in the wizard.
Comment 128 Adrien Plazas 2014-07-09 15:14:01 UTC
Review of attachment 280240 [details] [review]:

::: src/searchbar.vala
@@ +61,3 @@
     }
+
+    public void set_key_handler (bool enabled) {

To me properties represent states or values, and methods are used for actions.

The problem is that the original code is an action implemented as a property.

So either we store the state to transmit it to a new window if changed, or we put it in a method and it doesn't apply to a new window.
To keep the existing behavior I choosed the second one, which one would be best to you?
Comment 129 Lasse Schuirmann 2014-07-09 15:16:24 UTC
Review of attachment 280240 [details] [review]:

::: src/searchbar.vala
@@ +61,3 @@
     }
+
+    public void set_key_handler (bool enabled) {

I'd keep the existing behaviour while putting it into a property. key_handler_enabled is a state and the setter does some implications of the state.
Comment 130 Adrien Plazas 2014-07-10 07:23:19 UTC
Created attachment 280322 [details] [review]
wizard: Add toolbar prop

Use a WizardToolbar property instead of separated buttons.
This is so that it doesn't have to access it through the App.window
singleton, which will be removed in a following patch.
Comment 131 Adrien Plazas 2014-07-10 07:23:26 UTC
Created attachment 280323 [details] [review]
app: Set state to wizard when opening URI

This is so that wizard doesn't have to set it as it will be removed in a
following patch.
Comment 132 Adrien Plazas 2014-07-10 07:23:33 UTC
Created attachment 280324 [details] [review]
wizard: Notify failed installation

Add a signal to the Wizard class to notify on failed installations.
This is so that it doesn't have to access the notificationbar through
the App.window singleton, which will be removed in a following patch.
Comment 133 Adrien Plazas 2014-07-10 07:23:41 UTC
Created attachment 280325 [details] [review]
wizard: Notify successful installation

Add a signal to the Wizard class to notify on successful installations.
This is so that it doesn't have to set the app's UI state, which will be
removed in a following patch.
Comment 134 Adrien Plazas 2014-07-10 07:23:47 UTC
Created attachment 280326 [details] [review]
app-window: Set sidebar's wizard page

Set the sidebar's wizard page from the AppWindow class for more modularity.
This is so that the wizard doesn't have to access it through the
App.window singleton, which will be removed in a following patch.
Comment 135 Adrien Plazas 2014-07-10 07:23:54 UTC
Created attachment 280327 [details] [review]
wizard-toolbar: Add page prop

Add a WizardPage to the wizard toolbar and bind it to a wizard's page.
This is so that it doesn't have to access it through the App.window
singleton, which will be removed in a following patch.
Comment 136 Adrien Plazas 2014-07-10 07:24:01 UTC
Created attachment 280328 [details] [review]
wizard-toolbar: page prop set its state

The wizard toolbar now set its buttons depending on its page for a better
separation from the Wizard class.
Comment 137 Adrien Plazas 2014-07-10 07:24:09 UTC
Created attachment 280329 [details] [review]
vm-creator: Notify failed installation

Add a signal to the VMCreator class to notify on failed installations.
This is so that it doesn't have to access the notificationbar through
the App.window singleton, which will be removed in a following patch.
Comment 138 Adrien Plazas 2014-07-10 07:24:16 UTC
Created attachment 280330 [details] [review]
topbar: Notify clicked cancel

Add a signal to the Topbar class to notify clicks on a cancel button.
This is so that it doesn't have to set the app's selection mode, which
will be removed in a following patch.
Comment 139 Adrien Plazas 2014-07-10 07:24:23 UTC
Created attachment 280331 [details] [review]
topbar: Check its own ui state

The Topbar class now checks its own UI state.
This is so that it doesn't have to set the app's UI state, which will be
removed in a following patch.
Comment 140 Adrien Plazas 2014-07-10 07:24:31 UTC
Created attachment 280332 [details] [review]
topbar: Set title from machine name

Set the title of the display and properties toolbars form the machine's
name.
This is so that it doesn't have to set the app's current item, which will
be removed in a following patch.
Comment 141 Adrien Plazas 2014-07-10 07:24:38 UTC
Created attachment 280333 [details] [review]
topbar: Set display subtitle

Bind the display toolbar's title to the topbar's one.
This is so that it doesn't have to access it through the App.window
singleton, which will be removed in a following patch.

https://bugzilla.gnome.org/show_bug.cgi?id=732098

Conflicts:
	src/topbar.vala
Comment 142 Adrien Plazas 2014-07-10 07:24:45 UTC
Created attachment 280334 [details] [review]
app-window: Set topbar's selection mode

Set the page of the topbar according to the selection mode's value.
This is so that it doesn't have to set the app's selection mode, which will be
removed in a following patch.

https://bugzilla.gnome.org/show_bug.cgi?id=732098

Conflicts:
	src/topbar.vala
Comment 143 Adrien Plazas 2014-07-10 07:24:52 UTC
Created attachment 280335 [details] [review]
searchbar: Add window property

Add a window property to the Searchbar class.
This is so that it doesn't have assume its window is the App.window
singleton, which will be removed in a following patch.
Comment 144 Adrien Plazas 2014-07-10 07:24:59 UTC
Created attachment 280336 [details] [review]
searchbar: Transmit key handler blocked to windows

Store the key handler blocked state to transmit it to a new window.
It makes the behavior of the key_handler_enabled property consistent now
that the window can change.
Comment 145 Adrien Plazas 2014-07-10 07:25:06 UTC
Created attachment 280337 [details] [review]
searchbar: Notify on changed search

Notify the text property of the Searchbar class when on changed search.
This is so that it doesn't have to access the app's filter and App.window
singleton, which will be removed in a following patch.
Comment 146 Adrien Plazas 2014-07-10 07:25:14 UTC
Created attachment 280338 [details] [review]
searchbar: Notify on activated search

Add a signal to the Searchbar class to notify on activated search.
This is so that it doesn't have to access the view through the App.window
singleton, which will be removed in a following patch.
Comment 147 Adrien Plazas 2014-07-10 07:25:22 UTC
Created attachment 280339 [details] [review]
properties-toolbar: Notify on back clicked

Add a signal to the Topbar class to notify on back clicked on the
properties toolbar.
This is so that it doesn't have to set the app's UI state, which will be
removed in a following patch.
Comment 148 Lasse Schuirmann 2014-07-10 08:09:19 UTC
Review of attachment 280322 [details] [review]:

::: src/app.vala
@@ +246,2 @@
                     window.wizard.open_with_uri (arg);
+                } else

The only change you do here is adding unneeded brackets, am I wrong?
Comment 149 Lasse Schuirmann 2014-07-10 08:31:50 UTC
Review of attachment 280323 [details] [review]:

ack from me
Comment 150 Adrien Plazas 2014-07-10 08:52:49 UTC
Created attachment 280341 [details] [review]
wizard: Add toolbar prop

Use a WizardToolbar property instead of separated buttons.
This is so that it doesn't have to access it through the App.window
singleton, which will be removed in a following patch.
Comment 151 Adrien Plazas 2014-07-10 08:52:57 UTC
Created attachment 280342 [details] [review]
wizard: Notify failed installation

Add a signal to the Wizard class to notify on failed installations.
This is so that it doesn't have to access the notificationbar through
the App.window singleton, which will be removed in a following patch.
Comment 152 Adrien Plazas 2014-07-10 08:53:05 UTC
Created attachment 280343 [details] [review]
wizard: Notify successful installation

Add a signal to the Wizard class to notify on successful installations.
This is so that it doesn't have to set the app's UI state, which will be
removed in a following patch.
Comment 153 Adrien Plazas 2014-07-10 08:53:13 UTC
Created attachment 280344 [details] [review]
app-window: Set sidebar's wizard page

Set the sidebar's wizard page from the AppWindow class for more modularity.
This is so that the wizard doesn't have to access it through the
App.window singleton, which will be removed in a following patch.
Comment 154 Adrien Plazas 2014-07-10 08:53:20 UTC
Created attachment 280345 [details] [review]
wizard-toolbar: Add page prop

Add a WizardPage to the wizard toolbar and bind it to a wizard's page.
This is so that it doesn't have to access it through the App.window
singleton, which will be removed in a following patch.
Comment 155 Adrien Plazas 2014-07-10 08:53:28 UTC
Created attachment 280346 [details] [review]
wizard-toolbar: page prop set its state

The wizard toolbar now set its buttons depending on its page for a better
separation from the Wizard class.
Comment 156 Adrien Plazas 2014-07-10 08:53:35 UTC
Created attachment 280347 [details] [review]
vm-creator: Notify failed installation

Add a signal to the VMCreator class to notify on failed installations.
This is so that it doesn't have to access the notificationbar through
the App.window singleton, which will be removed in a following patch.
Comment 157 Adrien Plazas 2014-07-10 08:53:43 UTC
Created attachment 280348 [details] [review]
topbar: Notify clicked cancel

Add a signal to the Topbar class to notify clicks on a cancel button.
This is so that it doesn't have to set the app's selection mode, which
will be removed in a following patch.
Comment 158 Adrien Plazas 2014-07-10 08:53:50 UTC
Created attachment 280349 [details] [review]
topbar: Check its own ui state

The Topbar class now checks its own UI state.
This is so that it doesn't have to set the app's UI state, which will be
removed in a following patch.
Comment 159 Adrien Plazas 2014-07-10 08:53:58 UTC
Created attachment 280350 [details] [review]
topbar: Set title from machine name

Set the title of the display and properties toolbars form the machine's
name.
This is so that it doesn't have to set the app's current item, which will
be removed in a following patch.
Comment 160 Adrien Plazas 2014-07-10 08:54:06 UTC
Created attachment 280351 [details] [review]
topbar: Set display subtitle

Bind the display toolbar's title to the topbar's one.
This is so that it doesn't have to access it through the App.window
singleton, which will be removed in a following patch.

https://bugzilla.gnome.org/show_bug.cgi?id=732098

Conflicts:
	src/topbar.vala
Comment 161 Adrien Plazas 2014-07-10 08:54:13 UTC
Created attachment 280352 [details] [review]
app-window: Set topbar's selection mode

Set the page of the topbar according to the selection mode's value.
This is so that it doesn't have to set the app's selection mode, which will be
removed in a following patch.

https://bugzilla.gnome.org/show_bug.cgi?id=732098

Conflicts:
	src/topbar.vala
Comment 162 Adrien Plazas 2014-07-10 08:54:21 UTC
Created attachment 280353 [details] [review]
searchbar: Add window property

Add a window property to the Searchbar class.
This is so that it doesn't have assume its window is the App.window
singleton, which will be removed in a following patch.
Comment 163 Adrien Plazas 2014-07-10 08:54:29 UTC
Created attachment 280354 [details] [review]
searchbar: Transmit key handler blocked to windows

Store the key handler blocked state to transmit it to a new window.
It makes the behavior of the key_handler_enabled property consistent now
that the window can change.
Comment 164 Adrien Plazas 2014-07-10 08:54:37 UTC
Created attachment 280355 [details] [review]
searchbar: Notify on changed search

Notify the text property of the Searchbar class when on changed search.
This is so that it doesn't have to access the app's filter and App.window
singleton, which will be removed in a following patch.
Comment 165 Adrien Plazas 2014-07-10 08:54:45 UTC
Created attachment 280356 [details] [review]
searchbar: Notify on activated search

Add a signal to the Searchbar class to notify on activated search.
This is so that it doesn't have to access the view through the App.window
singleton, which will be removed in a following patch.
Comment 166 Adrien Plazas 2014-07-10 08:54:53 UTC
Created attachment 280357 [details] [review]
properties-toolbar: Notify on back clicked

Add a signal to the Topbar class to notify on back clicked on the
properties toolbar.
This is so that it doesn't have to set the app's UI state, which will be
removed in a following patch.
Comment 167 Lasse Schuirmann 2014-07-10 10:01:29 UTC
Review of attachment 280341 [details] [review]:

::: src/app.vala
@@ +248,2 @@
                     window.wizard.open_with_uri (arg);
+                } else

didnt you want to split this change into an own patch? (you even did this in the last iteration!)
Comment 168 Adrien Plazas 2014-07-10 10:22:10 UTC
Created attachment 280359 [details] [review]
wizard: Add toolbar prop

Use a WizardToolbar property instead of separated buttons.
This is so that it doesn't have to access it through the App.window
singleton, which will be removed in a following patch.
Comment 169 Adrien Plazas 2014-07-10 10:22:18 UTC
Created attachment 280360 [details] [review]
app: Set state to wizard when opening URI

This is so that wizard doesn't have to set it as it will be removed in a
following patch.
Comment 170 Adrien Plazas 2014-07-10 10:22:26 UTC
Created attachment 280362 [details] [review]
wizard: Notify failed installation

Add a signal to the Wizard class to notify on failed installations.
This is so that it doesn't have to access the notificationbar through
the App.window singleton, which will be removed in a following patch.
Comment 171 Adrien Plazas 2014-07-10 10:22:34 UTC
Created attachment 280364 [details] [review]
wizard: Notify successful installation

Add a signal to the Wizard class to notify on successful installations.
This is so that it doesn't have to set the app's UI state, which will be
removed in a following patch.
Comment 172 Adrien Plazas 2014-07-10 10:22:42 UTC
Created attachment 280365 [details] [review]
app-window: Set sidebar's wizard page

Set the sidebar's wizard page from the AppWindow class for more modularity.
This is so that the wizard doesn't have to access it through the
App.window singleton, which will be removed in a following patch.
Comment 173 Adrien Plazas 2014-07-10 10:22:50 UTC
Created attachment 280366 [details] [review]
wizard-toolbar: Add page prop

Add a WizardPage to the wizard toolbar and bind it to a wizard's page.
This is so that it doesn't have to access it through the App.window
singleton, which will be removed in a following patch.
Comment 174 Adrien Plazas 2014-07-10 10:22:58 UTC
Created attachment 280367 [details] [review]
wizard-toolbar: page prop set its state

The wizard toolbar now set its buttons depending on its page for a better
separation from the Wizard class.
Comment 175 Adrien Plazas 2014-07-10 10:23:06 UTC
Created attachment 280368 [details] [review]
vm-creator: Notify failed installation

Add a signal to the VMCreator class to notify on failed installations.
This is so that it doesn't have to access the notificationbar through
the App.window singleton, which will be removed in a following patch.
Comment 176 Adrien Plazas 2014-07-10 10:23:15 UTC
Created attachment 280369 [details] [review]
topbar: Notify clicked cancel

Add a signal to the Topbar class to notify clicks on a cancel button.
This is so that it doesn't have to set the app's selection mode, which
will be removed in a following patch.
Comment 177 Adrien Plazas 2014-07-10 10:23:22 UTC
Created attachment 280370 [details] [review]
topbar: Check its own ui state

The Topbar class now checks its own UI state.
This is so that it doesn't have to set the app's UI state, which will be
removed in a following patch.
Comment 178 Adrien Plazas 2014-07-10 10:23:28 UTC
Created attachment 280371 [details] [review]
topbar: Set title from machine name

Set the title of the display and properties toolbars form the machine's
name.
This is so that it doesn't have to set the app's current item, which will
be removed in a following patch.
Comment 179 Adrien Plazas 2014-07-10 10:23:37 UTC
Created attachment 280372 [details] [review]
topbar: Set display subtitle

Bind the display toolbar's title to the topbar's one.
This is so that it doesn't have to access it through the App.window
singleton, which will be removed in a following patch.

https://bugzilla.gnome.org/show_bug.cgi?id=732098

Conflicts:
	src/topbar.vala
Comment 180 Adrien Plazas 2014-07-10 10:23:45 UTC
Created attachment 280374 [details] [review]
app-window: Set topbar's selection mode

Set the page of the topbar according to the selection mode's value.
This is so that it doesn't have to set the app's selection mode, which will be
removed in a following patch.

https://bugzilla.gnome.org/show_bug.cgi?id=732098

Conflicts:
	src/topbar.vala
Comment 181 Adrien Plazas 2014-07-10 10:23:54 UTC
Created attachment 280375 [details] [review]
searchbar: Add window property

Add a window property to the Searchbar class.
This is so that it doesn't have assume its window is the App.window
singleton, which will be removed in a following patch.
Comment 182 Adrien Plazas 2014-07-10 10:24:02 UTC
Created attachment 280376 [details] [review]
searchbar: Transmit key handler blocked to windows

Store the key handler blocked state to transmit it to a new window.
It makes the behavior of the key_handler_enabled property consistent now
that the window can change.
Comment 183 Adrien Plazas 2014-07-10 10:24:10 UTC
Created attachment 280377 [details] [review]
searchbar: Notify on changed search

Notify the text property of the Searchbar class when on changed search.
This is so that it doesn't have to access the app's filter and App.window
singleton, which will be removed in a following patch.
Comment 184 Adrien Plazas 2014-07-10 10:24:17 UTC
Created attachment 280379 [details] [review]
searchbar: Notify on activated search

Add a signal to the Searchbar class to notify on activated search.
This is so that it doesn't have to access the view through the App.window
singleton, which will be removed in a following patch.
Comment 185 Adrien Plazas 2014-07-10 10:24:25 UTC
Created attachment 280380 [details] [review]
properties-toolbar: Notify on back clicked

Add a signal to the Topbar class to notify on back clicked on the
properties toolbar.
This is so that it doesn't have to set the app's UI state, which will be
removed in a following patch.
Comment 186 Lasse Schuirmann 2014-07-10 10:50:03 UTC
Review of attachment 280359 [details] [review]:

ack
Comment 187 Lasse Schuirmann 2014-07-10 10:50:31 UTC
Review of attachment 280360 [details] [review]:

ack
Comment 188 Lasse Schuirmann 2014-07-10 10:51:00 UTC
Review of attachment 280362 [details] [review]:

ack
Comment 189 Lasse Schuirmann 2014-07-10 10:51:22 UTC
Review of attachment 280364 [details] [review]:

still ack
Comment 190 Lasse Schuirmann 2014-07-10 10:53:15 UTC
Review of attachment 280365 [details] [review]:

looks good. Log: you say the App.window singleton will be removed in the following patch. Thats not true, it still exists in Boxes. But dont reupload the whole patch series just for that. Wait for zeenix' reviews.
Comment 191 Lasse Schuirmann 2014-07-10 10:54:07 UTC
Review of attachment 280366 [details] [review]:

looks good
Comment 192 Lasse Schuirmann 2014-07-10 10:54:36 UTC
Review of attachment 280365 [details] [review]:

wrong status
Comment 193 Lasse Schuirmann 2014-07-10 11:00:09 UTC
Review of attachment 280367 [details] [review]:

page in shortlog should have a big P. Despite this I dont like shortlog yet... how about
wizard-toolbar: Set buttons in page setter
or so... my version is also not perfect maybe you can come up with a better alternative?

::: src/wizard-toolbar.vala
@@ +13,3 @@
     public Button create_btn;
 
+    private Button next_button { get; set; }

we don't need that { get; set; } here, am I wrong?

@@ +19,3 @@
+        get { return _page; }
+        set {
+            back_btn.sensitive = (value != WizardPage.INTRODUCTION);

doesnt really matter but I'd put this statement down between line 25 and 26 and use _page instead of value.

::: src/wizard.vala
@@ +49,3 @@
         get { return _page; }
         private set {
+            if (toolbar != null) toolbar.page = value;

so you did test the corner cases?
Comment 194 Adrien Plazas 2014-07-10 11:18:09 UTC
Review of attachment 280367 [details] [review]:

::: src/wizard-toolbar.vala
@@ +13,3 @@
     public Button create_btn;
 
+    private Button next_button { get; set; }

I don't think it is needed anymore yes.
Comment 195 Lasse Schuirmann 2014-07-10 12:19:08 UTC
Review of attachment 280368 [details] [review]:

ack
Comment 196 Lasse Schuirmann 2014-07-10 12:19:45 UTC
Review of attachment 280369 [details] [review]:

ack
Comment 197 Lasse Schuirmann 2014-07-10 12:20:11 UTC
Review of attachment 280370 [details] [review]:

ack
Comment 198 Lasse Schuirmann 2014-07-10 12:21:38 UTC
Review of attachment 280372 [details] [review]:

Remove the conflicts statement from the commit log.
Comment 199 Lasse Schuirmann 2014-07-10 12:22:06 UTC
Review of attachment 280371 [details] [review]:

ack
Comment 200 Lasse Schuirmann 2014-07-10 12:22:46 UTC
Review of attachment 280377 [details] [review]:

::: src/searchbar.vala
@@ +75,3 @@
+        set {
+            entry.set_text (value);
+        }

this can go in one line, can't it?
Comment 201 Lasse Schuirmann 2014-07-10 12:23:09 UTC
Review of attachment 280379 [details] [review]:

ack
Comment 202 Lasse Schuirmann 2014-07-10 12:24:46 UTC
Review of attachment 280380 [details] [review]:

Shortlog: its not the properties-toolbar that notifies but the topbar isnt it?

::: data/ui/properties-toolbar.ui
@@ +11,2 @@
     <child>
+      <object class="GtkButton" id="back_btn">

"ideally" renaming this button would go into an own patch.
Comment 203 Lasse Schuirmann 2014-07-10 12:26:07 UTC
Review of attachment 280375 [details] [review]:

ack
Comment 204 Lasse Schuirmann 2014-07-10 12:26:59 UTC
Review of attachment 280374 [details] [review]:

same here about log: conflicts were from your merge but there are no c onflicts anymore, right?
Comment 205 Lasse Schuirmann 2014-07-10 12:29:37 UTC
Review of attachment 280376 [details] [review]:

::: src/searchbar.vala
@@ +11,3 @@
+                blocked = false;
+            }
+            else if (!value && ! blocked) {

} else ...

@@ +17,3 @@
+        }
+        get {
+            return !blocked;

since blocked basically represents the state of key_handler_enabled I'd rename it to _key_handler_enabled.
Comment 206 Adrien Plazas 2014-07-10 14:17:31 UTC
Created attachment 280395 [details] [review]
wizard: Add toolbar prop

Use a WizardToolbar property instead of separated buttons.
This is so that it doesn't have to access it through the App.window
singleton, which will be removed in a following patch.
Comment 207 Adrien Plazas 2014-07-10 14:17:40 UTC
Created attachment 280396 [details] [review]
app: Set state to wizard when opening URI

This is so that wizard doesn't have to set it as it will be removed in a
following patch.
Comment 208 Adrien Plazas 2014-07-10 14:17:49 UTC
Created attachment 280397 [details] [review]
wizard: Notify failed installation

Add a signal to the Wizard class to notify on failed installations.
This is so that it doesn't have to access the notificationbar through
the App.window singleton, which will be removed in a following patch.
Comment 209 Adrien Plazas 2014-07-10 14:17:57 UTC
Created attachment 280398 [details] [review]
wizard: Notify successful installation

Add a signal to the Wizard class to notify on successful installations.
This is so that it doesn't have to set the app's UI state, which will be
removed in a following patch.
Comment 210 Adrien Plazas 2014-07-10 14:18:06 UTC
Created attachment 280399 [details] [review]
app-window: Set sidebar's wizard page

Set the sidebar's wizard page from the AppWindow class for more modularity.
This is so that the wizard doesn't have to access it through the
App.window singleton, which will be removed in a following patch.
Comment 211 Adrien Plazas 2014-07-10 14:18:13 UTC
Created attachment 280400 [details] [review]
wizard-toolbar: Add page prop

Add a WizardPage to the wizard toolbar and bind it to a wizard's page.
This is so that it doesn't have to access it through the App.window
singleton, which will be removed in a following patch.
Comment 212 Adrien Plazas 2014-07-10 14:18:20 UTC
Created attachment 280401 [details] [review]
wizard-toolbar: page prop set its state

The wizard toolbar now set its buttons depending on its page for a better
separation from the Wizard class.
Comment 213 Adrien Plazas 2014-07-10 14:18:29 UTC
Created attachment 280402 [details] [review]
vm-creator: Notify failed installation

Add a signal to the VMCreator class to notify on failed installations.
This is so that it doesn't have to access the notificationbar through
the App.window singleton, which will be removed in a following patch.
Comment 214 Adrien Plazas 2014-07-10 14:18:38 UTC
Created attachment 280403 [details] [review]
topbar: Notify clicked cancel

Add a signal to the Topbar class to notify clicks on a cancel button.
This is so that it doesn't have to set the app's selection mode, which
will be removed in a following patch.
Comment 215 Adrien Plazas 2014-07-10 14:18:48 UTC
Created attachment 280404 [details] [review]
topbar: Check its own ui state

The Topbar class now checks its own UI state.
This is so that it doesn't have to set the app's UI state, which will be
removed in a following patch.
Comment 216 Adrien Plazas 2014-07-10 14:18:56 UTC
Created attachment 280405 [details] [review]
topbar: Set title from machine name

Set the title of the display and properties toolbars form the machine's
name.
This is so that it doesn't have to set the app's current item, which will
be removed in a following patch.
Comment 217 Adrien Plazas 2014-07-10 14:19:05 UTC
Created attachment 280406 [details] [review]
topbar: Set display subtitle

Bind the display toolbar's title to the topbar's one.
This is so that it doesn't have to access it through the App.window
singleton, which will be removed in a following patch.
Comment 218 Adrien Plazas 2014-07-10 14:19:14 UTC
Created attachment 280407 [details] [review]
app-window: Set topbar's selection mode

Set the page of the topbar according to the selection mode's value.
This is so that it doesn't have to set the app's selection mode, which will be
removed in a following patch.
Comment 219 Adrien Plazas 2014-07-10 14:19:22 UTC
Created attachment 280408 [details] [review]
searchbar: Add window property

Add a window property to the Searchbar class.
This is so that it doesn't have assume its window is the App.window
singleton, which will be removed in a following patch.
Comment 220 Adrien Plazas 2014-07-10 14:19:32 UTC
Created attachment 280409 [details] [review]
searchbar: Transmit key handler blocked to windows

Store the key handler blocked state to transmit it to a new window.
It makes the behavior of the key_handler_enabled property consistent now
that the window can change.
Comment 221 Adrien Plazas 2014-07-10 14:19:39 UTC
Created attachment 280410 [details] [review]
searchbar: Notify on changed search

Notify the text property of the Searchbar class when on changed search.
This is so that it doesn't have to access the app's filter and App.window
singleton, which will be removed in a following patch.
Comment 222 Adrien Plazas 2014-07-10 14:19:48 UTC
Created attachment 280411 [details] [review]
searchbar: Notify on activated search

Add a signal to the Searchbar class to notify on activated search.
This is so that it doesn't have to access the view through the App.window
singleton, which will be removed in a following patch.
Comment 223 Adrien Plazas 2014-07-10 14:19:57 UTC
Created attachment 280412 [details] [review]
properties-toolbar: Notify on back clicked

Add a signal to the Topbar class to notify on back clicked on the
properties toolbar.
This is so that it doesn't have to set the app's UI state, which will be
removed in a following patch.
Comment 224 Lasse Schuirmann 2014-07-10 14:24:29 UTC
Review of attachment 280400 [details] [review]:

::: src/wizard-toolbar.vala
@@ +13,3 @@
     public Button create_btn;
 
+    public WizardPage page { set; get; }

switch set and get
Comment 225 Lasse Schuirmann 2014-07-10 14:25:51 UTC
Review of attachment 280405 [details] [review]:

::: src/app.vala
@@ +23,3 @@
     public UIState ui_state { get; protected set; }
 
+    public CollectionItem current_item { set; get; } // current object/vm manipulated

another get <-> set switch
Comment 226 Lasse Schuirmann 2014-07-10 14:25:53 UTC
Review of attachment 280405 [details] [review]:

::: src/app.vala
@@ +23,3 @@
     public UIState ui_state { get; protected set; }
 
+    public CollectionItem current_item { set; get; } // current object/vm manipulated

another get <-> set switch
Comment 227 Lasse Schuirmann 2014-07-10 14:28:17 UTC
Okay, zeenix, these patches are really good if you ask me. The two ones I marked with needs work are only very trivial things so I don't think it does any good if he reuploads the whole series just for that.

Would you mind taking care of the rest?
Comment 228 Adrien Plazas 2014-07-11 09:03:56 UTC
Created attachment 280480 [details] [review]
wizard-toolbar: Add page prop

Add a WizardPage to the wizard toolbar and bind it to a wizard's page.
This is so that it doesn't have to access it through the App.window
singleton, which will be removed in a following patch.
Comment 229 Adrien Plazas 2014-07-11 09:04:07 UTC
Created attachment 280481 [details] [review]
wizard-toolbar: page prop set its state

The wizard toolbar now set its buttons depending on its page for a better
separation from the Wizard class.
Comment 230 Adrien Plazas 2014-07-11 09:04:16 UTC
Created attachment 280482 [details] [review]
vm-creator: Notify failed installation

Add a signal to the VMCreator class to notify on failed installations.
This is so that it doesn't have to access the notificationbar through
the App.window singleton, which will be removed in a following patch.
Comment 231 Adrien Plazas 2014-07-11 09:04:26 UTC
Created attachment 280483 [details] [review]
topbar: Notify clicked cancel

Add a signal to the Topbar class to notify clicks on a cancel button.
This is so that it doesn't have to set the app's selection mode, which
will be removed in a following patch.
Comment 232 Adrien Plazas 2014-07-11 09:04:36 UTC
Created attachment 280484 [details] [review]
topbar: Check its own ui state

The Topbar class now checks its own UI state.
This is so that it doesn't have to set the app's UI state, which will be
removed in a following patch.
Comment 233 Adrien Plazas 2014-07-11 09:04:46 UTC
Created attachment 280485 [details] [review]
topbar: Set title from machine name

Set the title of the display and properties toolbars form the machine's
name.
This is so that it doesn't have to set the app's current item, which will
be removed in a following patch.
Comment 234 Adrien Plazas 2014-07-11 09:04:55 UTC
Created attachment 280486 [details] [review]
topbar: Set display subtitle

Bind the display toolbar's title to the topbar's one.
This is so that it doesn't have to access it through the App.window
singleton, which will be removed in a following patch.
Comment 235 Adrien Plazas 2014-07-11 09:05:05 UTC
Created attachment 280487 [details] [review]
app-window: Set topbar's selection mode

Set the page of the topbar according to the selection mode's value.
This is so that it doesn't have to set the app's selection mode, which will be
removed in a following patch.
Comment 236 Adrien Plazas 2014-07-11 09:05:15 UTC
Created attachment 280488 [details] [review]
searchbar: Add window property

Add a window property to the Searchbar class.
This is so that it doesn't have assume its window is the App.window
singleton, which will be removed in a following patch.
Comment 237 Adrien Plazas 2014-07-11 09:05:30 UTC
Created attachment 280489 [details] [review]
searchbar: Transmit key handler blocked to windows

Store the key handler blocked state to transmit it to a new window.
It makes the behavior of the key_handler_enabled property consistent now
that the window can change.
Comment 238 Adrien Plazas 2014-07-11 09:05:39 UTC
Created attachment 280490 [details] [review]
searchbar: Notify on changed search

Notify the text property of the Searchbar class when on changed search.
This is so that it doesn't have to access the app's filter and App.window
singleton, which will be removed in a following patch.
Comment 239 Adrien Plazas 2014-07-11 09:05:49 UTC
Created attachment 280491 [details] [review]
searchbar: Notify on activated search

Add a signal to the Searchbar class to notify on activated search.
This is so that it doesn't have to access the view through the App.window
singleton, which will be removed in a following patch.
Comment 240 Adrien Plazas 2014-07-11 09:05:59 UTC
Created attachment 280492 [details] [review]
properties-toolbar: Notify on back clicked

Add a signal to the Topbar class to notify on back clicked on the
properties toolbar.
This is so that it doesn't have to set the app's UI state, which will be
removed in a following patch.
Comment 241 Zeeshan Ali 2014-07-11 13:08:28 UTC
Review of attachment 280395 [details] [review]:

Looks good otherwise. Very happy with commit log (Lasse can tell you that its rare :))

::: src/app-window.vala
@@ +90,3 @@
     public void setup_ui () {
         topbar.setup_ui ();
+        setup_wizard_ui ();

Instead of moving code here, why not just pass the toolbar to Wizard.setup_ui() ?

::: src/wizard.vala
@@ +171,3 @@
     }
 
+    private ulong tb_cancel_id;

tb == toolbar? There is no need to shorten it here as its not that long a name. Avoid unknown abbreviations always and only shorten names if really needed.

@@ +191,3 @@
+            _toolbar = value;
+
+            if (_toolbar != null) {

I prefer to keep code less nested for readability:

if (_toolbar == null)
   return;

rest of the code..
Comment 242 Lasse Schuirmann 2014-07-11 13:12:44 UTC
Review of attachment 280395 [details] [review]:

::: src/app-window.vala
@@ +90,3 @@
     public void setup_ui () {
         topbar.setup_ui ();
+        setup_wizard_ui ();

Because he needs to connect it with something from the app singleton and thats what he wants to have out of the wizard:
See lines 98 to 100 below. The wizard shouldnt have anything to do with that.
Comment 243 Adrien Plazas 2014-07-11 13:19:00 UTC
Review of attachment 280395 [details] [review]:

::: src/app-window.vala
@@ +90,3 @@
     public void setup_ui () {
         topbar.setup_ui ();
+        setup_wizard_ui ();

Because setup_ui methods are a bad practice, it's up to the constructors and setters to do its job.
They were needed only because the objects were accessing objects that were not certainly constructed during its construction (e.g. App.window).
With this design the object don't need to access objects from the outside and so can be fully set up during there construction, hence the removal of setup_ui methods.
Comment 244 Zeeshan Ali 2014-07-11 13:31:05 UTC
Review of attachment 280395 [details] [review]:

::: src/app-window.vala
@@ +90,3 @@
     public void setup_ui () {
         topbar.setup_ui ();
+        setup_wizard_ui ();

I understand the need to kill setup_ui (now) but I don't think this is the right patch to do that in. This patch is (or thats what the log says) is about removal of singleton access from wizard. If you pass all the objects (App and AppWindow) to Wizard.setup_ui() in this patch, later you can replace the setup_ui with construction parameters.
Comment 245 Adrien Plazas 2014-07-16 11:11:54 UTC
Created attachment 280787 [details] [review]
app-window: Add current-item props

Adds the current-item property to AppWindow.
Prepares its move from App to AppWindow.
Comment 246 Adrien Plazas 2014-07-16 11:12:04 UTC
Created attachment 280788 [details] [review]
app-window: Add show_properties method

Adds the show_properties method to AppWindow.
Part of the move of the current-item property from App to AppWindow.
Comment 247 Adrien Plazas 2014-07-16 11:12:13 UTC
Created attachment 280789 [details] [review]
app-window: Add connect_to method

Adds the connect_to method to AppWindow.
Part of the move of the current-item property from App to AppWindow.
Comment 248 Adrien Plazas 2014-07-16 11:12:23 UTC
Created attachment 280790 [details] [review]
app-window: Add select_item method

Adds the select_item method to AppWindow.
Part of the move of the current-item property from App to AppWindow.
Comment 249 Adrien Plazas 2014-07-16 11:12:33 UTC
Created attachment 280791 [details] [review]
app-window: Cancel machine connection on collection state

Cancels the current machine's conection when the UI is in collection state.
Part of the move of the current item from App to AppWindow.
Comment 250 Adrien Plazas 2014-07-16 11:12:43 UTC
Created attachment 280792 [details] [review]
app: Remove current_item attribute

Removes the current_item attribute from App.
It have been moved to AppWindow.
Comment 251 Adrien Plazas 2014-07-16 11:12:53 UTC
Created attachment 280793 [details] [review]
app-window: Add selection-mode props

Adds the selection-mode property to AppWindow.
Prepares its move from App to AppWindow.
Comment 252 Adrien Plazas 2014-07-16 11:13:03 UTC
Created attachment 280794 [details] [review]
collection-view: Add setup_ui method

Adds setup_ui method to CollectionView.
Prepares the move of the selection-mode prop from App to AppWindow.
Comment 253 Adrien Plazas 2014-07-16 11:13:13 UTC
Created attachment 280795 [details] [review]
selectionbar: Add setup_ui method

Adds setup_ui method to Selectionbar.
Prepares the move of the selection-mode prop from App to AppWindow.
Comment 254 Adrien Plazas 2014-07-16 11:13:23 UTC
Created attachment 280796 [details] [review]
Use AppWindow's selection-mode

Uses AppWindow's selection-mode property instead of App's one.
Comment 255 Adrien Plazas 2014-07-16 11:13:32 UTC
Created attachment 280797 [details] [review]
app: Remove selection-mode props

Removes the selection-mode property from App.
It have been moved to AppWindow.
Comment 256 Adrien Plazas 2014-07-16 11:13:42 UTC
Created attachment 280798 [details] [review]
app: Remove UI state

App no more implements UIState, it have been moved to AppWindow.
Comment 257 Adrien Plazas 2014-07-16 11:13:52 UTC
Created attachment 280799 [details] [review]
AppWindow: Remove use of App.window

App.window was accessed in AppWindow class when the current object could
be used.
Comment 258 Adrien Plazas 2014-07-16 11:16:23 UTC
The last set of patches moves some of the code from App to AppWindow.
It moves:
 * the current item
 * the selection mode
 * the ui state

It is done to prepare the use of multiple windows.
Comment 259 Adrien Plazas 2014-07-16 11:41:43 UTC
Created attachment 280805 [details] [review]
app-window: Add current-item props

Adds the current-item property to AppWindow.
Prepares its move from App to AppWindow.
Comment 260 Adrien Plazas 2014-07-16 11:41:52 UTC
Created attachment 280806 [details] [review]
app-window: Add show_properties method

Adds the show_properties method to AppWindow.
Part of the move of the current-item property from App to AppWindow.
Comment 261 Adrien Plazas 2014-07-16 11:42:01 UTC
Created attachment 280807 [details] [review]
app-window: Add connect_to method

Adds the connect_to method to AppWindow.
Part of the move of the current-item property from App to AppWindow.
Comment 262 Adrien Plazas 2014-07-16 11:42:11 UTC
Created attachment 280808 [details] [review]
app-window: Add select_item method

Adds the select_item method to AppWindow.
Part of the move of the current-item property from App to AppWindow.
Comment 263 Adrien Plazas 2014-07-16 11:42:21 UTC
Created attachment 280809 [details] [review]
app-window: Cancel machine connection on collection state

Cancels the current machine's conection when the UI is in collection state.
Part of the move of the current item from App to AppWindow.
Comment 264 Adrien Plazas 2014-07-16 11:42:32 UTC
Created attachment 280810 [details] [review]
app: Remove current_item attribute

Removes the current_item attribute from App.
It have been moved to AppWindow.
Comment 265 Adrien Plazas 2014-07-16 11:42:42 UTC
Created attachment 280811 [details] [review]
app-window: Add selection-mode props

Adds the selection-mode property to AppWindow.
Prepares its move from App to AppWindow.
Comment 266 Adrien Plazas 2014-07-16 11:42:53 UTC
Created attachment 280812 [details] [review]
collection-view: Add setup_ui method

Adds setup_ui method to CollectionView.
Prepares the move of the selection-mode prop from App to AppWindow.
Comment 267 Adrien Plazas 2014-07-16 11:43:03 UTC
Created attachment 280813 [details] [review]
selectionbar: Add setup_ui method

Adds setup_ui method to Selectionbar.
Prepares the move of the selection-mode prop from App to AppWindow.
Comment 268 Adrien Plazas 2014-07-16 11:43:14 UTC
Created attachment 280814 [details] [review]
Use AppWindow's selection-mode

Uses AppWindow's selection-mode property instead of App's one.
Comment 269 Adrien Plazas 2014-07-16 11:43:24 UTC
Created attachment 280815 [details] [review]
app: Remove selection-mode props

Removes the selection-mode property from App.
It have been moved to AppWindow.
Comment 270 Adrien Plazas 2014-07-16 11:43:34 UTC
Created attachment 280816 [details] [review]
collection-toolbar: Move UI state connction to setup_ui

Moves the connection to the App's UI state notification from the
constuctor to setup_ui.
Prepares the move of the UI state from App to AppWindow.
Comment 271 Adrien Plazas 2014-07-16 11:43:44 UTC
Created attachment 280817 [details] [review]
app: Remove UI state

App no more implements UIState, it have been moved to AppWindow.
Comment 272 Adrien Plazas 2014-07-16 11:43:54 UTC
Created attachment 280818 [details] [review]
AppWindow: Remove use of App.window

App.window was accessed in AppWindow class when the current object could
be used.
Comment 273 Adrien Plazas 2014-07-16 11:48:52 UTC
Created attachment 280820 [details] [review]
collection-toolbar: Move UI state connection to setup_ui

Moves the connection to the App's UI state notification from the
constuctor to setup_ui.
Prepares the move of the UI state from App to AppWindow.
Comment 274 Adrien Plazas 2014-07-16 11:49:02 UTC
Created attachment 280821 [details] [review]
app: Remove UI state

App no more implements UIState, it have been moved to AppWindow.
Comment 275 Adrien Plazas 2014-07-16 11:49:12 UTC
Created attachment 280822 [details] [review]
AppWindow: Remove use of App.window

App.window was accessed in AppWindow class when the current object could
be used.
Comment 276 Lasse Schuirmann 2014-07-16 12:14:27 UTC
Review of attachment 280805 [details] [review]:

Looks good otherwise. Thats almost atomic despite the comment below.

::: src/app.vala
@@ +23,3 @@
     public UIState ui_state { get; protected set; }
 
+    public CollectionItem current_item { get; set; } // current object/vm manipulated

This doesnt seem related to what you describe in the commit message.
Comment 277 Lasse Schuirmann 2014-07-16 12:14:27 UTC
Review of attachment 280805 [details] [review]:

Looks good otherwise. Thats almost atomic despite the comment below.

::: src/app.vala
@@ +23,3 @@
     public UIState ui_state { get; protected set; }
 
+    public CollectionItem current_item { get; set; } // current object/vm manipulated

This doesnt seem related to what you describe in the commit message.
Comment 278 Lasse Schuirmann 2014-07-16 12:56:03 UTC
Review of attachment 280806 [details] [review]:

Shortlog: a thing I learned from zeenix: you can shorten this by removing method and just saying "Add show_properties()"

First sentence of description says exactly the same as the shortlog. It's redundant for me. Second sentence isn't a whole sentence, how about a preceding "This is"?

Code looks good but log describes you adding show_properties while I'd say you rather move it from app to app-window.
Comment 279 Lasse Schuirmann 2014-07-16 12:58:43 UTC
Review of attachment 280806 [details] [review]:

actually all these things also apply to some other patches (at least the following.)
Comment 280 Adrien Plazas 2014-07-16 13:36:52 UTC
Review of attachment 280805 [details] [review]:

::: src/app.vala
@@ +23,3 @@
     public UIState ui_state { get; protected set; }
 
+    public CollectionItem current_item { get; set; } // current object/vm manipulated

Would changing the message to reflect that change be enough to you?
The change, while not perfect, is temporary and is corrected by a next patch.
Comment 281 Lasse Schuirmann 2014-07-16 13:38:02 UTC
Review of attachment 280805 [details] [review]:

::: src/app.vala
@@ +23,3 @@
     public UIState ui_state { get; protected set; }
 
+    public CollectionItem current_item { get; set; } // current object/vm manipulated

Would you mind to tell me the secret new commit message? ;)
Comment 282 Lasse Schuirmann 2014-07-16 13:41:26 UTC
Review of attachment 280808 [details] [review]:

looks good apart from commit message
Comment 283 Adrien Plazas 2014-07-16 13:56:11 UTC
Review of attachment 280805 [details] [review]:

What about:

app-window: Add current-item props

Adds the current-item property to AppWindow to prepare its move from App.
Makes the current-item from App public to bidirectionally bind them.
Comment 284 Lasse Schuirmann 2014-07-16 14:00:15 UTC
Review of attachment 280805 [details] [review]:

Errr... sorry. You don't need to put that in the log, my fault. But you can ;) but now I have a definite thing: cut the trailing s from the shortlog! :) You add just one prop and handle the implications arent you?
Comment 285 Lasse Schuirmann 2014-07-16 14:08:48 UTC
Review of attachment 280809 [details] [review]:

Commit message doesnt really reflect that you're moving the cancellation to app window. It sounds more like you introduce this. Can we make the movement shine through more at least in the description?
Comment 286 Lasse Schuirmann 2014-07-16 14:12:09 UTC
Review of attachment 280810 [details] [review]:

how about
app: Move current_item prop to AppWindow

as shortlog? have -> has in description.
Comment 287 Lasse Schuirmann 2014-07-16 14:13:40 UTC
Review of attachment 280807 [details] [review]:

looks good apart from commit message
Comment 288 Adrien Plazas 2014-07-16 15:27:19 UTC
Created attachment 280852 [details] [review]
app-window: Add current-item prop

Adds the current-item property to AppWindow.
Prepares its move from App to AppWindow.
Comment 289 Adrien Plazas 2014-07-16 15:27:30 UTC
Created attachment 280853 [details] [review]
app-window: Add show_properties()

Adds the show_properties method to AppWindow.
This is part of the move of the current-item property from App to AppWindow.
Comment 290 Adrien Plazas 2014-07-16 15:27:41 UTC
Created attachment 280854 [details] [review]
app-window: Add connect_to()

Adds the connect_to method to AppWindow.
This is part of the move of the current-item property from App to AppWindow.
Comment 291 Adrien Plazas 2014-07-16 15:27:51 UTC
Created attachment 280855 [details] [review]
app-window: Add select_item()

Adds the select_item method to AppWindow.
This is part of the move of the current-item property from App to AppWindow.
Comment 292 Adrien Plazas 2014-07-16 15:28:02 UTC
Created attachment 280856 [details] [review]
app-window: Cancel machine connection on collection state

Moves the cancelation of the current machine's conection when the UI is in
collection state from App to AppWindow.
This is part of the move of the current-item property from App to AppWindow.
Comment 293 Adrien Plazas 2014-07-16 15:28:13 UTC
Created attachment 280858 [details] [review]
app: Move current_item to AppWindow

Removes the current_item attribute from App.
It has been moved to AppWindow.
Comment 294 Adrien Plazas 2014-07-16 15:28:23 UTC
Created attachment 280859 [details] [review]
app-window: Add selection-mode prop

Adds the selection-mode property to AppWindow.
Prepares its move from App to AppWindow.
Comment 295 Adrien Plazas 2014-07-16 15:28:34 UTC
Created attachment 280860 [details] [review]
collection-view: Add setup_ui()

Adds setup_ui method to CollectionView.
This is part of the move of the selection-mode property from App to AppWindow.
Comment 296 Adrien Plazas 2014-07-16 15:28:45 UTC
Created attachment 280861 [details] [review]
selectionbar: Add setup_ui()

Adds setup_ui method to Selectionbar.
This is part of the move of the selection-mode property from App to AppWindow.
Comment 297 Adrien Plazas 2014-07-16 15:28:55 UTC
Created attachment 280862 [details] [review]
Use AppWindow's selection-mode

Uses AppWindow's selection-mode property instead of App's one.
Comment 298 Adrien Plazas 2014-07-16 15:29:06 UTC
Created attachment 280863 [details] [review]
app: Move selection-mode prop to AppWindow

Removes the selection-mode property from App.
It has been moved to AppWindow.
Comment 299 Adrien Plazas 2014-07-16 15:29:16 UTC
Created attachment 280864 [details] [review]
collection-toolbar: Move UI state connection to setup_ui

Moves the connection to the App's UI state notification from the
constuctor to setup_ui.
Prepares the move of the UI state from App to AppWindow.
Comment 300 Adrien Plazas 2014-07-16 15:29:27 UTC
Created attachment 280865 [details] [review]
app: Remove UI state

App no more implements UIState, it have been moved to AppWindow.
Comment 301 Adrien Plazas 2014-07-16 15:29:39 UTC
Created attachment 280866 [details] [review]
AppWindow: Remove use of App.window

App.window was accessed in AppWindow class when the current object could
be used.
Comment 302 Lasse Schuirmann 2014-07-16 15:47:58 UTC
Review of attachment 280852 [details] [review]:

Same as before: the first sentence of the description mirrors the summary and is therefore redundant. Not?

Ack besides.
Comment 303 Lasse Schuirmann 2014-07-16 15:48:32 UTC
Review of attachment 280853 [details] [review]:

saame with commit log as last patch.
Comment 304 Lasse Schuirmann 2014-07-16 15:59:23 UTC
Review of attachment 280854 [details] [review]:

1. You don't add it, you move it around.
2. The 1st sentence in the description is the same as the shortlog

(didnt I mention it last time on some patches?)
Comment 305 Lasse Schuirmann 2014-07-16 16:00:27 UTC
Review of attachment 280855 [details] [review]:

looks good apart commit message
Comment 306 Adrien Plazas 2014-07-17 05:07:21 UTC
Created attachment 280902 [details] [review]
app-window: Add current-item prop

Prepares its move from App to AppWindow.
Comment 307 Adrien Plazas 2014-07-17 05:07:33 UTC
Created attachment 280903 [details] [review]
app-window: Add show_properties()

This is part of the move of the current-item property from App to AppWindow.
Comment 308 Adrien Plazas 2014-07-17 05:07:43 UTC
Created attachment 280904 [details] [review]
app-window: Add connect_to()

This is part of the move of the current-item property from App to AppWindow.
Comment 309 Adrien Plazas 2014-07-17 05:07:54 UTC
Created attachment 280905 [details] [review]
app-window: Add select_item()

This is part of the move of the current-item property from App to AppWindow.
Comment 310 Adrien Plazas 2014-07-17 05:08:02 UTC
Created attachment 280906 [details] [review]
app-window: Cancel machine connection on collection state

Moves the cancelation of the current machine's conection when the UI is in
collection state from App to AppWindow.
This is part of the move of the current-item property from App to AppWindow.
Comment 311 Adrien Plazas 2014-07-17 05:08:12 UTC
Created attachment 280907 [details] [review]
app: Move current_item to AppWindow

Removes the current_item attribute from App.
It has been moved to AppWindow.
Comment 312 Adrien Plazas 2014-07-17 05:08:22 UTC
Created attachment 280908 [details] [review]
app-window: Add selection-mode prop

Prepares its move from App to AppWindow.
Comment 313 Adrien Plazas 2014-07-17 05:08:31 UTC
Created attachment 280909 [details] [review]
collection-view: Add setup_ui()

This is part of the move of the selection-mode property from App to AppWindow.
Comment 314 Adrien Plazas 2014-07-17 05:08:39 UTC
Created attachment 280910 [details] [review]
selectionbar: Add setup_ui()

This is part of the move of the selection-mode property from App to AppWindow.
Comment 315 Adrien Plazas 2014-07-17 05:08:46 UTC
Created attachment 280911 [details] [review]
Use AppWindow's selection-mode

Uses AppWindow's selection-mode property instead of App's one.
Comment 316 Adrien Plazas 2014-07-17 05:08:53 UTC
Created attachment 280912 [details] [review]
app: Move selection-mode prop to AppWindow

Removes the selection-mode property from App.
It has been moved to AppWindow.
Comment 317 Adrien Plazas 2014-07-17 05:09:01 UTC
Created attachment 280913 [details] [review]
collection-toolbar: Move UI state connection to setup_ui

Moves the connection to the App's UI state notification from the
constuctor to setup_ui.
Prepares the move of the UI state from App to AppWindow.
Comment 318 Adrien Plazas 2014-07-17 05:09:09 UTC
Created attachment 280914 [details] [review]
app: Remove UI state

App no more implements UIState, it have been moved to AppWindow.
Comment 319 Adrien Plazas 2014-07-17 05:09:16 UTC
Created attachment 280915 [details] [review]
AppWindow: Remove use of App.window

App.window was accessed in AppWindow class when the current object could
be used.
Comment 320 Lasse Schuirmann 2014-07-17 07:29:10 UTC
Review of attachment 280902 [details] [review]:

I've spoken with zeenix yesterday. These patches are very nicely atomic and he pointed out thats therefore more important to add some rationale to the log so you get the scope. E.g. with this patch I don't really know why you're moving this current_item (oh it has an underscore, not a minus like in the log, hasnt it?) one layer lower.

I know this is hard and maybe we can chat about the rationales to find something good.

I know these logs can make one mad but hey, at least they're easy to change ;)
Comment 321 Lasse Schuirmann 2014-07-17 07:35:27 UTC
Review of attachment 280903 [details] [review]:

current-item -> current_item
SHortlog: you don't add it you move it there from app dont you?

Same thing about rationale, I can't really see yet what this is good for.
Comment 322 Lasse Schuirmann 2014-07-17 07:36:30 UTC
Review of attachment 280904 [details] [review]:

same as last review
Comment 323 Lasse Schuirmann 2014-07-17 07:38:17 UTC
Review of attachment 280905 [details] [review]:

current-item -> current_item

Description line exceeds 74 chars, just break it.

Same things as last commit apply here too.
Comment 324 Lasse Schuirmann 2014-07-17 08:10:07 UTC
Review of attachment 280906 [details] [review]:

current-item -> 'current-item'

This is how zeenix does it in commit 7697874. Lets do it in yours too. (Therefore my previous comments on this don't apply and you'd need to make 'prop-name' out of it everywhere)

Plus the usual things ;) this is getting a bit chaotic so I'll stop reviewing since you're probably working on a new series right now.
Comment 325 Lasse Schuirmann 2014-07-17 08:13:55 UTC
Review of attachment 280914 [details] [review]:

this commit looks so great :) (apart from the usual log things)
I'm glad this is happening.
Comment 326 Lasse Schuirmann 2014-07-17 08:14:36 UTC
Review of attachment 280915 [details] [review]:

app-window instead of AppWindow in the shortlog
Comment 327 Lasse Schuirmann 2014-07-17 08:15:20 UTC
Review of attachment 280907 [details] [review]:

its a prop right? so 'current-item' instead of current_item will do?
Comment 328 Adrien Plazas 2014-07-17 09:47:58 UTC
Created attachment 280929 [details] [review]
app-window: Add current_item prop

Prepares its move from App to AppWindow because each window may have a
different current item.
Also makes App's current_item a property to bind them during the move.
Comment 329 Adrien Plazas 2014-07-17 09:48:09 UTC
Created attachment 280930 [details] [review]
app-window: Move show_properties() from App

This is part of the move of the current_item property from App to AppWindow.
It uses only things that are part of AppWindow (view, current_item) or will be
moved to it in next patches (selection_mode, ui_state) and so it should be
moved there too.
Comment 330 Adrien Plazas 2014-07-17 09:48:19 UTC
Created attachment 280931 [details] [review]
app-window: Move connect_to() from App

Allows to connect the current item to its window, so it should be part of
AppWindow as current_item is moving from App to it.
Comment 331 Adrien Plazas 2014-07-17 09:48:30 UTC
Created attachment 280932 [details] [review]
app-window: Move select_item() from App

As the current_item prop is moving from App to AppWindow, select_item should
follow it.
Comment 332 Adrien Plazas 2014-07-17 09:48:42 UTC
Created attachment 280933 [details] [review]
app-window: Cancel machine connection on collection state

Moves the cancelation of the current machine's conection when the UI is in
collection state from App to AppWindow.
As the current_item prop is moving from App to AppWindow and as the UI state
will too, the cancellation should move there too.
Comment 333 Adrien Plazas 2014-07-17 09:48:53 UTC
Created attachment 280934 [details] [review]
app: Remove current_item

Removes the current_item property from App.
Also make the current_item property of AppWindow an attribute as it was only
needed to bind current items during the move.
It finishes its move form App to AppWindow.
Comment 334 Adrien Plazas 2014-07-17 09:49:04 UTC
Created attachment 280935 [details] [review]
app-window: Add selection_mode prop

Prepares its move from App to AppWindow because each window may been in
selection mode independently from the others.
Comment 335 Adrien Plazas 2014-07-17 09:49:15 UTC
Created attachment 280936 [details] [review]
collection-view: Add setup_ui()

This is part of the move of the selection_mode property from App to AppWindow.
Selection mode is moving from App to Appwindow, so we must ensure that the
window is constructed when trying to connect to the selection mode.
Comment 336 Adrien Plazas 2014-07-17 09:49:28 UTC
Created attachment 280938 [details] [review]
selectionbar: Add setup_ui()

This is part of the move of the selection_mode property from App to AppWindow.
Selection mode is moving from App to Appwindow, so we must ensure that the
window is constructed when trying to connect to the selection mode.
Comment 337 Adrien Plazas 2014-07-17 09:49:39 UTC
Created attachment 280939 [details] [review]
Use AppWindow's selection_mode

Uses AppWindow's selection_mode property instead of App's one.
Comment 338 Adrien Plazas 2014-07-17 09:49:52 UTC
Created attachment 280940 [details] [review]
app: Remove selection_mode prop

Removes the selection_mode property from App.
It finishes its move form App to AppWindow.
Comment 339 Adrien Plazas 2014-07-17 09:50:03 UTC
Created attachment 280941 [details] [review]
collection-toolbar: Move UI state connection to setup_ui

Moves the connection to the App's UI state notification from the
constuctor to setup_ui.
Prepares the move of the UI state from App to AppWindow.
UI state will be removed from App in favor of AppWindow, so we must ensure that
the window is constructed when trying to connect to the UI state.
Comment 340 Adrien Plazas 2014-07-17 09:50:15 UTC
Created attachment 280942 [details] [review]
app: Remove UI state

App no more implements UIState, its role is transfered to AppWindow as each
window can have a different UI state from the others.
Comment 341 Adrien Plazas 2014-07-17 09:50:27 UTC
Created attachment 280943 [details] [review]
AppWindow: Remove use of App.window

App.window was accessed in AppWindow class when the current object should
be used.
Comment 342 Adrien Plazas 2014-07-17 10:51:37 UTC
Created attachment 280947 [details] [review]
app-window: Add 'current-item' prop

Prepares its move from App to AppWindow because each window may have a
different current item.
Also makes App's current_item ttribute a property to bind them during the
move.
Comment 343 Adrien Plazas 2014-07-17 10:51:44 UTC
Created attachment 280948 [details] [review]
app-window: Move show_properties() from App

It uses only things that are part of AppWindow ('view', 'current-item')
or will be moved to it in next patches ('selection-mode', 'ui-state') and
so it should be moved to AppWindow.
Comment 344 Adrien Plazas 2014-07-17 10:51:52 UTC
Created attachment 280949 [details] [review]
app-window: Move connect_to() from App

Allows to connect the current item to its window, so it should be part of
AppWindow as 'current-item' is moving from App to it.
Comment 345 Adrien Plazas 2014-07-17 10:52:04 UTC
Created attachment 280950 [details] [review]
app-window: Move select_item() from App

As the 'current-item' property is moving from App to AppWindow,
select_item() should follow it.
Comment 346 Adrien Plazas 2014-07-17 10:52:17 UTC
Created attachment 280951 [details] [review]
app-window: Cancel machine connection on collection state

Moves the cancelation of the current machine's conection when the UI is in
collection state from App to AppWindow.
As the 'current-item' property is moving from App to AppWindow and as the
UI state will too, the cancellation should move.
Comment 347 Adrien Plazas 2014-07-17 10:52:29 UTC
Created attachment 280952 [details] [review]
app: Remove 'current-item'

Removes the 'current-item' property from App.
Also make the 'current-item' property of AppWindow an attribute as it was
only needed to bind current items during the move.
It finishes its move from App to AppWindow.
Comment 348 Adrien Plazas 2014-07-17 10:52:41 UTC
Created attachment 280953 [details] [review]
app-window: Add 'selection-mode' prop

Prepares its move from App to AppWindow because each window may been in
selection mode independently from the others.
Comment 349 Adrien Plazas 2014-07-17 10:52:53 UTC
Created attachment 280954 [details] [review]
collection-view: Add setup_ui()

This is part of the move of the 'selection-mode' property from App to
AppWindow.
'selection-mode' is moving from App to Appwindow, so we must ensure that
the window is constructed when trying to connect to notification of
'selection-mode'.
Comment 350 Adrien Plazas 2014-07-17 10:53:05 UTC
Created attachment 280955 [details] [review]
selectionbar: Add setup_ui()

This is part of the move of the 'selection-mode'  property from App to
AppWindow.
'selection-mode' is moving from App to Appwindow, so we must ensure that
the window is constructed when trying to connect to the notification of
'selection-mode'.
Comment 351 Adrien Plazas 2014-07-17 10:53:16 UTC
Created attachment 280956 [details] [review]
Use AppWindow's 'selection-mode' prop instead of App's
Comment 352 Adrien Plazas 2014-07-17 10:53:28 UTC
Created attachment 280957 [details] [review]
app: Remove 'selection-mode' prop

It finishes its move from App to AppWindow.
Comment 353 Adrien Plazas 2014-07-17 10:53:39 UTC
Created attachment 280958 [details] [review]
collection-toolbar: Move 'ui-state' connection to setup_ui()

Moves the connection to the App's 'ui-state' notifications from the
constuctor to setup_ui().
UIState will no more be implemented by App in favor of AppWindow, so we
must ensure that the window is constructed when trying to connect to
'ui-state' notifications.
Comment 354 Adrien Plazas 2014-07-17 10:53:52 UTC
Created attachment 280959 [details] [review]
app: No more implements UIState

Its role is transfered to AppWindow as each window can have a different
UI state from the others.
Comment 355 Adrien Plazas 2014-07-17 10:54:04 UTC
Created attachment 280960 [details] [review]
app-window: Remove use of App.window

App.window was accessed in AppWindow class when the current object should
be used.
Comment 356 Zeeshan Ali 2014-07-17 12:38:34 UTC
Review of attachment 280951 [details] [review]:

Since machine connection was already being cancelled on collection state, the short log is a bit misleading. "Move connection cancellation from App" ?
Comment 357 Zeeshan Ali 2014-07-17 12:43:45 UTC
Review of attachment 280953 [details] [review]:

been -> be.
Comment 358 Zeeshan Ali 2014-07-17 12:51:45 UTC
Review of attachment 280954 [details] [review]:

* If you want to put something on newline (talking of log), you should make it a new paragraph. Looks more correct and more readable.

* 'when trying to connect' -> 'before connecting'?
Comment 359 Zeeshan Ali 2014-07-17 12:53:41 UTC
Review of attachment 280955 [details] [review]:

::: src/app-window.vala
@@ +44,3 @@
     public Notificationbar notificationbar;
     [GtkChild]
+    public Selectionbar selectionbar;

why public?
Comment 360 Zeeshan Ali 2014-07-17 12:57:11 UTC
Review of attachment 280956 [details] [review]:

shorter shortlog:

"Drop usage of App.selection_mode

All classes now use AppWindow.selection_mode rather than App.selection_mode.

RATIONALE FOR WHY"
Comment 361 Adrien Plazas 2014-07-17 12:59:15 UTC
Created attachment 280972 [details] [review]
app-window: Add 'current-item' prop

Prepares its move from App to AppWindow because each window may have a
different current item.
Also makes App's current_item ttribute a property to bind them during the
move.

This is needed to have multiple windows.
Comment 362 Adrien Plazas 2014-07-17 12:59:28 UTC
Created attachment 280973 [details] [review]
app-window: Move show_properties() from App

It uses only things that are part of AppWindow ('view', 'current-item')
or will be moved to it in next patches ('selection-mode', 'ui-state') and
so it should be moved to AppWindow.

This is needed to have multiple windows.
Comment 363 Adrien Plazas 2014-07-17 12:59:40 UTC
Created attachment 280974 [details] [review]
app-window: Move connect_to() from App

Allows to connect the current item to its window, so it should be part of
AppWindow as 'current-item' is moving from App to it.

This is needed to have multiple windows.
Comment 364 Zeeshan Ali 2014-07-17 13:01:38 UTC
Review of attachment 280958 [details] [review]:

"in favor of AppWindow" -> "but by AppWindow". I'd rather write "UI state is moving from App to AppWindow" actually.
Comment 365 Zeeshan Ali 2014-07-17 13:04:21 UTC
Review of attachment 280959 [details] [review]:

"Move UI state from App to AppWindow" is better shortlog IMHO.
Comment 366 Zeeshan Ali 2014-07-17 13:06:15 UTC
Review of attachment 280960 [details] [review]:

I'd append " singleton" to shortlog.
Comment 367 Adrien Plazas 2014-07-17 13:16:47 UTC
Created attachment 280981 [details] [review]
app-window: Add 'current-item' prop

Prepares its move from App to AppWindow because each window may have a
different current item.
Also makes App's current_item ttribute a property to bind them during the
move.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 368 Adrien Plazas 2014-07-17 13:17:00 UTC
Created attachment 280982 [details] [review]
app-window: Move show_properties() from App

It uses only things that are part of AppWindow ('view', 'current-item')
or will be moved to it in next patches ('selection-mode', 'ui-state') and
so it should be moved to AppWindow.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 369 Adrien Plazas 2014-07-17 13:17:12 UTC
Created attachment 280983 [details] [review]
app-window: Move connect_to() from App

Allows to connect the current item to its window, so it should be part of
AppWindow as 'current-item' is moving from App to it.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 370 Adrien Plazas 2014-07-17 13:17:25 UTC
Created attachment 280984 [details] [review]
app-window: Move select_item() from App

As the 'current-item' property is moving from App to AppWindow,
select_item() should follow it.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 371 Adrien Plazas 2014-07-17 13:17:38 UTC
Created attachment 280985 [details] [review]
app-window: Cancel machine connection on collection state

Moves the cancelation of the current machine's conection when the UI is in
collection state from App to AppWindow.
As the 'current-item' property is moving from App to AppWindow and as the
UI state will too, the cancellation should move.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 372 Adrien Plazas 2014-07-17 13:17:51 UTC
Created attachment 280986 [details] [review]
app: Remove 'current-item'

Removes the 'current-item' property from App.
Also make the 'current-item' property of AppWindow an attribute as it was
only needed to bind current items during the move.
It finishes its move from App to AppWindow.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 373 Adrien Plazas 2014-07-17 13:18:04 UTC
Created attachment 280987 [details] [review]
app-window: Add 'selection-mode' prop

Prepares its move from App to AppWindow because each window may been in
selection mode independently from the others.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 374 Adrien Plazas 2014-07-17 13:18:14 UTC
Created attachment 280988 [details] [review]
collection-view: Add setup_ui()

This is part of the move of the 'selection-mode' property from App to
AppWindow.
'selection-mode' is moving from App to Appwindow, so we must ensure that
the window is constructed when trying to connect to notification of
'selection-mode'.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 375 Adrien Plazas 2014-07-17 13:18:27 UTC
Created attachment 280989 [details] [review]
selectionbar: Add setup_ui()

This is part of the move of the 'selection-mode'  property from App to
AppWindow.
'selection-mode' is moving from App to Appwindow, so we must ensure that
the window is constructed when trying to connect to the notification of
'selection-mode'.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 376 Adrien Plazas 2014-07-17 13:18:40 UTC
Created attachment 280990 [details] [review]
Use AppWindow's 'selection-mode' prop instead of App's

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 377 Adrien Plazas 2014-07-17 13:18:53 UTC
Created attachment 280991 [details] [review]
app: Remove 'selection-mode' prop

It finishes its move from App to AppWindow.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 378 Adrien Plazas 2014-07-17 13:19:05 UTC
Created attachment 280992 [details] [review]
collection-toolbar: Move 'ui-state' connection to setup_ui()

Moves the connection to the App's 'ui-state' notifications from the
constuctor to setup_ui().
UIState will no more be implemented by App in favor of AppWindow, so we
must ensure that the window is constructed when trying to connect to
'ui-state' notifications.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 379 Adrien Plazas 2014-07-17 13:19:19 UTC
Created attachment 280993 [details] [review]
app: No more implements UIState

Its role is transfered to AppWindow as each window can have a different
UI state from the others.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 380 Adrien Plazas 2014-07-17 13:19:29 UTC
Created attachment 280994 [details] [review]
app-window: Remove use of App.window

App.window was accessed in AppWindow class when the current object should
be used.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 381 Lasse Schuirmann 2014-07-17 13:51:02 UTC
Review of attachment 280981 [details] [review]:

I'd change:

Prepares its move from App to AppWindow because each window may have a
different current item.
Also makes App's current_item ttribute a property to bind them during the
move.

to:

Prepares its move from App to AppWindow because each window may have a
different current item. Also makes App's current_item ttribute a
property to bind them during the move.

(Removed only the newline.) For me single line breaks decrease readability so I'd go for paragraphs. But thats a nitpick. Log is pretty good now :) (and the code too, of course.)
Comment 382 Lasse Schuirmann 2014-07-17 13:53:27 UTC
Review of attachment 280982 [details] [review]:

It -> show_properties ()
Comment 383 Lasse Schuirmann 2014-07-17 13:59:21 UTC
Review of attachment 280983 [details] [review]:

::: src/app.vala
@@ +169,3 @@
         bind_property ("current-item", window, "current-item", BindingFlags.BIDIRECTIONAL);
+        bind_property ("status-bind", window, "status-bind", BindingFlags.BIDIRECTIONAL);
+        bind_property ("got-error-id", window, "got-error-id", BindingFlags.BIDIRECTIONAL);

I think binding the properties is more complicated than just accessing the ones from app-window.vala directly, don't you?
Comment 384 Lasse Schuirmann 2014-07-17 14:01:54 UTC
Review of attachment 280984 [details] [review]:

This log is great. I think in the description you'd do select_item () instead of select_item() which is the right thing in the shortlog because this is limited to 50 chars.
Comment 385 Lasse Schuirmann 2014-07-17 14:11:10 UTC
Review of attachment 280985 [details] [review]:

Shortlog is a bit long and isnt a really good description IMO. Description itself is good.

What do you think about:
Handle whole UI State in AppWindow instead of App

Same thing about paragraphs/line break as in the first patch IIRC. Another very very _very_ minor thing thats rather informative than relevant: I'd change cancelation in cancellation since the former is rather american spelling and the latter is more popular. (Thus both are correct.)

Code's good.
Comment 386 Lasse Schuirmann 2014-07-17 14:15:18 UTC
Review of attachment 280983 [details] [review]:

::: src/app.vala
@@ +169,3 @@
         bind_property ("current-item", window, "current-item", BindingFlags.BIDIRECTIONAL);
+        bind_property ("status-bind", window, "status-bind", BindingFlags.BIDIRECTIONAL);
+        bind_property ("got-error-id", window, "got-error-id", BindingFlags.BIDIRECTIONAL);

not relevant anymore. Patch is good :)
Comment 387 Lasse Schuirmann 2014-07-17 14:16:09 UTC
Review of attachment 280986 [details] [review]:

In other logs you do:
<tag>: <operation> '<property-name>' prop
here you omitted the prop, looks inconsistent to me.

Same thing about line breaks here.

Also make -> Also makes
Comment 388 Lasse Schuirmann 2014-07-17 14:20:16 UTC
Review of attachment 280987 [details] [review]:

ack :)
Comment 389 Lasse Schuirmann 2014-07-17 16:04:41 UTC
Review of attachment 280988 [details] [review]:

This line breaking thing applies too.
Appwindow -> AppWindow
Comment 390 Lasse Schuirmann 2014-07-17 16:28:50 UTC
Review of attachment 280989 [details] [review]:

Line breaks.

::: src/selectionbar.vala
@@ +26,3 @@
     }
 
+    public void setup_ui () {

don't you have to remove this functionality from the construct {} method then?

@@ +30,3 @@
+            reveal_child = App.app.selection_mode;
+        });
+

This \n doesnt increase readability for me.
Comment 391 Lasse Schuirmann 2014-07-17 16:30:54 UTC
Review of attachment 280990 [details] [review]:

good otherwise

::: src/selectionbar.vala
@@ -17,3 @@
-            reveal_child = App.app.selection_mode;
-        });
-

yeah, don't this needs to be removed in the previous patch? Or am I missing something?
Comment 392 Lasse Schuirmann 2014-07-17 16:32:55 UTC
Review of attachment 280991 [details] [review]:

"It finishes its move" Thats too many inspecified it's in the first sentence!

How about:
This finishes the move of 'selection-mode' to AppWindow.
Comment 393 Lasse Schuirmann 2014-07-17 16:36:55 UTC
Review of attachment 280992 [details] [review]:

Theres the line break thing here and the bracket thing in the long description. I think zeenix won't like the "in favor of" phrasing.
Comment 394 Lasse Schuirmann 2014-07-17 16:44:40 UTC
Review of attachment 280993 [details] [review]:

"Its role" is a bit unspecific. Maybe simply write that "UI state is managed by AppWindow as each window ..."?

Short log: I don't really like it.
...
... *thinking*
...
I don't really know a better one. Lets take yours.
Comment 395 Lasse Schuirmann 2014-07-17 16:45:33 UTC
Review of attachment 280994 [details] [review]:

looks good
Comment 396 Adrien Plazas 2014-07-17 18:11:30 UTC
Created attachment 281025 [details] [review]
app-window: Add 'current-item' prop

Prepares its move from App to AppWindow because each window may have a
different current item. Also makes App's current_item attribute a
property to bind them during the move.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 397 Adrien Plazas 2014-07-17 18:11:39 UTC
Created attachment 281026 [details] [review]
app-window: Move show_properties() from App

show_properties () only uses things that are part of AppWindow ('view',
'current-item') or will be moved to it in next patches ('selection-mode',
'ui-state') and so it should be moved to AppWindow.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 398 Adrien Plazas 2014-07-17 18:11:51 UTC
Created attachment 281027 [details] [review]
app-window: Move connect_to() from App

Allows to connect the current item to its window, so it should be part of
AppWindow as 'current-item' is moving from App to it.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 399 Adrien Plazas 2014-07-17 18:12:01 UTC
Created attachment 281028 [details] [review]
app-window: Move select_item() from App

As the 'current-item' property is moving from App to AppWindow,
select_item () should follow it.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 400 Adrien Plazas 2014-07-17 18:12:11 UTC
Created attachment 281029 [details] [review]
app-window: Disconnect machine on collection page

Moves the cancellation of the current machine's conection when the UI is
in collection state from App to AppWindow. As the 'current-item' property
is moving from App to AppWindow and as the UI state will too, the
cancellation should move. Also moves the setting of the current item's
state from App to AppWindow.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 401 Adrien Plazas 2014-07-17 18:12:26 UTC
Created attachment 281030 [details] [review]
app: Remove 'current-item' prop

Removes the 'current-item' property from App. Also makes the
'current-item' property of AppWindow an attribute as it was only needed
to bind current items during the move. It finishes its move from App to
AppWindow.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 402 Adrien Plazas 2014-07-17 18:12:40 UTC
Created attachment 281031 [details] [review]
app-window: Add 'selection-mode' prop

Prepares its move from App to AppWindow because each window may been in
selection mode independently from the others.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 403 Adrien Plazas 2014-07-17 18:12:54 UTC
Created attachment 281032 [details] [review]
collection-view: Add setup_ui()

This is part of the move of the 'selection-mode' property from App to
AppWindow. 'selection-mode' is moving from App to AppWindow, so we must
ensure that the window is constructed when trying to connect to
notification of 'selection-mode'.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 404 Adrien Plazas 2014-07-17 18:13:07 UTC
Created attachment 281033 [details] [review]
selectionbar: Add setup_ui()

This is part of the move of the 'selection-mode'  property from App to
AppWindow. 'selection-mode' is moving from App to Appwindow, so we must
ensure that the window is constructed when trying to connect to the
notification of 'selection-mode'.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 405 Adrien Plazas 2014-07-17 18:13:21 UTC
Created attachment 281034 [details] [review]
Use AppWindow's 'selection-mode' prop instead of App's

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 406 Adrien Plazas 2014-07-17 18:13:33 UTC
Created attachment 281035 [details] [review]
app: Remove 'selection-mode' prop

Finishes the move of the 'selection-mode' property from App to AppWindow.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 407 Adrien Plazas 2014-07-17 18:13:46 UTC
Created attachment 281036 [details] [review]
collection-toolbar: Move 'ui-state' connection to setup_ui()

Moves the connection to the App's 'ui-state' notifications from the
constuctor to setup_ui (). UIState will no more be implemented by App in
favor of AppWindow, so we must ensure that the window is constructed when
trying to connect to 'ui-state' notifications.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 408 Adrien Plazas 2014-07-17 18:14:00 UTC
Created attachment 281037 [details] [review]
app: No more implements UIState

UI state is now managed by AppWindow as each window can have a different
UI state from the others.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 409 Adrien Plazas 2014-07-17 18:14:13 UTC
Created attachment 281038 [details] [review]
app-window: Remove use of App.window

App.window was accessed in AppWindow class when the current object should
be used.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 410 Lasse Schuirmann 2014-07-17 18:31:49 UTC
Review of attachment 281025 [details] [review]:

ack
Comment 411 Lasse Schuirmann 2014-07-17 18:31:50 UTC
Review of attachment 281026 [details] [review]:

ack
Comment 412 Lasse Schuirmann 2014-07-17 18:31:51 UTC
Review of attachment 281027 [details] [review]:

ack
Comment 413 Lasse Schuirmann 2014-07-17 18:31:51 UTC
Review of attachment 281028 [details] [review]:

One could say those "should follow it" phrasings are not really a good reason but the reason is rather obvious because it just makes sense anyway. Ack.
Comment 414 Lasse Schuirmann 2014-07-17 18:31:53 UTC
Review of attachment 281029 [details] [review]:

"like" thats modern isn't it? Or is this out of date by now... I was never up to date on those social network things...
Comment 415 Lasse Schuirmann 2014-07-17 18:31:54 UTC
Review of attachment 281030 [details] [review]:

ack too
Comment 416 Lasse Schuirmann 2014-07-17 18:31:55 UTC
Review of attachment 281031 [details] [review]:

Shortlog: I hope I didn't propose this shortlog. Sometimes I propose things to people and then state them they're wrong ;) in any case thats not personal but just stupid me.
How about:
Move 'selection-mode' prop from App to AppWindow
Comment 417 Lasse Schuirmann 2014-07-17 18:31:57 UTC
Review of attachment 281032 [details] [review]:

you know how boring it is ACKing all your patches?
Comment 418 Lasse Schuirmann 2014-07-17 18:31:58 UTC
Review of attachment 281033 [details] [review]:

...really...
Comment 419 Lasse Schuirmann 2014-07-17 18:32:00 UTC
Review of attachment 281034 [details] [review]:

... r e a l l y ...
Comment 420 Lasse Schuirmann 2014-07-17 18:32:01 UTC
Review of attachment 281035 [details] [review]:

...boring!
Comment 421 Lasse Schuirmann 2014-07-17 18:32:03 UTC
Review of attachment 281036 [details] [review]:

ACKnowledged.
Comment 422 Lasse Schuirmann 2014-07-17 18:32:04 UTC
Review of attachment 281037 [details] [review]:

*yawn*
Comment 423 Lasse Schuirmann 2014-07-17 18:32:05 UTC
Review of attachment 281038 [details] [review]:

Zzzzzz....
Comment 424 Adrien Plazas 2014-07-17 18:36:19 UTC
Review of attachment 281031 [details] [review]:

I t doesn't really move it as it still exists in App and will be removed in a next patch.
It's not a one step move, first an addition, then a removal. =p
Comment 425 Lasse Schuirmann 2014-07-17 18:38:26 UTC
Review of attachment 281031 [details] [review]:

isnt it more exact to say the first step is you move it over to app-window and second is the full removal? You don't add the 'selection-mode' out of nothing.
Comment 426 Adrien Plazas 2014-07-17 18:41:33 UTC
Review of attachment 281031 [details] [review]:

In a sense it is more exact, but in another way, it still exists in App so it have not been "moved".
The log then explain were it come from.

I don't think "move" reflect reality well enough, but maybe something better than both could be found?
Comment 427 Zeeshan Ali 2014-07-18 00:03:26 UTC
(In reply to comment #417)
> Review of attachment 281032 [details] [review]:
> 
> you know how boring it is ACKing all your patches?

(In reply to comment #418)
> Review of attachment 281033 [details] [review]:
> 
> ...really...

While I realize its boring and I really appreciate you doing this boring activity for us, these comments aren't very helpful on their own since you are marking them as 'reviewed'. Even if you were marking as 'accepted_commitnow', the updated status unfortunately doesn't make it to the comment on bz so there is no easy way of telling easily what you are saying. So even if its boring, please just write a simple 'ack'. :)
Comment 428 Zeeshan Ali 2014-07-18 00:11:42 UTC
Review of attachment 281038 [details] [review]:

* "This is needed to drop the use of AppWindow singleton"

In case of this patch, this is not correct but rather the patch is removing some of the singleton usage.

* You missed my comment on the previous version of this patch: I'd append " singleton" to shortlog.

* I think this patch should remove App.window usage from all objects since all the previous patches to this are to make this possible. Also once we at least are removing the singleton usage, we have the actual changes that we are aiming for and therefore I can merge the patches.
Comment 429 Lasse Schuirmann 2014-07-18 07:13:30 UTC
(In reply to comment #427)
> (In reply to comment #417)
> > Review of attachment 281032 [details] [review] [details]:
> > 
> > you know how boring it is ACKing all your patches?
> 
> (In reply to comment #418)
> > Review of attachment 281033 [details] [review] [details]:
> > 
> > ...really...
> 
> While I realize its boring and I really appreciate you doing this boring
> activity for us, these comments aren't very helpful on their own since you are
> marking them as 'reviewed'. Even if you were marking as 'accepted_commitnow',
> the updated status unfortunately doesn't make it to the comment on bz so there
> is no easy way of telling easily what you are saying. So even if its boring,
> please just write a simple 'ack'. :)

Ok, sorry. I thought it was obvious and IIRC I informed both of you (also Kekun) that I'll mark acked patches as reviewed. (I just entered a message because you get an error from bz if you try to change the status only.) Btw. not the reviews are boring but writing "ack" all the time, at the same time I intended to say suggestively to Kekun that he's doing good work.

I'll stick for ack in the future. ;)
Comment 430 Adrien Plazas 2014-07-18 15:57:22 UTC
Created attachment 281109 [details] [review]
topbar: Add AppWindow param to setup_ui()

It allows to setup the topbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 431 Adrien Plazas 2014-07-18 15:57:35 UTC
Created attachment 281110 [details] [review]
collection-toolbar:  Add AppWindow param to setup_ui()

It allows to setup the collection toolbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 432 Adrien Plazas 2014-07-18 15:57:48 UTC
Created attachment 281111 [details] [review]
selection-toolbar:  Add AppWindow param to setup_ui()

It allows to setup the selection toolbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 433 Adrien Plazas 2014-07-18 15:58:03 UTC
Created attachment 281112 [details] [review]
wizard:  Add AppWindow param to setup_ui()

It allows to setup the wizard for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 434 Adrien Plazas 2014-07-18 15:58:15 UTC
Created attachment 281113 [details] [review]
wizard-toolbar: Add 'wizard' prop

It allows to setup the wizard toolbar for a parametrable wizard.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 435 Adrien Plazas 2014-07-18 15:58:30 UTC
Created attachment 281114 [details] [review]
display-page:  Add AppWindow param to setup_ui()

It allows to setup the display page for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 436 Adrien Plazas 2014-07-18 15:58:41 UTC
Created attachment 281116 [details] [review]
display-toolbar: Add setup_ui()

It allows to setup the display toolbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 437 Adrien Plazas 2014-07-18 15:58:53 UTC
Created attachment 281117 [details] [review]
collection-view: Add AppWindow param to setup_ui()

It allows to setup the collection view for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 438 Adrien Plazas 2014-07-18 15:59:06 UTC
Created attachment 281118 [details] [review]
selectionbar: Add AppWindow param to setup_ui()

It allows to setup the selectionbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 439 Adrien Plazas 2014-07-18 15:59:20 UTC
Created attachment 281119 [details] [review]
auth-notfication: Add Searchbar param to constructor

It allows to setup the auth notification for a parametrable searchbar.
Also, this adds a 'searchbar' property to Notificationbar to build the
auth notification with.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 440 Adrien Plazas 2014-07-18 15:59:33 UTC
Created attachment 281120 [details] [review]
searchbar: Add setup_ui()

It allows to setup the searchbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 441 Adrien Plazas 2014-07-18 15:59:46 UTC
Created attachment 281121 [details] [review]
sidebar: Add setup_ui()

It allows to setup the sidebar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 442 Adrien Plazas 2014-07-18 16:00:00 UTC
Created attachment 281122 [details] [review]
properties: Add AppWindow param to setup_ui()

It allows to setup the properties for a parametrable window. Also, this
makes setup_ui () public and remove its call from Properties' constructor.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 443 Adrien Plazas 2014-07-18 16:00:10 UTC
Created attachment 281123 [details] [review]
properties-toolbar: Add setup_ui()

It allows to setup the properties toolbar for a parametrable window.
Also, this makes the topbar's props_toolbar a PropertiesToolbar instead of
a Gtk.HeaderBar.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 444 Adrien Plazas 2014-07-18 16:00:21 UTC
Created attachment 281124 [details] [review]
empty-boxes: Add setup_ui()

It allows to setup the empty boxes for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 445 Adrien Plazas 2014-07-18 16:00:35 UTC
Created attachment 281125 [details] [review]
wizard-source: Add setup_ui()

It allows to setup the wizard source for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 446 Adrien Plazas 2014-07-18 16:00:47 UTC
Created attachment 281126 [details] [review]
app: Add get_main_window()

This allows to access the app's main window in OvirtBrobker, VMCreator and
VMImporter.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 447 Adrien Plazas 2014-07-18 16:01:01 UTC
Created attachment 281127 [details] [review]
machine: Add 'window' prop

It allows to setup the machine for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 448 Adrien Plazas 2014-07-18 16:01:16 UTC
Created attachment 281128 [details] [review]
libvirt-machine: Remove usage of App.window singleton

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 449 Adrien Plazas 2014-07-18 16:01:30 UTC
Created attachment 281129 [details] [review]
libvirt-machine-properties: Remove usage of App.window singleton

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 450 Adrien Plazas 2014-07-18 16:01:44 UTC
Created attachment 281130 [details] [review]
spice-display: Add 'machine' prop

SpiceDisplay checks the state of it's machine instead of the App.window
singleton's one.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 451 Adrien Plazas 2014-07-18 16:01:58 UTC
Created attachment 281131 [details] [review]
app-window: Remove usage of App.window singleton

App.window was accessed in AppWindow class when the current object should
be used.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 452 Adrien Plazas 2014-07-18 16:02:13 UTC
Created attachment 281132 [details] [review]
app: Remove window singleton

App's window attribute is now a non-static private instance.

This is needed to make multiple windows possible.
Comment 453 Lasse Schuirmann 2014-07-18 16:11:53 UTC
Review of attachment 281109 [details] [review]:

looks good otherwise

::: src/topbar.vala
@@ +99,3 @@
 
+    public void setup_ui (AppWindow window) {
+        assert (window != null);

Vala should do this automatically since you're using AppWindow and not AppWindow?.
Comment 454 Lasse Schuirmann 2014-07-18 16:16:43 UTC
Review of attachment 281110 [details] [review]:

::: src/collection-toolbar.vala
@@ +24,3 @@
 
+    public void setup_ui (AppWindow window) {
+        assert (window != null);

same as with last patch

@@ +25,3 @@
+    public void setup_ui (AppWindow window) {
+        assert (window != null);
+        assert (window.searchbar != null);

AppWindow.searchbar is a Searchbar (without ?) so this check is also unneeded.
Comment 455 Lasse Schuirmann 2014-07-18 16:19:36 UTC
Review of attachment 281111 [details] [review]:

::: src/selection-toolbar.vala
@@ +24,3 @@
+    public void setup_ui (AppWindow window) {
+        assert (window != null);
+        assert (window.searchbar != null);

maybe do an own patch "Remove unneeded asserts" which removes all these asserts so nobody complains that this change doesnt belong here
Comment 456 Adrien Plazas 2014-07-18 16:22:41 UTC
Review of attachment 281109 [details] [review]:

::: src/topbar.vala
@@ +99,3 @@
 
+    public void setup_ui (AppWindow window) {
+        assert (window != null);

Objects are nullable in Vala, "?" is for non referenced values such as int or struct (it basically makes a pointer out of them).
Comment 457 Lasse Schuirmann 2014-07-18 16:23:14 UTC
Review of attachment 281112 [details] [review]:

::: src/wizard.vala
@@ +538,3 @@
 
+    public void setup_ui (AppWindow window) {
+        assert (window != null);

not needed
Comment 458 Lasse Schuirmann 2014-07-18 16:28:32 UTC
Review of attachment 281109 [details] [review]:

::: src/topbar.vala
@@ +99,3 @@
 
+    public void setup_ui (AppWindow window) {
+        assert (window != null);

https://wiki.gnome.org/Projects/Vala/Tutorial#Assertions_and_Contract_Programming
"You might be tempted to use assertions in order to check method arguments for null. However, this is not necessary, since Vala does that implicitly for all parameters that are not marked with ? as being nullable. "
Comment 459 Lasse Schuirmann 2014-07-18 16:35:40 UTC
Review of attachment 281113 [details] [review]:

basically this is an ack ;)

::: src/wizard-toolbar.vala
@@ +13,3 @@
     public Button create_btn;
 
+    public Wizard wizard { get; set; }

Hm. Some theory:
You say here: this is a Wizard object and the following invariant is true at every time: wizard != null. (Else you'd write Wizard?)
In order to guarantee that invariant you'd need to establish in the constructor and make sure that every method that touches the invariant reinstates it.

Ok. I'm crazy. Nobody does this. At least it seems that nobody who wrote on this program did that ;) so I don't know if there's any point in starting to do it now but I think thats a thing worth to waste a thought.
Comment 460 Lasse Schuirmann 2014-07-18 16:37:05 UTC
Review of attachment 281114 [details] [review]:

::: src/display-page.vala
@@ +39,3 @@
 
+    public void setup_ui (AppWindow window) {
+        assert (window != null);

as discussed, not necessary
Comment 461 Adrien Plazas 2014-08-03 07:40:43 UTC
Created attachment 282361 [details] [review]
app-window: Add 'current-item' prop

Prepares its move from App to AppWindow because each window may have a
different current item. Also makes App's current_item attribute a
property to bind them during the move.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 462 Adrien Plazas 2014-08-03 07:40:58 UTC
Created attachment 282362 [details] [review]
app-window: Move show_properties() from App

show_properties () only uses things that are part of AppWindow ('view',
'current-item') or will be moved to it in next patches ('selection-mode',
'ui-state') and so it should be moved to AppWindow.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 463 Adrien Plazas 2014-08-03 07:41:13 UTC
Created attachment 282363 [details] [review]
app-window: Move connect_to() from App

Allows to connect the current item to its window, so it should be part of
AppWindow as 'current-item' is moving from App to it.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 464 Adrien Plazas 2014-08-03 07:41:27 UTC
Created attachment 282364 [details] [review]
app-window: Move select_item() from App

As the 'current-item' property is moving from App to AppWindow,
select_item () should follow it.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 465 Adrien Plazas 2014-08-03 07:41:42 UTC
Created attachment 282365 [details] [review]
app-window: Disconnect machine on collection page

Moves the cancellation of the current machine's conection when the UI is
in collection state from App to AppWindow. As the 'current-item' property
is moving from App to AppWindow and as the UI state will too, the
cancellation should move. Also moves the setting of the current item's
state from App to AppWindow.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 466 Adrien Plazas 2014-08-03 07:41:55 UTC
Created attachment 282366 [details] [review]
app: Remove 'current-item' prop

Removes the 'current-item' property from App. Also makes the
'current-item' property of AppWindow an attribute as it was only needed
to bind current items during the move. It finishes its move from App to
AppWindow.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 467 Adrien Plazas 2014-08-03 07:42:09 UTC
Created attachment 282367 [details] [review]
app-window: Add 'selection-mode' prop

Prepares its move from App to AppWindow because each window may been in
selection mode independently from the others.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 468 Adrien Plazas 2014-08-03 07:42:23 UTC
Created attachment 282368 [details] [review]
collection-view: Add setup_ui()

This is part of the move of the 'selection-mode' property from App to
AppWindow. 'selection-mode' is moving from App to AppWindow, so we must
ensure that the window is constructed when trying to connect to
notification of 'selection-mode'.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 469 Adrien Plazas 2014-08-03 07:42:38 UTC
Created attachment 282369 [details] [review]
selectionbar: Add setup_ui()

This is part of the move of the 'selection-mode'  property from App to
AppWindow. 'selection-mode' is moving from App to Appwindow, so we must
ensure that the window is constructed when trying to connect to the
notification of 'selection-mode'.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 470 Adrien Plazas 2014-08-03 07:42:52 UTC
Created attachment 282370 [details] [review]
Use AppWindow's 'selection-mode' prop instead of App's

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 471 Adrien Plazas 2014-08-03 07:43:07 UTC
Created attachment 282371 [details] [review]
app: Remove 'selection-mode' prop

Finishes the move of the 'selection-mode' property from App to AppWindow.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 472 Adrien Plazas 2014-08-03 07:43:22 UTC
Created attachment 282372 [details] [review]
collection-toolbar: Move 'ui-state' connection to setup_ui()

Moves the connection to the App's 'ui-state' notifications from the
constuctor to setup_ui (). UIState will no more be implemented by App in
favor of AppWindow, so we must ensure that the window is constructed when
trying to connect to 'ui-state' notifications.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 473 Adrien Plazas 2014-08-03 07:43:37 UTC
Created attachment 282373 [details] [review]
app: No more implements UIState

UI state is now managed by AppWindow as each window can have a different
UI state from the others.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 474 Adrien Plazas 2014-08-03 07:43:51 UTC
Created attachment 282374 [details] [review]
topbar: Add AppWindow param to setup_ui()

It allows to setup the topbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 475 Adrien Plazas 2014-08-03 07:44:05 UTC
Created attachment 282375 [details] [review]
collection-toolbar:  Add AppWindow param to setup_ui()

It allows to setup the collection toolbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 476 Adrien Plazas 2014-08-03 07:44:20 UTC
Created attachment 282376 [details] [review]
selection-toolbar:  Add AppWindow param to setup_ui()

It allows to setup the selection toolbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 477 Adrien Plazas 2014-08-03 07:44:35 UTC
Created attachment 282377 [details] [review]
wizard:  Add AppWindow param to setup_ui()

It allows to setup the wizard for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 478 Adrien Plazas 2014-08-03 07:44:49 UTC
Created attachment 282378 [details] [review]
display-page:  Add AppWindow param to setup_ui()

It allows to setup the display page for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 479 Adrien Plazas 2014-08-03 07:45:04 UTC
Created attachment 282379 [details] [review]
display-toolbar: Add setup_ui()

It allows to setup the display toolbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 480 Adrien Plazas 2014-08-03 07:45:18 UTC
Created attachment 282380 [details] [review]
collection-view: Add AppWindow param to setup_ui()

It allows to setup the collection view for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 481 Adrien Plazas 2014-08-03 07:45:32 UTC
Created attachment 282381 [details] [review]
selectionbar: Add AppWindow param to setup_ui()

It allows to setup the selectionbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 482 Adrien Plazas 2014-08-03 07:45:47 UTC
Created attachment 282382 [details] [review]
auth-notfication: Add Searchbar param to constructor

It allows to setup the auth notification for a parametrable searchbar.
Also, this adds a 'searchbar' property to Notificationbar to build the
auth notification with.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 483 Adrien Plazas 2014-08-03 07:46:01 UTC
Created attachment 282383 [details] [review]
searchbar: Add setup_ui()

It allows to setup the searchbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 484 Adrien Plazas 2014-08-03 07:46:17 UTC
Created attachment 282384 [details] [review]
sidebar: Add setup_ui()

It allows to setup the sidebar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 485 Adrien Plazas 2014-08-03 07:46:31 UTC
Created attachment 282385 [details] [review]
properties: Add AppWindow param to setup_ui()

It allows to setup the properties for a parametrable window. Also, this
makes setup_ui () public and remove its call from Properties' constructor.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 486 Adrien Plazas 2014-08-03 07:46:45 UTC
Created attachment 282386 [details] [review]
properties-toolbar: Add setup_ui()

It allows to setup the properties toolbar for a parametrable window.
Also, this makes the topbar's props_toolbar a PropertiesToolbar instead of
a Gtk.HeaderBar.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 487 Adrien Plazas 2014-08-03 07:47:00 UTC
Created attachment 282387 [details] [review]
empty-boxes: Add setup_ui()

It allows to setup the empty boxes for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 488 Adrien Plazas 2014-08-03 07:47:15 UTC
Created attachment 282388 [details] [review]
wizard-source: Add setup_ui()

It allows to setup the wizard source for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 489 Adrien Plazas 2014-08-03 07:47:30 UTC
Created attachment 282389 [details] [review]
machine: Add 'window' prop

It allows to setup the machine for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 490 Adrien Plazas 2014-08-03 07:47:45 UTC
Created attachment 282390 [details] [review]
libvirt-machine: Remove usage of App.window singleton

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 491 Adrien Plazas 2014-08-03 07:48:01 UTC
Created attachment 282391 [details] [review]
libvirt-machine-properties: Remove usage of App.window singleton

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 492 Adrien Plazas 2014-08-03 07:48:16 UTC
Created attachment 282392 [details] [review]
spice-display: Add 'machine' prop

SpiceDisplay checks the state of it's machine instead of the App.window
singleton's one.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 493 Adrien Plazas 2014-08-03 07:48:31 UTC
Created attachment 282393 [details] [review]
app-window: Remove usage of App.window singleton

App.window was accessed in AppWindow class when the current object should
be used.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 494 Adrien Plazas 2014-08-03 07:48:46 UTC
Created attachment 282394 [details] [review]
app: Remove window singleton

App's window attribute is now a non-static private instance.

This is needed to make multiple windows possible.
Comment 495 Adrien Plazas 2014-08-03 08:19:51 UTC
Created attachment 282395 [details] [review]
wizard-source: Add setup_ui()

It allows to setup the wizard source for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 496 Adrien Plazas 2014-08-03 08:20:02 UTC
Created attachment 282396 [details] [review]
app: Add get_main_window()

This allows to access the app's main window in OvirtBrobker, VMCreator and
VMImporter.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 497 Adrien Plazas 2014-08-03 08:20:15 UTC
Created attachment 282397 [details] [review]
machine: Add 'window' prop

It allows to setup the machine for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 498 Adrien Plazas 2014-08-03 08:20:30 UTC
Created attachment 282398 [details] [review]
libvirt-machine: Remove usage of App.window singleton

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 499 Adrien Plazas 2014-08-03 08:20:43 UTC
Created attachment 282399 [details] [review]
libvirt-machine-properties: Remove usage of App.window singleton

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 500 Adrien Plazas 2014-08-03 08:20:59 UTC
Created attachment 282400 [details] [review]
spice-display: Add 'machine' prop

SpiceDisplay checks the state of it's machine instead of the App.window
singleton's one.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 501 Adrien Plazas 2014-08-03 08:21:14 UTC
Created attachment 282401 [details] [review]
app-window: Remove usage of App.window singleton

App.window was accessed in AppWindow class when the current object should
be used.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 502 Adrien Plazas 2014-08-03 08:21:30 UTC
Created attachment 282402 [details] [review]
app: Remove window singleton

App's window attribute is now a non-static private instance.

This is needed to make multiple windows possible.
Comment 503 Adrien Plazas 2014-08-03 12:06:29 UTC
Created attachment 282407 [details] [review]
app: Replace window singleton by windows list

This is needed to make multiple windows possible.
Comment 504 Adrien Plazas 2014-08-04 15:27:33 UTC
Created attachment 282456 [details] [review]
app: Add WindowsCollection class

It will help to manage multiple windows, and therefore to make multiple
windows possible.
Comment 505 Adrien Plazas 2014-08-04 15:27:50 UTC
Created attachment 282457 [details] [review]
app: Replace AppWindow singleton by WindowsCollection

This is needed to make multiple windows possible.
Comment 506 Adrien Plazas 2014-08-04 15:28:07 UTC
Created attachment 282458 [details] [review]
app: Remove get_main_window()

The app uses windows.get_main() instead.

Moving window management code to the WindowsCollection class will help to
manage multiple windows, and therefore to make multiple windows possible.
Comment 507 Adrien Plazas 2014-08-04 15:28:23 UTC
Created attachment 282459 [details] [review]
app-window: Closing window removes it from collection

This is needed to make multiple windows possible.
Comment 508 Adrien Plazas 2014-08-04 15:28:38 UTC
Created attachment 282460 [details] [review]
app-window: Present when machine selected

This is needed to make multiple windows possible.
Comment 509 Adrien Plazas 2014-08-04 15:28:54 UTC
Created attachment 282461 [details] [review]
app-window: Add 'main' prop

This is needed to make multiple windows possible.
Comment 510 Adrien Plazas 2014-08-04 15:29:11 UTC
Created attachment 282462 [details] [review]
app: Populate new main window's collection

This is needed to make multiple windows possible.
Comment 511 Adrien Plazas 2014-08-04 15:33:12 UTC
These patches allow to have multiple windows, each one with its own machine, and behaving correctly.
They introduce no UI change which would allow to actually have multiple windows.
Comment 512 Lasse Schuirmann 2014-08-04 18:28:13 UTC
Review of attachment 282361 [details] [review]:

ack
Comment 513 Lasse Schuirmann 2014-08-04 18:35:16 UTC
Review of attachment 282362 [details] [review]:

ack
Comment 514 Lasse Schuirmann 2014-08-04 18:36:40 UTC
Review of attachment 282363 [details] [review]:

ack
Comment 515 Lasse Schuirmann 2014-08-05 16:03:24 UTC
Review of attachment 282364 [details] [review]:

ack
Comment 516 Lasse Schuirmann 2014-08-05 16:09:28 UTC
Review of attachment 282365 [details] [review]:

not sure about the comment below, else: good :)

::: src/app.vala
@@ +402,2 @@
     private void ui_state_changed () {
         window.set_state (ui_state);

now that ui_state_changed contains only one line and given the fact that its invoked only once how about moving it into a lambda in line 107?
Comment 517 Lasse Schuirmann 2014-08-05 16:11:26 UTC
Review of attachment 282366 [details] [review]:

ack
Comment 518 Lasse Schuirmann 2014-08-05 16:13:41 UTC
Review of attachment 282367 [details] [review]:

dont know if I like the commit message enough, but we have zeeshan for this. So ack from me.
Comment 519 Lasse Schuirmann 2014-08-05 16:19:08 UTC
Review of attachment 282368 [details] [review]:

ack
Comment 520 Lasse Schuirmann 2014-08-05 16:24:55 UTC
Review of attachment 282369 [details] [review]:

ack
Comment 521 Lasse Schuirmann 2014-08-05 16:27:33 UTC
Review of attachment 282370 [details] [review]:

I think this and the following patch can safely be merged and belong together. You can do this easily by doing
git rebase --interactive
(at least I didnt know about this until I saw zeeshan doing this)
Comment 522 Lasse Schuirmann 2014-08-05 16:27:51 UTC
Review of attachment 282371 [details] [review]:

see comment of last patch
Comment 523 Zeeshan Ali 2014-08-05 16:43:01 UTC
Review of attachment 282365 [details] [review]:

ack

::: src/app.vala
@@ +402,2 @@
     private void ui_state_changed () {
         window.set_state (ui_state);

Nah, let it be like this.
Comment 524 Zeeshan Ali 2014-08-05 16:45:43 UTC
Review of attachment 282366 [details] [review]:

ack
Comment 525 Zeeshan Ali 2014-08-05 17:11:09 UTC
Review of attachment 282367 [details] [review]:

ack
Comment 526 Zeeshan Ali 2014-08-05 17:13:19 UTC
Review of attachment 282368 [details] [review]:

ack
Comment 527 Zeeshan Ali 2014-08-05 17:20:41 UTC
Review of attachment 282369 [details] [review]:

The shortlog doesn't exactly tell anything meaninful (same is true for previous patch to this). The description part also doesn't say anything about this patch itself but only about the overall change this patch is part of. Patch is fine otherwise.
Comment 528 Zeeshan Ali 2014-08-05 17:21:30 UTC
Review of attachment 282368 [details] [review]:

Actually same comment as the next patch (which is about selection-bar).
Comment 529 Zeeshan Ali 2014-08-05 17:34:52 UTC
Review of attachment 282370 [details] [review]:

agree with Lasse. Although I don't think its worth it to keep this from getting merged, since you gotta edit the previous patch anyway, you might as well merge this at the same time. Maybe that also makes it easy to make the commit logs more meaningful.
Comment 530 Zeeshan Ali 2014-08-05 17:36:04 UTC
Review of attachment 282371 [details] [review]:

same comment as Lasse :)
Comment 531 Adrien Plazas 2014-08-05 19:03:56 UTC
Review of attachment 282368 [details] [review]:

What about:

collection-view: Setup UI once window is constructed

This is part of the move of the 'selection-mode' property from App to
AppWindow. This adds setup_ui() to the CollectionView class to ensure that
the connection to the notification of 'selection-mode' is done once the
window is constructed as 'selection-mode' is moving from App to Appwindow.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 532 Adrien Plazas 2014-08-05 19:04:16 UTC
Review of attachment 282369 [details] [review]:

What about:

selectionbar: Setup UI once window is constructed

This is part of the move of the 'selection-mode' property from App to
AppWindow. This adds setup_ui() to the Selectionbar class to ensure that
the connection to the notification of 'selection-mode' is done once the
window is constructed as 'selection-mode' is moving from App to Appwindow.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 533 Zeeshan Ali 2014-08-06 11:41:11 UTC
Review of attachment 282369 [details] [review]:

That sounds good.
Comment 534 Zeeshan Ali 2014-08-06 11:44:01 UTC
Review of attachment 282368 [details] [review]:

Good (just like the other patch) but I just realized that " as 'selection-mode' is moving from App to Appwindow" part is now redundant.
Comment 535 Adrien Plazas 2014-08-06 12:12:42 UTC
Created attachment 282663 [details] [review]
collection-view: Setup UI once window is constructed

This is part of the move of the 'selection-mode' property from App to
AppWindow. This adds setup_ui() to the CollectionView class to ensure that
the connection to the notification of 'selection-mode' is done once the
window is constructed.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 536 Adrien Plazas 2014-08-06 12:12:57 UTC
Created attachment 282664 [details] [review]
selectionbar: Setup UI once window is constructed

This is part of the move of the 'selection-mode' property from App to
AppWindow. This adds setup_ui() to the Selectionbar class to ensure that
the connection to the notification of 'selection-mode' is done once the
window is constructed.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 537 Adrien Plazas 2014-08-06 12:13:14 UTC
Created attachment 282665 [details] [review]
Use AppWindow's 'selection-mode' prop instead of App's

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 538 Adrien Plazas 2014-08-06 12:13:29 UTC
Created attachment 282666 [details] [review]
collection-toolbar: Move 'ui-state' connection to setup_ui()

Moves the connection to the App's 'ui-state' notifications from the
constuctor to setup_ui (). UIState will no more be implemented by App in
favor of AppWindow, so we must ensure that the window is constructed when
trying to connect to 'ui-state' notifications.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 539 Adrien Plazas 2014-08-06 12:13:44 UTC
Created attachment 282667 [details] [review]
app: No more implements UIState

UI state is now managed by AppWindow as each window can have a different
UI state from the others.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 540 Adrien Plazas 2014-08-06 12:14:01 UTC
Created attachment 282668 [details] [review]
topbar: Add AppWindow param to setup_ui()

It allows to setup the topbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 541 Adrien Plazas 2014-08-06 12:14:17 UTC
Created attachment 282669 [details] [review]
collection-toolbar:  Add AppWindow param to setup_ui()

It allows to setup the collection toolbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 542 Adrien Plazas 2014-08-06 12:14:34 UTC
Created attachment 282670 [details] [review]
selection-toolbar:  Add AppWindow param to setup_ui()

It allows to setup the selection toolbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 543 Adrien Plazas 2014-08-06 12:14:49 UTC
Created attachment 282671 [details] [review]
wizard:  Add AppWindow param to setup_ui()

It allows to setup the wizard for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 544 Adrien Plazas 2014-08-06 12:15:05 UTC
Created attachment 282672 [details] [review]
display-page:  Add AppWindow param to setup_ui()

It allows to setup the display page for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 545 Adrien Plazas 2014-08-06 12:15:21 UTC
Created attachment 282673 [details] [review]
display-toolbar: Add setup_ui()

It allows to setup the display toolbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 546 Adrien Plazas 2014-08-06 12:15:39 UTC
Created attachment 282674 [details] [review]
collection-view: Add AppWindow param to setup_ui()

It allows to setup the collection view for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 547 Adrien Plazas 2014-08-06 12:15:55 UTC
Created attachment 282675 [details] [review]
selectionbar: Add AppWindow param to setup_ui()

It allows to setup the selectionbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 548 Adrien Plazas 2014-08-06 12:16:11 UTC
Created attachment 282676 [details] [review]
auth-notfication: Add Searchbar param to constructor

It allows to setup the auth notification for a parametrable searchbar.
Also, this adds a 'searchbar' property to Notificationbar to build the
auth notification with.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 549 Adrien Plazas 2014-08-06 12:16:28 UTC
Created attachment 282677 [details] [review]
searchbar: Add setup_ui()

It allows to setup the searchbar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 550 Adrien Plazas 2014-08-06 12:16:45 UTC
Created attachment 282678 [details] [review]
sidebar: Add setup_ui()

It allows to setup the sidebar for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 551 Adrien Plazas 2014-08-06 12:17:02 UTC
Created attachment 282679 [details] [review]
properties: Add AppWindow param to setup_ui()

It allows to setup the properties for a parametrable window. Also, this
makes setup_ui () public and remove its call from Properties' constructor.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 552 Adrien Plazas 2014-08-06 12:17:19 UTC
Created attachment 282680 [details] [review]
properties-toolbar: Add setup_ui()

It allows to setup the properties toolbar for a parametrable window.
Also, this makes the topbar's props_toolbar a PropertiesToolbar instead of
a Gtk.HeaderBar.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 553 Adrien Plazas 2014-08-06 12:17:36 UTC
Created attachment 282681 [details] [review]
empty-boxes: Add setup_ui()

It allows to setup the empty boxes for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 554 Adrien Plazas 2014-08-06 12:17:53 UTC
Created attachment 282682 [details] [review]
wizard-source: Add setup_ui()

It allows to setup the wizard source for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 555 Adrien Plazas 2014-08-06 12:18:10 UTC
Created attachment 282683 [details] [review]
app: Add get_main_window()

This allows to access the app's main window in OvirtBrobker, VMCreator and
VMImporter.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 556 Adrien Plazas 2014-08-06 12:18:26 UTC
Created attachment 282684 [details] [review]
machine: Add 'window' prop

It allows to setup the machine for a parametrable window.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 557 Adrien Plazas 2014-08-06 12:18:43 UTC
Created attachment 282685 [details] [review]
libvirt-machine: Remove usage of App.window singleton

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 558 Adrien Plazas 2014-08-06 12:18:58 UTC
Created attachment 282686 [details] [review]
libvirt-machine-properties: Remove usage of App.window singleton

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 559 Adrien Plazas 2014-08-06 12:19:14 UTC
Created attachment 282687 [details] [review]
spice-display: Add 'machine' prop

SpiceDisplay checks the state of it's machine instead of the App.window
singleton's one.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 560 Adrien Plazas 2014-08-06 12:19:30 UTC
Created attachment 282688 [details] [review]
app-window: Remove usage of App.window singleton

App.window was accessed in AppWindow class when the current object should
be used.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 561 Adrien Plazas 2014-08-06 12:19:44 UTC
Created attachment 282689 [details] [review]
app: Add WindowsCollection class

It will help to manage multiple windows, and therefore to make multiple
windows possible.
Comment 562 Adrien Plazas 2014-08-06 12:19:58 UTC
Created attachment 282690 [details] [review]
app: Replace AppWindow singleton by WindowsCollection

This is needed to make multiple windows possible.
Comment 563 Adrien Plazas 2014-08-06 12:20:14 UTC
Created attachment 282691 [details] [review]
app: Remove get_main_window()

The app uses windows.get_main() instead.

Moving window management code to the WindowsCollection class will help to
manage multiple windows, and therefore to make multiple windows possible.
Comment 564 Adrien Plazas 2014-08-06 12:20:30 UTC
Created attachment 282692 [details] [review]
app-window: Closing window removes it from collection

This is needed to make multiple windows possible.
Comment 565 Adrien Plazas 2014-08-06 12:20:46 UTC
Created attachment 282693 [details] [review]
app-window: Present when machine selected

This is needed to make multiple windows possible.
Comment 566 Adrien Plazas 2014-08-06 12:21:03 UTC
Created attachment 282694 [details] [review]
app-window: Add 'main' prop

This is needed to make multiple windows possible.
Comment 567 Adrien Plazas 2014-08-06 12:21:14 UTC
Created attachment 282695 [details] [review]
app: Populate new main window's collection

This is needed to make multiple windows possible.
Comment 568 Zeeshan Ali 2014-08-06 19:20:42 UTC
Review of attachment 282663 [details] [review]:

ack
Comment 569 Zeeshan Ali 2014-08-06 19:22:00 UTC
Review of attachment 282664 [details] [review]:

ack
Comment 570 Zeeshan Ali 2014-08-06 19:24:30 UTC
Review of attachment 282665 [details] [review]:

ack
Comment 571 Zeeshan Ali 2014-08-06 19:28:40 UTC
Review of attachment 282666 [details] [review]:

* "Moves"-> "Move" or "This moves" or "This patches moves". :)
* "in favor of AppWindow" -> "but rather by AppWindow".
Comment 572 Zeeshan Ali 2014-08-06 19:30:46 UTC
Review of attachment 282667 [details] [review]:

ack
Comment 573 Zeeshan Ali 2014-08-06 19:37:17 UTC
Review of attachment 282668 [details] [review]:

Adding of param to setup_ui() is more a detail here. The actual change is topbar now keeping a ref to AppWindow and using that instead of using the singleton. I'll write:

topbar: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, Topbar now has its own reference
to AppWindow that it receives through a paremeter to setup_ui().

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 574 Zeeshan Ali 2014-08-06 19:38:06 UTC
Review of attachment 282669 [details] [review]:

Same comment as to previous patch.
Comment 575 Zeeshan Ali 2014-08-06 19:38:40 UTC
Review of attachment 282670 [details] [review]:

same
Comment 576 Zeeshan Ali 2014-08-06 19:39:20 UTC
Review of attachment 282671 [details] [review]:

same
Comment 577 Zeeshan Ali 2014-08-06 19:40:07 UTC
Review of attachment 282672 [details] [review]:

same
Comment 578 Zeeshan Ali 2014-08-06 19:46:05 UTC
Review of attachment 282673 [details] [review]:

Same thing here: Addition of setup_ui() is a detail, not the actual change.
Comment 579 Zeeshan Ali 2014-08-06 19:48:16 UTC
Review of attachment 282674 [details] [review]:

same here
Comment 580 Zeeshan Ali 2014-08-06 19:48:43 UTC
Review of attachment 282675 [details] [review]:

same
Comment 581 Zeeshan Ali 2014-08-06 19:58:44 UTC
Review of attachment 282676 [details] [review]:

* Same thing here, the change is about AuthNotification getting direct access to searchbar instance so it doesn't have to access it through singleton.
* Googling for 'parametrable' leads me into French dictionaries and since I haven't heard this term before, I doubt its term used in English. :)

::: src/notificationbar.vala
@@ +7,3 @@
     GLib.List<Widget> active_notifications;
 
+    public Searchbar searchbar { get; set; }

Why a property here rather than a field like in AuthNotification?
Comment 582 Zeeshan Ali 2014-08-06 19:59:39 UTC
Review of attachment 282677 [details] [review]:

same
Comment 583 Zeeshan Ali 2014-08-06 20:00:26 UTC
Review of attachment 282678 [details] [review]:

same
Comment 584 Zeeshan Ali 2014-08-06 20:03:16 UTC
Review of attachment 282679 [details] [review]:

same
Comment 585 Zeeshan Ali 2014-08-06 20:05:00 UTC
Review of attachment 282680 [details] [review]:

Same comment about log

::: src/topbar.vala
@@ +28,2 @@
     [GtkChild]
+    private PropertiesToolbar props_toolbar;

How is this change related to this patch?
Comment 586 Zeeshan Ali 2014-08-06 20:05:33 UTC
Review of attachment 282681 [details] [review]:

same
Comment 587 Zeeshan Ali 2014-08-06 20:06:14 UTC
Review of attachment 282682 [details] [review]:

same
Comment 588 Zeeshan Ali 2014-08-06 20:15:21 UTC
Review of attachment 282683 [details] [review]:

::: src/app.vala
@@ +483,3 @@
     }
+
+    public unowned AppWindow get_main_window () {

Use property whenever possible/appropriate.
Comment 589 Zeeshan Ali 2014-08-06 20:16:08 UTC
Review of attachment 282685 [details] [review]:

ack
Comment 590 Zeeshan Ali 2014-08-06 20:17:53 UTC
Review of attachment 282686 [details] [review]:

We could have slightly shorter shortlog: libvirt-machine-properties: Drop use of App.window singleton
Comment 591 Zeeshan Ali 2014-08-06 20:22:08 UTC
Review of attachment 282684 [details] [review]:

Same commment about use of 'parametrable' term.

::: src/machine.vala
@@ +79,3 @@
     }
 
+    private unowned AppWindow _window;

if AppWindow has no ref to machine, we don't want the 'unowned'. It only confuses reader if its redundant.
Comment 592 Zeeshan Ali 2014-08-06 20:28:14 UTC
Review of attachment 282687 [details] [review]:

In the log, we reserve present continuous verb form for describing how things are before the patch. "checks" -> "now checks" would be better. :)

::: src/spice-display.vala
@@ +97,2 @@
         requires (port != 0 || tls_port != 0) {
+        Object (machine: machine);

We have been avoiding use of gobject-style construction so would be better to not start using it here. Instead of declaring 'machine' construct property, just declare it 'private set' and this just set it from here, like other props/fields are being initialized.

@@ +118,3 @@
 
+    public SpiceDisplay.with_uri (Machine machine, BoxConfig config, string uri) {
+        Object (machine: machine);

same here i guess.
Comment 593 Adrien Plazas 2014-08-07 09:10:49 UTC
Review of attachment 282680 [details] [review]:

::: src/topbar.vala
@@ +28,2 @@
     [GtkChild]
+    private PropertiesToolbar props_toolbar;

It is so we can call "props_toolbar.setup_ui (window)".
Comment 594 Adrien Plazas 2014-08-07 11:32:23 UTC
Review of attachment 282684 [details] [review]:

::: src/machine.vala
@@ +79,3 @@
     }
 
+    private unowned AppWindow _window;

It does as AppWindow haves a current_item CollectionItem attribute which should be a Machine, machine which here store this very same AppWindow, causing a reference cycle.
Comment 595 Adrien Plazas 2014-08-07 11:52:01 UTC
Created attachment 282773 [details] [review]
collection-toolbar: Move 'ui-state' connection to setup_ui()

This moves the connection to the App's 'ui-state' notifications from the
constuctor to setup_ui (). UIState will no more be implemented by App but
rather by AppWindow, so we must ensure that the window is constructed when
trying to connect to 'ui-state' notifications.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 596 Adrien Plazas 2014-08-07 11:52:33 UTC
Created attachment 282774 [details] [review]
app: No more implements UIState

UI state is now managed by AppWindow as each window can have a different
UI state from the others.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 597 Adrien Plazas 2014-08-07 11:52:58 UTC
Created attachment 282775 [details] [review]
topbar: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, Topbar now has its own reference
to AppWindow that it receives through a parameter to setup_ui().

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 598 Adrien Plazas 2014-08-07 11:53:15 UTC
Created attachment 282776 [details] [review]
collection-toolbar: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, CollectionToolbar now has its
own reference to AppWindow that it receives through a parameter to
setup_ui().

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 599 Adrien Plazas 2014-08-07 11:53:31 UTC
Created attachment 282777 [details] [review]
selection-toolbar: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, SelectionToolbar now has its
own reference to AppWindow that it receives through a parameter to
setup_ui().

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 600 Adrien Plazas 2014-08-07 11:53:49 UTC
Created attachment 282778 [details] [review]
wizard: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, Wizard now has its own
reference to AppWindow that it receives through a parameter to setup_ui().

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 601 Adrien Plazas 2014-08-07 11:54:06 UTC
Created attachment 282779 [details] [review]
display-page: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, DisplayPage now has its own
reference to AppWindow that it receives through a parameter to setup_ui().

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 602 Adrien Plazas 2014-08-07 11:54:21 UTC
Created attachment 282780 [details] [review]
display-toolbar: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, DisplayToolbar now has its own
reference to AppWindow that it receives through a parameter to setup_ui().

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 603 Adrien Plazas 2014-08-07 11:54:39 UTC
Created attachment 282781 [details] [review]
collection-view: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, CollectionView now has its own
reference to AppWindow that it receives through a parameter to setup_ui().

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 604 Adrien Plazas 2014-08-07 11:54:58 UTC
Created attachment 282782 [details] [review]
selectionbar: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, Selectionbar now has its own
reference to AppWindow that it receives through a parameter to setup_ui().

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 605 Adrien Plazas 2014-08-07 11:55:16 UTC
Created attachment 282783 [details] [review]
auth-notfication: Drop use of AppWindow singleton

Instead of using the AppWindow singleton to access the Searchbar,
AuthNotification now has its own reference to Searchbar that it receives
through a parameter to its constructor.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 606 Adrien Plazas 2014-08-07 11:55:33 UTC
Created attachment 282784 [details] [review]
searchbar: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, Searchbar now has its own
reference to AppWindow that it receives through a parameter to setup_ui().

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 607 Adrien Plazas 2014-08-07 11:55:49 UTC
Created attachment 282785 [details] [review]
sidebar: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, Sidebar now has its own
reference to AppWindow that it receives through a parameter to setup_ui().

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 608 Adrien Plazas 2014-08-07 11:56:07 UTC
Created attachment 282786 [details] [review]
properties: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, Properties now has its own
reference to AppWindow that it receives through a parameter to setup_ui().

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 609 Adrien Plazas 2014-08-07 11:56:24 UTC
Created attachment 282787 [details] [review]
properties-toolbar: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, PropertiesToolbar now has its
own reference to AppWindow that it receives through a parameter to
setup_ui().

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 610 Adrien Plazas 2014-08-07 11:56:42 UTC
Created attachment 282789 [details] [review]
empty-boxes: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, EmptyBoxes now has its own
reference to AppWindow that it receives through a parameter to setup_ui().

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 611 Adrien Plazas 2014-08-07 11:57:00 UTC
Created attachment 282790 [details] [review]
wizard-source: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, WizardSource now has its own
reference to AppWindow that it receives through a parameter to setup_ui().

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 612 Adrien Plazas 2014-08-07 11:57:17 UTC
Created attachment 282791 [details] [review]
app: Add 'main-window' prop

This allows to access the app's main window in OvirtBrobker, VMCreator and
VMImporter.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 613 Adrien Plazas 2014-08-07 11:57:33 UTC
Created attachment 282792 [details] [review]
machine: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, Machine now has its own
reference to AppWindow as the 'window' property.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 614 Adrien Plazas 2014-08-07 11:57:46 UTC
Created attachment 282793 [details] [review]
libvirt-machine: Drop use of AppWindow singleton

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 615 Adrien Plazas 2014-08-07 11:58:01 UTC
Created attachment 282794 [details] [review]
libvirt-machine-properties: Drop use of AppWindow singleton

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 616 Adrien Plazas 2014-08-07 11:58:20 UTC
Created attachment 282795 [details] [review]
spice-display: Add 'machine' prop

SpiceDisplay now checks the state of it's machine instead of the
App.window singleton's one.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 617 Adrien Plazas 2014-08-07 11:58:38 UTC
Created attachment 282796 [details] [review]
app-window: Remove usage of App.window singleton

App.window was accessed in AppWindow class when the current object should
be used.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 618 Adrien Plazas 2014-08-07 11:58:57 UTC
Created attachment 282797 [details] [review]
app: Add WindowsCollection class

It will help to manage multiple windows, and therefore to make multiple
windows possible.
Comment 619 Adrien Plazas 2014-08-07 11:59:15 UTC
Created attachment 282798 [details] [review]
app: Replace AppWindow singleton by WindowsCollection

This is needed to make multiple windows possible.
Comment 620 Adrien Plazas 2014-08-07 11:59:33 UTC
Created attachment 282799 [details] [review]
app: Remove 'main-window' prop

The app uses WindowsCollection's 'main' prop instead.

Moving window management code to the WindowsCollection class will help to
manage multiple windows, and therefore to make multiple windows possible.
Comment 621 Adrien Plazas 2014-08-07 11:59:51 UTC
Created attachment 282800 [details] [review]
app-window: Add 'main' prop

This is needed to make multiple windows possible.
Comment 622 Adrien Plazas 2014-08-07 12:00:07 UTC
Created attachment 282801 [details] [review]
app-window: Closing window removes it from collection

This is needed to make multiple windows possible.
Comment 623 Adrien Plazas 2014-08-07 12:00:24 UTC
Created attachment 282802 [details] [review]
app-window: Present when machine selected

This is needed to make multiple windows possible.
Comment 624 Adrien Plazas 2014-08-07 12:00:42 UTC
Created attachment 282803 [details] [review]
app: Populate new main window's collection

This is needed to make multiple windows possible.
Comment 625 Zeeshan Ali 2014-08-07 16:45:10 UTC
Review of attachment 282791 [details] [review]:

So that you later remove it? :) Sorry I didn't get to the bottom of the patch series before so I didn't realize you later remove this property. You'll need to rework this and the following patches so you don't add API that you later remove in the same patch series.
Comment 626 Zeeshan Ali 2014-08-07 16:59:28 UTC
Review of attachment 282797 [details] [review]:

::: src/app.vala
@@ +16,3 @@
 }
 
+private class Boxes.WindowsCollection : Object {

I don't think we need a special collection class for windows and especially implementation of iterator. Just have a list in App.App and have any needed API (which isn't much if you remove the Iterator class) in there.
Comment 627 Zeeshan Ali 2014-08-07 17:26:25 UTC
Review of attachment 282797 [details] [review]:

::: src/app.vala
@@ +30,3 @@
+        window.present ();
+
+        if (window == main)

As mentioned on IRC, this is not a good way to testing if window is the first in the list since its not very obvious. Lets check size of 'windows' or compare window with first item in 'windows' instead.
Comment 628 Zeeshan Ali 2014-08-07 17:59:47 UTC
Review of attachment 282801 [details] [review]:

::: src/app.vala
@@ +39,3 @@
 
+    public bool remove (AppWindow window) {
+        if (windows.find (window) == null)

Don't think we need to check this. If window is not in the list, it just wont be removed.

@@ +48,3 @@
+            return App.app.quit_app ();
+
+        if (!main.main)

I think its a bit weird that we have two separate notions of what is a main window: The first window on the windows list and the one that is marked as such with a bool property on it and we need to keep them in sync like this. I'd just get rid of the bool property and always compare with App.main.

BTW, not the right place for this comment but i'd prefer 'main_window' name rather than 'main'.
Comment 629 Zeeshan Ali 2014-08-07 18:08:41 UTC
Review of attachment 282801 [details] [review]:

::: src/app-window.vala
@@ +311,3 @@
+            var machine = current_item as Machine;
+
+            machine.window = null;

* Its unlikely we are going to ever actually have non-Machine item so lets make the condition a g_return_if_fail instead.

* nitpick: just code will be simpler and still very readable if you don't use a variable:

if (current_item is Machine)
    (current_item as Machine).window = null;
Comment 630 Zeeshan Ali 2014-08-07 18:18:36 UTC
Review of attachment 282802 [details] [review]:

I first thought that you always have a window for each item looking at this patch, I think you want to put in the log that this is only if machine is already running in a window.

::: src/app-window.vala
@@ +222,3 @@
     public void select_item (CollectionItem item) {
         if (ui_state == UIState.COLLECTION && !selection_mode) {
+            if (item is Machine) {

same comment about item never being non-Machine.
Comment 631 Zeeshan Ali 2014-08-07 18:31:54 UTC
Review of attachment 282803 [details] [review]:

Not very clear from commit log what the patch is about. Whats does 'populate' mean and whats 'new main window'? Maybe it should have been part of the patch that added App.remove ?

::: src/app.vala
@@ +48,3 @@
             return App.app.quit_app ();
 
+        // The main window have been removed, make the next window the main one

* this comment should have been added in previous patch along with the if condition below.
* We want this inside the 'if' so its clear we are talking about the case that is true

::: src/collection.vala
@@ +51,3 @@
+
+    public void populate (CollectionView view) {
+        for (uint i = 0 ; i < items.length ; i++) {

coding-style: No braces for single statements
Comment 632 Adrien Plazas 2014-08-07 19:07:02 UTC
Review of attachment 282791 [details] [review]:

As discussed on IRC, I wont change this commit as we finally will keep this property. =)
Comment 633 Adrien Plazas 2014-08-08 07:44:52 UTC
Created attachment 282867 [details] [review]
app: Add 'main-window' prop

This allows to access the app's main window in OvirtBrobker, VMCreator and
VMImporter.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 634 Adrien Plazas 2014-08-08 07:45:10 UTC
Created attachment 282868 [details] [review]
machine: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, Machine now has its own
reference to AppWindow as the 'window' property.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 635 Adrien Plazas 2014-08-08 07:45:29 UTC
Created attachment 282869 [details] [review]
libvirt-machine: Drop use of AppWindow singleton

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 636 Adrien Plazas 2014-08-08 07:45:47 UTC
Created attachment 282870 [details] [review]
libvirt-machine-properties: Drop use of AppWindow singleton

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 637 Adrien Plazas 2014-08-08 07:46:05 UTC
Created attachment 282871 [details] [review]
spice-display: Add 'machine' prop

SpiceDisplay now checks the state of it's machine instead of the
App.window singleton's one.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 638 Adrien Plazas 2014-08-08 07:46:23 UTC
Created attachment 282872 [details] [review]
app-window: Remove usage of App.window singleton

App.window was accessed in AppWindow class when the current object should
be used.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 639 Adrien Plazas 2014-08-08 07:46:40 UTC
Created attachment 282873 [details] [review]
app: Replace AppWindow singleton by List<Boxes.AppWindow>

This is needed to make multiple windows possible.
Comment 640 Adrien Plazas 2014-08-08 07:46:59 UTC
Created attachment 282874 [details] [review]
app-window: Closing window removes it from collection

This is needed to make multiple windows possible.
Comment 641 Adrien Plazas 2014-08-08 07:47:17 UTC
Created attachment 282875 [details] [review]
Set toolbar UI depending on if window is the main one

This is needed to forbid non-main windows to access the collection and
therefore to make multiple windows possible.
Comment 642 Adrien Plazas 2014-08-08 07:47:36 UTC
Created attachment 282876 [details] [review]
app-window: Present window when machine selected

This presents a machine's window if the machine is selected and is already
running in a window, otherwise it creates a new window for the machine.

This is needed to make multiple windows possible.
Comment 643 Adrien Plazas 2014-08-08 07:47:54 UTC
Created attachment 282877 [details] [review]
app: Populate new main window's collection

This is needed to make multiple windows possible.
Comment 644 Zeeshan Ali 2014-08-08 14:29:14 UTC
As discussed on IRC, due to overwelming amount of patches, lets make this bug about the refactoring required and create another one for actual multi-window patches.
Comment 645 Zeeshan Ali 2014-08-08 14:43:09 UTC
Review of attachment 282867 [details] [review]:

You are adding 'window' property bug log says you are adding 'main-window' property. Just update the log and we are good. We'll want to rename this to "main_window" in bug#734486 I guess.
Comment 646 Zeeshan Ali 2014-08-08 14:55:25 UTC
Comment on attachment 282873 [details] [review]
app: Replace AppWindow singleton by List<Boxes.AppWindow>

I guess this is where we draw the line to say that its more a patch about implementing multi-window rather than refactoring required.
Comment 647 Adrien Plazas 2014-08-08 15:36:12 UTC
Created attachment 282926 [details] [review]
app: Add 'main-window' prop

This allows to access the app's main window in OvirtBrobker, VMCreator and
VMImporter.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 648 Adrien Plazas 2014-08-08 15:36:30 UTC
Created attachment 282927 [details] [review]
machine: Drop use of AppWindow singleton

Instead of using the AppWindow singleton, Machine now has its own
reference to AppWindow as the 'window' property.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 649 Adrien Plazas 2014-08-08 15:36:49 UTC
Created attachment 282928 [details] [review]
libvirt-machine: Drop use of AppWindow singleton

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 650 Adrien Plazas 2014-08-08 15:37:04 UTC
Created attachment 282929 [details] [review]
libvirt-machine-properties: Drop use of AppWindow singleton

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 651 Adrien Plazas 2014-08-08 15:37:23 UTC
Created attachment 282930 [details] [review]
spice-display: Add 'machine' prop

SpiceDisplay now checks the state of it's machine instead of the
App.window singleton's one.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 652 Adrien Plazas 2014-08-08 15:37:42 UTC
Created attachment 282931 [details] [review]
app-window: Remove usage of App.window singleton

App.window was accessed in AppWindow class when the current object should
be used.

This is needed to drop the use of AppWindow singleton and therefore to
make multiple windows possible.
Comment 653 Zeeshan Ali 2014-08-08 16:22:07 UTC
Attachment 282361 [details] pushed as 3926808 - app-window: Add 'current-item' prop
Attachment 282362 [details] pushed as b57445e - app-window: Move show_properties() from App
Attachment 282363 [details] pushed as c88fd65 - app-window: Move connect_to() from App
Attachment 282364 [details] pushed as 1132dca - app-window: Move select_item() from App
Attachment 282365 [details] pushed as 9fb944b - app-window: Disconnect machine on collection page
Attachment 282366 [details] pushed as 395395d - app: Remove 'current-item' prop
Attachment 282367 [details] pushed as e953fbb - app-window: Add 'selection-mode' prop
Attachment 282663 [details] pushed as 72e5b77 - collection-view: Setup UI once window is constructed
Attachment 282664 [details] pushed as 5e41e15 - selectionbar: Setup UI once window is constructed
Attachment 282665 [details] pushed as 9228ecc - Use AppWindow's 'selection-mode' prop instead of App's
Attachment 282773 [details] pushed as 968e9e3 - collection-toolbar: Move 'ui-state' connection to setup_ui()
Attachment 282774 [details] pushed as adef58f - app: No more implements UIState
Attachment 282775 [details] pushed as d21ae7d - topbar: Drop use of AppWindow singleton
Attachment 282776 [details] pushed as c887018 - collection-toolbar: Drop use of AppWindow singleton
Attachment 282777 [details] pushed as 4d57be7 - selection-toolbar: Drop use of AppWindow singleton
Attachment 282778 [details] pushed as d76cd93 - wizard: Drop use of AppWindow singleton
Attachment 282779 [details] pushed as 6169121 - display-page: Drop use of AppWindow singleton
Attachment 282780 [details] pushed as fa191b4 - display-toolbar: Drop use of AppWindow singleton
Attachment 282781 [details] pushed as 4de1f9a - collection-view: Drop use of AppWindow singleton
Attachment 282782 [details] pushed as 57fa554 - selectionbar: Drop use of AppWindow singleton
Attachment 282783 [details] pushed as 7d23555 - auth-notfication: Drop use of AppWindow singleton
Attachment 282784 [details] pushed as 464fd47 - searchbar: Drop use of AppWindow singleton
Attachment 282785 [details] pushed as 164b067 - sidebar: Drop use of AppWindow singleton
Attachment 282786 [details] pushed as 127dacd - properties: Drop use of AppWindow singleton
Attachment 282787 [details] pushed as 6ac38db - properties-toolbar: Drop use of AppWindow singleton
Attachment 282789 [details] pushed as ce68a22 - empty-boxes: Drop use of AppWindow singleton
Attachment 282790 [details] pushed as 8d7698d - wizard-source: Drop use of AppWindow singleton
Attachment 282927 [details] pushed as 6463ad2 - machine: Drop use of AppWindow singleton
Attachment 282928 [details] pushed as 6625c9e - libvirt-machine: Drop use of AppWindow singleton
Attachment 282929 [details] pushed as 3517b7a - libvirt-machine-properties: Drop use of AppWindow singleton
Attachment 282930 [details] pushed as 5612a91 - spice-display: Add 'machine' prop
Comment 654 Zeeshan Ali 2014-08-08 16:36:16 UTC
Comment on attachment 282926 [details] [review]
app: Add 'main-window' prop

This was pushed with one small change: The singleton removal is moved into a separate patch that comes after all othe patches since otherwise this patch will break the build.