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

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

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

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

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

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.

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

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.

(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 ;))

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.)

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.

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.

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.

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?

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.

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

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

Created attachment 279879 [details] [review]
wizard: Replace buttons with toolbar

Created attachment 279880 [details] [review]
wizard: Notify failed installation

Created attachment 279881 [details] [review]
wizard: Notify successful installation

Created attachment 279882 [details] [review]
app-window: Edit sidebar's wizard page

Created attachment 279883 [details] [review]
wizard: Bind page to toolbar's page

Created attachment 279884 [details] [review]
vm-creator: Notify failed installation

I started a new series of lighter commits, they should be easier to review.

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.

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?

Review of attachment 279881 [details] [review]:

Same as last patch

Review of attachment 279882 [details] [review]:

I dislike the commit message but I don't have a better one :/ Else: looks good for me.

Review of attachment 279883 [details] [review]:

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

or so.

Review of attachment 279884 [details] [review]:

same as with the other notify patches

Review of attachment 279882 [details] [review]:

Edit -> Set

Some rationale in the logs would be nice. Same goes for other patches.

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?

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.

Review of attachment 279882 [details] [review]:

We'll figure something out.

(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.

(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.

(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 :)

(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.

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.

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.

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.

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.

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.

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.

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.

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?

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?

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.

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?

Review of attachment 280029 [details] [review]:

.

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.

(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?

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.

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.

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.

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.

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.

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.

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.

Created attachment 280108 [details] [review]
searchbar: Add window property

Add a window property to the Searchbar class to untie it from App.window.

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.

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.

Created attachment 280112 [details] [review]
searchbar: Add window property

Add a window property to the Searchbar class to untie it from App.window.

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.

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.

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.

Created attachment 280116 [details] [review]
searchbar: Add window property

Add a window property to the Searchbar class to untie it from App.window.

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.

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.

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.

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.

Review of attachment 280032 [details] [review]:

Looks good.

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 :))

Review of attachment 280038 [details] [review]:

looks good for me

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 ()".

Review of attachment 280057 [details] [review]:

ack from me

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 :)

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?

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?

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.

Review of attachment 280118 [details] [review]:

You have a \t in the log. Looks good apart from that.

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/

Review of attachment 280117 [details] [review]:

looks good

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?

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?

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.

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 ();
}

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.

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.

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) {
//...
}

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

Created attachment 280240 [details] [review]
searchbar: Add window property

Add a window property to the Searchbar class to untie it from App.window.

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.

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.

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.

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.

Review of attachment 280229 [details] [review]:

Assuming patch applies cleanly and compiles. I'll recheck that for all patches in the next iteration. ACK

Review of attachment 280230 [details] [review]:

same here.

Review of attachment 280231 [details] [review]:

I think log could be a bit more verbose.

Review of attachment 280232 [details] [review]:

ack

Review of attachment 280233 [details] [review]:

ack

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.

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.

Review of attachment 280242 [details] [review]:

ack

Review of attachment 280243 [details] [review]:

ack

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.

Review of attachment 280236 [details] [review]:

nice small patch

Review of attachment 280238 [details] [review]:

ok

Review of attachment 280239 [details] [review]:

Can you switch this commit with its parent? So we have title and subtitle patch together.

Review of attachment 280237 [details] [review]:

ack

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

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?

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.)

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.

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.

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?

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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

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

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.

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.

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.

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.

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.

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?

Review of attachment 280323 [details] [review]:

ack from me

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.

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.

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.