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 750784 - Refactor wizard pages into their own classes
Refactor wizard pages into their own classes
Status: RESOLVED OBSOLETE
Product: gnome-boxes
Classification: Applications
Component: wizard
unspecified
Other Linux
: Normal normal
: --
Assigned To: GNOME Boxes maintainer(s)
GNOME Boxes maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2015-06-11 11:02 UTC by Adrien Plazas
Modified: 2018-01-11 10:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add wizard-setup-page (2.97 KB, patch)
2015-06-11 11:21 UTC, Adrien Plazas
needs-work Details | Review
wizard: Use the WizardSetupPage widget (2.47 KB, patch)
2015-06-11 11:21 UTC, Adrien Plazas
needs-work Details | Review
wizard-setup-page: Add setup_for_vm_creator() (1.69 KB, patch)
2015-06-11 11:21 UTC, Adrien Plazas
needs-work Details | Review
wizard: Use WizardSetupPage.setup_for_vm_creator() (1.55 KB, patch)
2015-06-11 11:21 UTC, Adrien Plazas
needs-work Details | Review
wizard: Move the setup page into its own class (5.18 KB, patch)
2015-06-14 08:18 UTC, Adrien Plazas
needs-work Details | Review
wizard-setup-page: Move behaviour out from wizard (3.01 KB, patch)
2015-06-14 08:19 UTC, Adrien Plazas
needs-work Details | Review
wizard-source: Move to WizardSourcePage class (43.25 KB, patch)
2015-06-14 19:13 UTC, Adrien Plazas
none Details | Review
wizard-source-page: Implement WizardSourceInfo (1.93 KB, patch)
2015-06-14 19:13 UTC, Adrien Plazas
none Details | Review
wizard: Move the preparation page to its own class (16.63 KB, patch)
2015-06-14 19:13 UTC, Adrien Plazas
none Details | Review
wizard-prep-page: Move behaviour out from wizard (22.68 KB, patch)
2015-06-14 19:13 UTC, Adrien Plazas
none Details | Review
wizard: Move the setup page into its own class (6.49 KB, patch)
2015-06-15 19:39 UTC, Adrien Plazas
none Details | Review
wizard-source: Move to WizardSourcePage class (43.25 KB, patch)
2015-06-15 19:39 UTC, Adrien Plazas
none Details | Review
wizard-source-page: Implement WizardSourceInfo (1.93 KB, patch)
2015-06-15 19:40 UTC, Adrien Plazas
none Details | Review
wizard: Move the preparation page to its own class (33.81 KB, patch)
2015-06-15 19:40 UTC, Adrien Plazas
none Details | Review
wizard: Move the setup page into its own class (6.54 KB, patch)
2015-06-16 06:34 UTC, Adrien Plazas
needs-work Details | Review
wizard-source: Move to WizardSourcePage class (43.26 KB, patch)
2015-06-16 06:34 UTC, Adrien Plazas
none Details | Review
wizard-source-page: Implement WizardSourceInfo (1.94 KB, patch)
2015-06-16 06:34 UTC, Adrien Plazas
none Details | Review
wizard: Move the preparation page to its own class (33.83 KB, patch)
2015-06-16 06:35 UTC, Adrien Plazas
none Details | Review
wizard-source: Move to WizardSourcePage class (45.05 KB, patch)
2015-06-16 08:00 UTC, Adrien Plazas
needs-work Details | Review
wizard-source-page: Implement WizardSourceInfo (1.94 KB, patch)
2015-06-16 08:00 UTC, Adrien Plazas
none Details | Review
wizard: Move the preparation page to its own class (33.83 KB, patch)
2015-06-16 08:00 UTC, Adrien Plazas
none Details | Review
wizard: Move setup page into own class (6.52 KB, patch)
2015-06-16 13:38 UTC, Adrien Plazas
needs-work Details | Review
wizard-source: Take bounding box from wizard (31.83 KB, patch)
2015-06-16 13:39 UTC, Adrien Plazas
needs-work Details | Review
wizard-source: Move to WizardSourcePage class (13.40 KB, patch)
2015-06-16 13:39 UTC, Adrien Plazas
needs-work Details | Review
wizard-source-page: Implement WizardSourceInfo (2.16 KB, patch)
2015-06-16 13:39 UTC, Adrien Plazas
reviewed Details | Review
wizard: Move preparation page into own class (33.82 KB, patch)
2015-06-16 13:39 UTC, Adrien Plazas
none Details | Review
wizard: Move review page into own class (29.23 KB, patch)
2015-06-16 13:39 UTC, Adrien Plazas
needs-work Details | Review
wizard-source: Take bounding box from wizard (29.18 KB, patch)
2015-06-17 07:02 UTC, Adrien Plazas
none Details | Review
wizard-source: No more access wizard window (3.74 KB, patch)
2015-06-17 07:03 UTC, Adrien Plazas
none Details | Review
Rename WizardSource to WizardSourcePage (13.33 KB, patch)
2015-06-17 07:03 UTC, Adrien Plazas
none Details | Review
wizard-source-page: Implement WizardSourceInfo (2.16 KB, patch)
2015-06-17 07:03 UTC, Adrien Plazas
none Details | Review
wizard: Move preparation page into own class (33.82 KB, patch)
2015-06-17 07:03 UTC, Adrien Plazas
none Details | Review
wizard: Move review page into own class (29.23 KB, patch)
2015-06-17 07:03 UTC, Adrien Plazas
none Details | Review
wizard: Move setup page into own class (6.53 KB, patch)
2015-06-19 11:53 UTC, Adrien Plazas
accepted-commit_now Details | Review
wizard-source: Take bounding box from wizard (29.18 KB, patch)
2015-06-19 11:53 UTC, Adrien Plazas
accepted-commit_now Details | Review
wizard-source: No more access wizard window (3.74 KB, patch)
2015-06-19 11:53 UTC, Adrien Plazas
accepted-commit_after_freeze Details | Review
Rename WizardSource to WizardSourcePage (13.33 KB, patch)
2015-06-19 11:53 UTC, Adrien Plazas
accepted-commit_now Details | Review
wizard-source-page: Implement WizardSourceInfo (2.16 KB, patch)
2015-06-19 11:53 UTC, Adrien Plazas
rejected Details | Review
wizard: Move preparation page into own class (33.82 KB, patch)
2015-06-19 11:53 UTC, Adrien Plazas
needs-work Details | Review
wizard: Move review page into own class (29.23 KB, patch)
2015-06-19 11:53 UTC, Adrien Plazas
needs-work Details | Review
wizard: Move setup page into own class (6.53 KB, patch)
2015-06-22 11:55 UTC, Adrien Plazas
accepted-commit_now Details | Review
wizard-source: Take bounding box from Wizard (29.19 KB, patch)
2015-06-22 11:55 UTC, Adrien Plazas
accepted-commit_now Details | Review
wizard-source: Don't access wizard window (3.74 KB, patch)
2015-06-22 11:55 UTC, Adrien Plazas
needs-work Details | Review
Rename WizardSource to WizardSourcePage (13.34 KB, patch)
2015-06-22 11:55 UTC, Adrien Plazas
accepted-commit_now Details | Review
wizard: Move preparation page into own class (34.06 KB, patch)
2015-06-22 11:55 UTC, Adrien Plazas
needs-work Details | Review
wizard-summary: Make self contained (14.80 KB, patch)
2015-06-22 11:55 UTC, Adrien Plazas
needs-work Details | Review
wizard: Move review page into own class (18.97 KB, patch)
2015-06-22 11:55 UTC, Adrien Plazas
needs-work Details | Review
wizard-review: Don't access any window (3.30 KB, patch)
2015-06-22 11:56 UTC, Adrien Plazas
accepted-commit_now Details | Review
Move WizardSummary to WizardReviewPage's file (13.83 KB, patch)
2015-06-22 11:56 UTC, Adrien Plazas
rejected Details | Review

Description Adrien Plazas 2015-06-11 11:02:16 UTC
The wizard's pages should be refactored into individual classes. That would make the wizard's code more simple and make its transition to GtkAssistant easier.
Comment 1 Adrien Plazas 2015-06-11 11:21:02 UTC
Created attachment 305064 [details] [review]
Add wizard-setup-page

This adds the WizardSetupPage widget.

It is part of the wizard refactoring process.
Comment 2 Adrien Plazas 2015-06-11 11:21:07 UTC
Created attachment 305065 [details] [review]
wizard: Use the WizardSetupPage widget

This makes the wizard use the WizardSetupPage widget.

It is part of the wizard refactoring process.
Comment 3 Adrien Plazas 2015-06-11 11:21:12 UTC
Created attachment 305066 [details] [review]
wizard-setup-page: Add setup_for_vm_creator()

This adds the setup_for_vm_creator() method to WizardSetupPage. It
allows to set up the VM creator.

It is part of the wizard refactoring process.
Comment 4 Adrien Plazas 2015-06-11 11:21:18 UTC
Created attachment 305067 [details] [review]
wizard: Use WizardSetupPage.setup_for_vm_creator()

This makes the wizard use the setup_for_vm_creator() method of its setup
page.

It is part of the wizard refactoring process.
Comment 5 Zeeshan Ali 2015-06-12 11:10:57 UTC
Review of attachment 305064 [details] [review]:

Looks fine and simple but some nitpicks on log:

* I always mention class name rather than the module name unless it's used as prefix (i-e module-name: Changes).

* I'd rather you describe more specifically why this change is needed, "refactoring" is too vague.
Comment 6 Zeeshan Ali 2015-06-12 11:12:41 UTC
Review of attachment 305064 [details] [review]:

Also in the description, instead of repeating the shortlog with different words, please describe what this class is.
Comment 7 Zeeshan Ali 2015-06-12 11:14:15 UTC
Review of attachment 305065 [details] [review]:

This and the previous patch can/should actually be squashed since its not about addition of new code and then making use of it but rather moving of code from one class to another new one.
Comment 8 Zeeshan Ali 2015-06-12 11:20:05 UTC
Review of attachment 305066 [details] [review]:

we'll need a bit better explanation/justification in log for me to say anything about this change. :)

::: src/wizard-setup-page.vala
@@ +14,3 @@
+
+        // mimick a property binding with sync at creation,
+        // allowing to set the private "complete" property

What? Is it a bug?
Comment 9 Zeeshan Ali 2015-06-12 11:29:02 UTC
Review of attachment 305067 [details] [review]:

Same here, need a bit better explanation/justification. I think the last two patches should also be squashed into the first one or if you keep the first two separate then perhaps the previous patch should be merged into the first one and this one into the second patch.
Comment 10 Adrien Plazas 2015-06-14 08:00:00 UTC
(In reply to Zeeshan Ali (Khattak) from comment #8)
> ::: src/wizard-setup-page.vala
> @@ +14,3 @@
> +
> +        // mimick a property binding with sync at creation,
> +        // allowing to set the private "complete" property
> 
> What? Is it a bug?

No it's not a bug but a workaround the expected behaviour.
The completeness of a page should be stated by the page itself from the informations it hold, hence the setter of the "complete" property is private.

In order to bind the "complete" property to the "vm_creator.install_media.ready_to_create" one, the Binding object have to monitor the latter and to set the former. But it can't set it as it haves to be private.

Hence you have to mimick the behaviour of the binding object directly into class possessing the destination property in order to be able to set this property.

So, not a bug, just a workaround a regular behaviour. =)
Comment 11 Adrien Plazas 2015-06-14 08:18:59 UTC
Created attachment 305224 [details] [review]
wizard: Move the setup page into its own class

This moves the wizard's setup page out of it and to the new
WizardSetupPage class.

This change is needed to make the wizard simpler and to ease its port to
GtkAssistant.
Comment 12 Adrien Plazas 2015-06-14 08:19:04 UTC
Created attachment 305225 [details] [review]
wizard-setup-page: Move behaviour out from wizard

This adds the setup_for_vm_creator() method, the complete prop and the
creation_requested signal to WizardSetupPage, allowing it to set up the VM creator and to notify its state.
This also makes the wizard use these changes instead of modifying the
page's state directly.

This change is needed to make the wizard simpler and to ease its port to
GtkAssistant.
Comment 13 Adrien Plazas 2015-06-14 19:13:37 UTC
Created attachment 305252 [details] [review]
wizard-source: Move to WizardSourcePage class

This renames the WizardSource class as WizardSourcePage and moves the
box containing it out of the wizard and into it.

This change is needed to keep the wizard's pages' name consistent and to
make the wizard simpler and to ease its port to GtkAssistant.
Comment 14 Adrien Plazas 2015-06-14 19:13:43 UTC
Created attachment 305253 [details] [review]
wizard-source-page: Implement WizardSourceInfo

This add the WizardSourceInfo interface and makes WizardSourcePage
implement it.

This change is needed to make the wizard's preparation page manipulate
only the model of the source page and not the page itself, as only the
wizard should know about the pages in order to keep it sane.
Comment 15 Adrien Plazas 2015-06-14 19:13:48 UTC
Created attachment 305254 [details] [review]
wizard: Move the preparation page to its own class

This moves the wizard's preparation page out of it and to the new
WizardPreparationPage class.

This change is needed to make the wizard simpler and to ease its port to
GtkAssistant.
Comment 16 Adrien Plazas 2015-06-14 19:13:54 UTC
Created attachment 305255 [details] [review]
wizard-prep-page: Move behaviour out from wizard

This moves the methods corresponding to the preparation page's bahaviour
to WizardPreparationPage, allowing it to prepare the source and to
notify its state. This also makes the wizard use these changes instead
of modifying the page's state directly.

This change is needed to make the wizard simpler and to ease its port to
GtkAssistant.
Comment 17 Zeeshan Ali 2015-06-15 17:53:45 UTC
(In reply to Adrien Plazas from comment #10)
> (In reply to Zeeshan Ali (Khattak) from comment #8)
> > ::: src/wizard-setup-page.vala
> > @@ +14,3 @@
> > +
> > +        // mimick a property binding with sync at creation,
> > +        // allowing to set the private "complete" property
> > 
> > What? Is it a bug?
> 
> No it's not a bug but a workaround the expected behaviour.
> The completeness of a page should be stated by the page itself from the
> informations it hold, hence the setter of the "complete" property is private.
> 
> In order to bind the "complete" property to the
> "vm_creator.install_media.ready_to_create" one, the Binding object have to
> monitor the latter and to set the former. But it can't set it as it haves to
> be private.
> 
> Hence you have to mimick the behaviour of the binding object directly into
> class possessing the destination property in order to be able to set this
> property.
> 
> So, not a bug, just a workaround a regular behaviour. =)

* Ah ok. I think the comment could be improved a bit then. For starters, it says "complete" is private property, while its public with private setter.
* You want to get back to the habbit of hitting 'Review' link to reply to inline comments. :)
Comment 18 Zeeshan Ali 2015-06-15 17:59:50 UTC
Review of attachment 305224 [details] [review]:

Looks good but needs to be squashed with following patch.
Comment 19 Adrien Plazas 2015-06-15 19:39:48 UTC
Created attachment 305336 [details] [review]
wizard: Move the setup page into its own class

This moves the wizard's setup page out of it and to the new
WizardSetupPage class. This also makes the wizard use this new class
instead of modifying the page's state directly.

This change is needed to make the wizard simpler and to ease its port to
GtkAssistant.
Comment 20 Adrien Plazas 2015-06-15 19:39:58 UTC
Created attachment 305337 [details] [review]
wizard-source: Move to WizardSourcePage class

This renames the WizardSource class as WizardSourcePage and moves the
box containing it out of the wizard and into it.

This change is needed to keep the wizard's pages' name consistent and to
make the wizard simpler and to ease its port to GtkAssistant.
Comment 21 Adrien Plazas 2015-06-15 19:40:07 UTC
Created attachment 305338 [details] [review]
wizard-source-page: Implement WizardSourceInfo

This add the WizardSourceInfo interface and makes WizardSourcePage
implement it.

This change is needed to make the wizard's preparation page manipulate
only the model of the source page and not the page itself, as only the
wizard should know about the pages in order to keep it sane.
Comment 22 Adrien Plazas 2015-06-15 19:40:16 UTC
Created attachment 305339 [details] [review]
wizard: Move the preparation page to its own class

This moves the wizard's preparation page out of it and to the new
WizardPreparationPage class. This also makes the wizard use this new
class instead of modifying the page's state directly.

This change is needed to make the wizard simpler and to ease its port to
GtkAssistant.
Comment 23 Zeeshan Ali 2015-06-15 21:24:47 UTC
Review of attachment 305225 [details] [review]:

* 'behaviour' is way too vague to mean anything.
* When there is multiple classes/modules involved, its better to prefix methods, properties and signals with class (and/or namespace in some cases), e.g WizardSetupPage.setup_vm_creator(). Otherwise, you'd want to surround property and signal names in "" or '' (I first wondered what is a complete property).
* Please always keep an empty line above paragraphs in commit log since its supposed to be mostly normal English. :)
* Second line in description goes way beyond 74 characters.
* Didn't notice in previous patch review. 'make the wizard simpler' -> 'make the Wizard class simpler'.

::: src/wizard-setup-page.vala
@@ +14,3 @@
+
+        // mimick a property binding with sync at creation,
+        // allowing to set the private "complete" property

See my reply to your reply from previous review about this.
Comment 24 Adrien Plazas 2015-06-16 06:34:33 UTC
Created attachment 305361 [details] [review]
wizard: Move the setup page into its own class

This moves the wizard's setup page out of the Wizard class it and to the
new WizardSetupPage class. This also makes the Wizard class use this new
class instead of modifying the page's state directly.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 25 Adrien Plazas 2015-06-16 06:34:44 UTC
Created attachment 305362 [details] [review]
wizard-source: Move to WizardSourcePage class

This renames the WizardSource class as WizardSourcePage and moves the
box containing it out of the Wizard class and into it.

This change is needed to keep the wizard's pages' name consistent, to
make the Wizard class simpler and to ease its port to GtkAssistant.
Comment 26 Adrien Plazas 2015-06-16 06:34:52 UTC
Created attachment 305363 [details] [review]
wizard-source-page: Implement WizardSourceInfo

This add the WizardSourceInfo interface and makes WizardSourcePage
implement it.

This change is needed to make the wizard's preparation page manipulate
only the model of the source page and not the page itself, as only the
Wizard class should know about the pages in order to keep itself sane.
Comment 27 Adrien Plazas 2015-06-16 06:35:02 UTC
Created attachment 305364 [details] [review]
wizard: Move the preparation page to its own class

This moves the wizard's preparation page out of the Wizard class and to
the new WizardPreparationPage class. This also makes the Wizard class
use this new class instead of modifying the page's state directly.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 28 Adrien Plazas 2015-06-16 08:00:32 UTC
Created attachment 305366 [details] [review]
wizard-source: Move to WizardSourcePage class

This renames the WizardSource class as WizardSourcePage and moves the
box containing it out of the Wizard class and into it.

This change is needed to keep the wizard's pages' name consistent, to
make the Wizard class simpler and to ease its port to GtkAssistant.
Comment 29 Adrien Plazas 2015-06-16 08:00:38 UTC
Created attachment 305367 [details] [review]
wizard-source-page: Implement WizardSourceInfo

This add the WizardSourceInfo interface and makes WizardSourcePage
implement it.

This change is needed to make the wizard's preparation page manipulate
only the model of the source page and not the page itself, as only the
Wizard class should know about the pages in order to keep itself sane.
Comment 30 Adrien Plazas 2015-06-16 08:00:44 UTC
Created attachment 305368 [details] [review]
wizard: Move the preparation page to its own class

This moves the wizard's preparation page out of the Wizard class and to
the new WizardPreparationPage class. This also makes the Wizard class
use this new class instead of modifying the page's state directly.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 31 Lasse Schuirmann 2015-06-16 09:09:43 UTC
Review of attachment 305361 [details] [review]:

Shortlog could be shorter IMO. I'd prefer `wizard: Move setup page to own class` - faster to read.

"This moves the wizard's setup page out of the Wizard class it and to the
new WizardSetupPage class." That sentence doesn't make so much sense, did you mean s/it and to/and into/ ?

Grammar: You're beginning three sentences with "This", so for a truly beautiful commit message you'd be more creative ;) E.g. I like "This moves ... . It also makes ... .\n\nThis change is needed..." better.

::: src/wizard-setup-page.vala
@@ +20,3 @@
+        );
+
+        vm_creator.install_media.populate_setup_box (box);

I've been thinking about object oriented design a bit more lately and I think for a truly good design we'd need to rethink this a bit.

The WizardSetupPage  shouldn't hold any specifics to a VMCreator. It should be a setup page. Just that. It should show that page to the user, right. It should forward user actions in an abstract manner, Continue, Cancel, Back. It should not hold any behaviour relevant things. It should certainly not hold assumptions over other parts of the program. Connecting signals should be done from the outside to keep the object generic.

I'm not the one to change Boxes' design, I'm just pointing out what I think could be done better - and what better place for that is there than in a refactoring commit review :)
Comment 32 Lasse Schuirmann 2015-06-16 11:33:50 UTC
Review of attachment 305366 [details] [review]:

hey, could you do the file renaming and the changes each in one commit? This is way easier to review and thus less buggy in the end.
Comment 33 Adrien Plazas 2015-06-16 12:08:18 UTC
Review of attachment 305366 [details] [review]:

Sure, I'll do it.
Comment 34 Adrien Plazas 2015-06-16 12:08:21 UTC
Review of attachment 305366 [details] [review]:

Sure, I'll do it.
Comment 35 Adrien Plazas 2015-06-16 13:38:55 UTC
Created attachment 305392 [details] [review]
wizard: Move setup page into own class

This moves the wizard's setup page out of the Wizard class and to the
new WizardSetupPage class. It also makes the Wizard class use this new
class instead of modifying the page's state directly.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 36 Adrien Plazas 2015-06-16 13:39:04 UTC
Created attachment 305393 [details] [review]
wizard-source: Take bounding box from wizard

This moves the box containing the source page out of the Wizard class
and into it. It also makes the WizardPage more self-contained by adding
the file_chooser_requested() signal and the on_file_choosen() method to
remove the need of accessing the wizard's window.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 37 Adrien Plazas 2015-06-16 13:39:16 UTC
Created attachment 305394 [details] [review]
wizard-source: Move to WizardSourcePage class

This renames the WizardSource class as WizardSourcePage.

This change is needed to keep the wizard's pages' name consistent.
Comment 38 Adrien Plazas 2015-06-16 13:39:28 UTC
Created attachment 305395 [details] [review]
wizard-source-page: Implement WizardSourceInfo

This adds the WizardSourceInfo interface and makes WizardSourcePage
implement it.

This change is needed to make the wizard's preparation page manipulate
only the model of the source page and not the page itself, as only the
Wizard class should know about the pages in order to keep itself sane.
Comment 39 Adrien Plazas 2015-06-16 13:39:41 UTC
Created attachment 305396 [details] [review]
wizard: Move preparation page into own class

This moves the wizard's preparation page out of the Wizard class and to
the new WizardPreparationPage class. It also makes the Wizard class use
this new class instead of modifying the page's state directly.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 40 Adrien Plazas 2015-06-16 13:39:54 UTC
Created attachment 305397 [details] [review]
wizard: Move review page into own class

This moves the wizard's preparation page out of the Wizard class and to
the new WizardReviewPage class. It also makes the Wizard class use this
new class instead of modifying the page's state directly. This also
moves the WizardSummary class to the same file as the WizardReviewPage
class.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 41 Lasse Schuirmann 2015-06-16 19:37:12 UTC
Review of attachment 305392 [details] [review]:

::: src/wizard-setup-page.vala
@@ +18,3 @@
+        vm_creator.install_media.notify["ready-to-create"].connect (
+            () => complete = vm_creator.install_media.ready_to_create
+        );

so after staring at it a while I finally understood it. (I'm a tiny little big bit pythonic in thinking by now...) It looks quite hacky... you sure there is nothing easier?
Comment 42 Lasse Schuirmann 2015-06-16 19:41:44 UTC
Review of attachment 305393 [details] [review]:

"This moves the box containing the source page out of the Wizard class
and into it. It also makes the WizardPage more self-contained by adding
the file_chooser_requested() signal and the on_file_choosen() method to
remove the need of accessing the wizard's window."

Solely judging by the commit message that sounds like two commits.

"This moves the box containing the source page out of the Wizard class
and into it." - first one
"It also makes the WizardPage more self-contained by adding
the file_chooser_requested() signal and the on_file_choosen() method to
remove the need of accessing the wizard's window." - second one

If that doesn't yield true commit message needs to be refactored :)
Comment 43 Lasse Schuirmann 2015-06-16 19:44:10 UTC
Review of attachment 305394 [details] [review]:

shortlog: I'd rather do `Rename WizardSource to WizardSourcePage` and then leave out first sentence in long desccription. That should still comply to the commit message guidelines as the tag is optional.

Looks good apart.
Comment 44 Lasse Schuirmann 2015-06-16 19:49:52 UTC
Review of attachment 305396 [details] [review]:

on first sight looks good, I really love this change btw, but I don't have time for a thorough review right now
Comment 45 Lasse Schuirmann 2015-06-16 19:50:48 UTC
Review of attachment 305397 [details] [review]:

"This moves the wizard's preparation page out of the Wizard class and to
the new WizardReviewPage class."
I assume you mean "wizard's review page"

Same as last commit.
Comment 46 Lasse Schuirmann 2015-06-16 19:51:42 UTC
Review of attachment 305395 [details] [review]:

lgtm
Comment 47 Adrien Plazas 2015-06-17 07:02:57 UTC
Created attachment 305454 [details] [review]
wizard-source: Take bounding box from wizard

This moves the box containing the source page out of the Wizard class
and into it.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 48 Adrien Plazas 2015-06-17 07:03:04 UTC
Created attachment 305455 [details] [review]
wizard-source: No more access wizard window

This makes the WizardPage more self-contained by adding the
file_chooser_requested() signal and the on_file_choosen() method to
remove the need of accessing the wizard's window.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 49 Adrien Plazas 2015-06-17 07:03:10 UTC
Created attachment 305456 [details] [review]
Rename WizardSource to WizardSourcePage

This change is needed to keep the wizard's pages' name consistent.
Comment 50 Adrien Plazas 2015-06-17 07:03:16 UTC
Created attachment 305457 [details] [review]
wizard-source-page: Implement WizardSourceInfo

This adds the WizardSourceInfo interface and makes WizardSourcePage
implement it.

This change is needed to make the wizard's preparation page manipulate
only the model of the source page and not the page itself, as only the
Wizard class should know about the pages in order to keep itself sane.
Comment 51 Adrien Plazas 2015-06-17 07:03:23 UTC
Created attachment 305458 [details] [review]
wizard: Move preparation page into own class

This moves the wizard's preparation page out of the Wizard class and to
the new WizardPreparationPage class. It also makes the Wizard class use
this new class instead of modifying the page's state directly.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 52 Adrien Plazas 2015-06-17 07:03:30 UTC
Created attachment 305459 [details] [review]
wizard: Move review page into own class

This moves the wizard's review page out of the Wizard class and to the
new WizardReviewPage class. It also makes the Wizard class use this new
class instead of modifying the page's state directly. This also moves
the WizardSummary class to the same file as the WizardReviewPage class.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 53 Zeeshan Ali 2015-06-18 17:25:04 UTC
Review of attachment 305392 [details] [review]:

Looks good apart from these nits.

::: src/wizard-setup-page.vala
@@ +13,3 @@
+        return_if_fail (vm_creator != null);
+
+        // mimick a property binding with sync at creation, allowing to

Please always start English phrases and sentences with capital letter.

@@ +14,3 @@
+
+        // mimick a property binding with sync at creation, allowing to
+        // set the "complete" property as its setter is private

* Would be more clear with: as -> since.
* its -> it's
* . at the end of sentences please. :)
* lines are 120 chars (not 80) btw.

@@ +18,3 @@
+        vm_creator.install_media.notify["ready-to-create"].connect (
+            () => complete = vm_creator.install_media.ready_to_create
+        );

* Kekun? Doesn't seem too hack to me through.
* Let's follow the coding style:

vm_creator.install_media.notify["ready-to-create"].connect (() => {
    complete = vm_creator.install_media.ready_to_create;
});
Comment 54 Adrien Plazas 2015-06-18 17:54:25 UTC
Review of attachment 305392 [details] [review]:

Sorry, I forgot that you have to go back to the overview to actually publish a review... I need to grow customed to Bugzilla's weird UI again.

::: src/wizard-setup-page.vala
@@ +18,3 @@
+        vm_creator.install_media.notify["ready-to-create"].connect (
+            () => complete = vm_creator.install_media.ready_to_create
+        );

Well, Binding objects can't set a property if they can't access this property's setter, and the "complete" property's setter should be private. A Binding object with the SYNC_CREATE flag basically just does that: setting the destination prop's value to the source's one, and setting it again at each value change.

Honestly I don't see a better way of keeping the exact same behaviour while restraining the object's boundings.
Comment 55 Adrien Plazas 2015-06-18 17:54:30 UTC
Review of attachment 305392 [details] [review]:

Sorry, I forgot that you have to go back to the overview to actually publish a review... I need to grow customed to Bugzilla's weird UI again.

::: src/wizard-setup-page.vala
@@ +18,3 @@
+        vm_creator.install_media.notify["ready-to-create"].connect (
+            () => complete = vm_creator.install_media.ready_to_create
+        );

Well, Binding objects can't set a property if they can't access this property's setter, and the "complete" property's setter should be private. A Binding object with the SYNC_CREATE flag basically just does that: setting the destination prop's value to the source's one, and setting it again at each value change.

Honestly I don't see a better way of keeping the exact same behaviour while restraining the object's boundings.
Comment 56 Zeeshan Ali 2015-06-18 20:41:59 UTC
Review of attachment 305392 [details] [review]:

::: src/wizard-setup-page.vala
@@ +14,3 @@
+
+        // mimick a property binding with sync at creation, allowing to
+        // set the "complete" property as its setter is private

Actually "its" is correct in this context.

@@ +18,3 @@
+        vm_creator.install_media.notify["ready-to-create"].connect (
+            () => complete = vm_creator.install_media.ready_to_create
+        );

Yeah, i can't think of anything better either. However, have you ensured that Object.bind_property() doesn't work cause of property being private? IIRC the private part is not translated to C-level and thats where Object.bind_property() is implemented.
Comment 57 Adrien Plazas 2015-06-18 21:29:58 UTC
Review of attachment 305392 [details] [review]:

::: src/wizard-setup-page.vala
@@ +18,3 @@
+        vm_creator.install_media.notify["ready-to-create"].connect (
+            () => complete = vm_creator.install_media.ready_to_create
+        );

That's what I did first and it didn't work: the page was never set ass "complete" and hence the "next" button stayed unclickable. After binding the properties by hand it worked just as before.
Comment 58 Zeeshan Ali 2015-06-18 21:38:34 UTC
Review of attachment 305392 [details] [review]:

::: src/wizard-setup-page.vala
@@ +18,3 @@
+        vm_creator.install_media.notify["ready-to-create"].connect (
+            () => complete = vm_creator.install_media.ready_to_create
+        );

Ah ok. Looking at the generated C code, the property isn't even registered as writable. This small hack if fine then. :)
Comment 59 Adrien Plazas 2015-06-19 11:53:11 UTC
Created attachment 305686 [details] [review]
wizard: Move setup page into own class

This moves the wizard's setup page out of the Wizard class and to the
new WizardSetupPage class. It also makes the Wizard class use this new
class instead of modifying the page's state directly.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 60 Adrien Plazas 2015-06-19 11:53:17 UTC
Created attachment 305687 [details] [review]
wizard-source: Take bounding box from wizard

This moves the box containing the source page out of the Wizard class
and into it.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 61 Adrien Plazas 2015-06-19 11:53:23 UTC
Created attachment 305688 [details] [review]
wizard-source: No more access wizard window

This makes the WizardPage more self-contained by adding the
file_chooser_requested() signal and the on_file_choosen() method to
remove the need of accessing the wizard's window.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 62 Adrien Plazas 2015-06-19 11:53:30 UTC
Created attachment 305689 [details] [review]
Rename WizardSource to WizardSourcePage

This change is needed to keep the wizard's pages' name consistent.
Comment 63 Adrien Plazas 2015-06-19 11:53:35 UTC
Created attachment 305690 [details] [review]
wizard-source-page: Implement WizardSourceInfo

This adds the WizardSourceInfo interface and makes WizardSourcePage
implement it.

This change is needed to make the wizard's preparation page manipulate
only the model of the source page and not the page itself, as only the
Wizard class should know about the pages in order to keep itself sane.
Comment 64 Adrien Plazas 2015-06-19 11:53:42 UTC
Created attachment 305691 [details] [review]
wizard: Move preparation page into own class

This moves the wizard's preparation page out of the Wizard class and to
the new WizardPreparationPage class. It also makes the Wizard class use
this new class instead of modifying the page's state directly.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 65 Adrien Plazas 2015-06-19 11:53:49 UTC
Created attachment 305692 [details] [review]
wizard: Move review page into own class

This moves the wizard's review page out of the Wizard class and to the
new WizardReviewPage class. It also makes the Wizard class use this new
class instead of modifying the page's state directly. This also moves
the WizardSummary class to the same file as the WizardReviewPage class.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 66 Zeeshan Ali 2015-06-19 16:20:55 UTC
Review of attachment 305686 [details] [review]:

ACK
Comment 67 Zeeshan Ali 2015-06-19 16:30:04 UTC
Review of attachment 305687 [details] [review]:

"wizard" -> "Wizard"
"source page" -> WizardSource
"into it" - "into WizardSource
Comment 68 Zeeshan Ali 2015-06-19 16:36:42 UTC
Review of attachment 305688 [details] [review]:

"no more" -> "Don't"

::: src/wizard-source.vala
@@ +297,1 @@
+    public void on_file_choosen (string uri) {

the name makes it sound like its a callback, which it shouldn't be from the POV of this class itself. How about select_file() ?
Comment 69 Zeeshan Ali 2015-06-19 16:38:11 UTC
Review of attachment 305689 [details] [review]:

ack
Comment 70 Zeeshan Ali 2015-06-19 16:55:52 UTC
Review of attachment 305690 [details] [review]:

With this approach, we'll want an interface for each class. So unless the same interface will be later implemented by other classes, i don't think this abstraction is worth it.
Comment 71 Zeeshan Ali 2015-06-19 17:04:06 UTC
Review of attachment 305691 [details] [review]:

Is this patch only moving code into new class or there is new changes in the new class as well?

::: src/wizard-preparation-page.vala
@@ +21,3 @@
+    private MediaManager media_manager;
+
+    // these attributes are mirrors of the ones in the wizard

Capitalize first charachter please

@@ +170,3 @@
+            spice_validate_uri (uri_as_text);
+        } else if (uri.scheme == "vnc") {
+            // accept any vnc:// uri

Capitalize

@@ +172,3 @@
+            // accept any vnc:// uri
+        } else if (uri.scheme.has_prefix ("qemu")) {
+            // accept any qemu..:// uri

and here
Comment 72 Zeeshan Ali 2015-06-19 17:12:18 UTC
Review of attachment 305692 [details] [review]:

Same general question about this patch as previous one: Any new changes under the carpet? :)

::: src/wizard-review-page.vala
@@ +2,3 @@
+
+[GtkTemplate (ui = "/org/gnome/Boxes/ui/wizard-summary.ui")]
+private class Boxes.WizardSummary: Gtk.Grid {

Primary class of this module should be defined first.

@@ +40,3 @@
+
+            if (libvirt_machine == null) {
+                // notify the VM creation failed

C A P I T A L I Z E :)

@@ +117,3 @@
+
+            nokvm = (libvirt_machine.domain_config.get_virt_type () != GVirConfig.DomainVirtType.KVM);
+            // FIXME

in what way should i fix you? :)

@@ +150,3 @@
+
+    private void append_customize_button (CustomizeFunc customize_func) {
+        // there is nothing to customize if review page is empty

Guess what :)

@@ +173,3 @@
+        nokvm = false;
+        text = "";
+//        machine = null;

???

@@ +215,3 @@
+
+    public async bool review (WizardSourceInfo source_info, CollectionSource? source, VMCreator? vm_creator) {
+        // only one outstanding review () permitted

here too and many comments below.
Comment 73 Adrien Plazas 2015-06-19 17:37:41 UTC
Review of attachment 305690 [details] [review]:

OK, let's change this later when refactoring the pages themselves then.
Comment 74 Zeeshan Ali 2015-06-19 19:00:26 UTC
Review of attachment 305690 [details] [review]:

Change in what way? Just to be clear, I don't like the idea of having an interface just for one class.
Comment 75 Adrien Plazas 2015-06-19 19:30:18 UTC
What I mean is that it may be a good thing to separate the pages from the actual data they process. It's more or less done but the fact that a page have to know another one just to access some data points to a design problem.

Basically, the widgets should be views and controllers but not the model. Adding an interface to mimick this split of the model clearly was a very poor move and you are right to say that it shouldn't be done there.

Still, I think that the pages should be reengineered a bit to make this split properly once the wizard is refactored as a GtkAssistant.
Comment 76 Adrien Plazas 2015-06-22 11:55:15 UTC
Created attachment 305820 [details] [review]
wizard: Move setup page into own class

This moves the wizard's setup page out of the Wizard class and to the
new WizardSetupPage class. It also makes the Wizard class use this new
class instead of modifying the page's state directly.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 77 Adrien Plazas 2015-06-22 11:55:22 UTC
Created attachment 305821 [details] [review]
wizard-source: Take bounding box from Wizard

This moves the box containing the WizardSource out of the Wizard class
and into WizardSource.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 78 Adrien Plazas 2015-06-22 11:55:28 UTC
Created attachment 305822 [details] [review]
wizard-source: Don't access wizard window

This makes the WizardSource class more self-contained by adding the
file_chooser_requested() signal and the select_file() method to remove
the need of accessing the wizard's window.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 79 Adrien Plazas 2015-06-22 11:55:34 UTC
Created attachment 305823 [details] [review]
Rename WizardSource to WizardSourcePage

This change is needed to keep the wizard's pages' name consistent.
Comment 80 Adrien Plazas 2015-06-22 11:55:41 UTC
Created attachment 305824 [details] [review]
wizard: Move preparation page into own class

This moves the wizard's preparation page out of the Wizard class and to
the new WizardPreparationPage class. It adds to this new class the
"complete" prop and preparation_failed() signal to notify the Wizard
class of the page's state and it add the error_occured() signal to don't
have to access the wizard's window.

This also makes the Wizard class use the WizardPreparationPage instead
of modifying the page's state directly.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 81 Adrien Plazas 2015-06-22 11:55:48 UTC
Created attachment 305825 [details] [review]
wizard-summary: Make self contained

This moves the Wizard.do_review_cancellable() method to
WizardSummary.sum_up_cancellable(), add the failed() and
customize_libvirt_machine() signals to the WizardSummary class and make
all of its other methods private to make it self contained.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 82 Adrien Plazas 2015-06-22 11:55:54 UTC
Created attachment 305826 [details] [review]
wizard: Move review page into own class

This moves the wizard's review page out of the Wizard class and to the
new WizardReviewPage class. It also makes the Wizard class use this new
class instead of modifying the page's state directly.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 83 Adrien Plazas 2015-06-22 11:56:01 UTC
Created attachment 305827 [details] [review]
wizard-review: Don't access any window

This makes the WizardReviewPage class more self-contained by adding the
failed() and customize_libvirt_machine() signals to remove the need of
accessing the wizard's window and the window.

This change is needed to make the Wizard class simpler and to ease its
port to GtkAssistant.
Comment 84 Adrien Plazas 2015-06-22 11:56:07 UTC
Created attachment 305828 [details] [review]
Move WizardSummary to WizardReviewPage's file

Move the WizardSummary class from the same file as the Wizard class to
the same file as the WizardReviewPage class.

This change is needed to keep the code composing the WizardReviewPage in
the same file.
Comment 85 Zeeshan Ali 2015-06-23 14:51:59 UTC
(In reply to Adrien Plazas from comment #75)
> What I mean is that it may be a good thing to separate the pages from the
> actual data they process. It's more or less done but the fact that a page
> have to know another one just to access some data points to a design problem.

Sure but adding an interface specific to the target page and then accessing the page through that page-specific interface isn't solving any problems. This IMO would be over-engineering.

> Basically, the widgets should be views and controllers but not the model.
> Adding an interface to mimick this split of the model clearly was a very
> poor move and you are right to say that it shouldn't be done there.

Glad we agree. :)

> Still, I think that the pages should be reengineered a bit to make this
> split properly once the wizard is refactored as a GtkAssistant.

Sure.
Comment 86 Zeeshan Ali 2015-06-23 16:33:14 UTC
Review of attachment 305820 [details] [review]:

If its the same as last version, still ACK. Please note that you don't have to (and I'd prefer that) re-upload unchanged patches if they are at the beginning of the patch series.
Comment 87 Zeeshan Ali 2015-06-23 16:35:29 UTC
Review of attachment 305821 [details] [review]:

ack
Comment 88 Zeeshan Ali 2015-06-23 17:25:39 UTC
Review of attachment 305822 [details] [review]:

Looks good apart from this nitpicks

::: src/wizard-source.vala
@@ +165,3 @@
     }
 
+    public signal void file_chooser_requested ();

I think the usual convention is to declare signals above (public) properties.

::: src/wizard.vala
@@ +196,3 @@
         });
 
+        wizard_source.file_chooser_requested.connect(() => {

Space before (
Comment 89 Zeeshan Ali 2015-06-23 17:27:27 UTC
Review of attachment 305823 [details] [review]:

ACK, assuming nothing changed since last review.
Comment 90 Zeeshan Ali 2015-06-23 17:53:59 UTC
Review of attachment 305824 [details] [review]:

Looks good otherwise.

::: data/ui/wizard-preparation-page.ui
@@ +1,3 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<interface>
+  <template class="BoxesWizardPreparationPage" parent="GtkBox">

I'd just name it WizardPrepPage. Shorter but still obvious.

::: src/wizard-preparation-page.vala
@@ +21,3 @@
+    private MediaManager media_manager;
+
+    // These attributes are mirrors of the ones in the wizard

Huh? Not really. This class creates them itself below.

@@ +25,3 @@
+    public VMCreator? vm_creator { get; private set; }
+
+    public signal void error_occured (string msg);

'failed' would be a good name.

@@ +26,3 @@
+
+    public signal void error_occured (string msg);
+    public signal void preparation_failed ();

Here is the order I've been trying to follow:

1. constants
2. signals
3. props/fields (public first)
4. public static methods
5. public methods
6. private static methods
7. private methods

@@ +39,3 @@
+    }
+
+    public bool prepare_source (Boxes.WizardSourcePage source_page) {

* I feel like these methods should return the created CollectionSource. Don't you? It could be null in case of probing and this could be async in case of needing to wait for an asyn activity.

* I don't think these two methods should be taking source_page but rather caller should explicitly set this property before calling these methods and these methods should have a return_if_fail (source_page != null); Otherwise caller doesn't know that these methods have side-effects.

Alternatively, source_page could just be passed around so that these methods don't have unexpected side-affects..

::: src/wizard.vala
@@ -429,3 @@
-        return true;
-    }
-

Cool! This hunk alone makes me really happy. :)
Comment 91 Zeeshan Ali 2015-06-23 18:11:52 UTC
Review of attachment 305825 [details] [review]:

* "Make" -> "Make it" but you should change the whole shortlog. Remember to summarize the change itself and is summary turns out very high-level/vague, it probably means you want to divide the patch a bit.
* Better make use of bullet points when summarizing multiple changes.
* "add the" -> "adds the"
* I'd at least put changing visibility of methods into another patch.

::: src/wizard.vala
@@ +214,3 @@
         return_if_fail (review_cancellable == null);
 
+        // Show the nokvm infobar if requested by the summary

This comment is more relevant 3 lines below (inside the nokvm notify handler).

@@ +220,3 @@
+            nokvm_infobar.visible = summary.nokvm;
+        });
+        // Set the review label as requested by the summary

Same here.

@@ +226,3 @@
+            review_label.set_text (summary.text);
+        });
+        // Bind the machine to to summary's one

* same here but only 2 lines below just before you connect to notify.
* Extra "to".

@@ +232,3 @@
+            machine = summary.machine;
+        });
+        // Notify the user of setup failure

Same here.

@@ +238,3 @@
+            window.notificationbar.display_error (_("Box setup failed"));
+        });
+        // Show the customization page when requested by the summary

and here.

@@ +446,3 @@
     }
 
+    public async bool sum_up_cancellable (WizardSourcePage source_page, CollectionSource? source, VMCreator? vm_creator, Cancellable? cancellable) {

* do_review_cancellable was an unfortunate name, iirc named to avoid name conflict with method that did the same but wasn't cancellable.
* sum_up -> show_summary ?

@@ +587,3 @@
     }
 
+    public void cleanup () {

I don't see the point of renaming the public method and having a separate clear method.
Comment 92 Zeeshan Ali 2015-06-23 18:41:25 UTC
Review of attachment 305826 [details] [review]:

Since you didn't answer my first question from previous review, I assume there are no changes but to simply move the code and update of Wizard accordingly.

::: src/wizard-review-page.vala
@@ +7,3 @@
+
+    [GtkChild]
+    private WizardSummary summary;

The point of WizardSummer was quite the same as that of this new class. IMO you should simply move the code into WizardSummary and then move that into a separate file (separate patch).

@@ +16,3 @@
+    private unowned WizardWindow wizard_window;
+
+    public Machine? machine { get; private set; default = null; }

While we are moving these props here, lets take this opportunity to correct the order of public vs private etc.

@@ +40,3 @@
+        return_if_fail (review_cancellable == null);
+
+        // Show the nokvm infobar if requested by the summary

this and following comments are a bit far from the relevant context. Its the same set of changes I commented on, in a previous patch.
Comment 93 Zeeshan Ali 2015-06-23 18:43:42 UTC
Review of attachment 305827 [details] [review]:

If you are going to put these changes in a separate page for review, you probably should do the same for other pages. Otherwise they'll feel offended. :)
Comment 94 Zeeshan Ali 2015-06-23 18:52:55 UTC
Review of attachment 305828 [details] [review]:

Not needed. We should just have WizardSummary (maybe renamed to WizardReviewPage for consistency).
Comment 95 Zeeshan Ali 2015-08-25 20:32:34 UTC
Meh, this got completely forgotten. Seeing that we did have a lot of progress here and it's mostly just about reworking the patches here, I would really appreciate if you could try to get this done before 3.18. Once we are done with 3.18, you can go on vacation for an year. :)
Comment 96 Adrien Plazas 2015-08-28 09:28:48 UTC
In fact I kept working on it up until slightly before GUADEC, but trying to refactor it the right way was really hard and I didn't want to rush it.

What's the point of replacing known and working spaghetti code with new unknown slightly less spaghetti code, right?

Maybe we could split this refactoring up into several page refactorings? It would probably ease the reviews and speed the refatoring up.
Comment 97 Zeeshan Ali 2015-09-29 18:39:23 UTC
Alright, could you please get this to completion in next few months?
Comment 98 Adrien Plazas 2015-09-29 20:19:00 UTC
Sure.
Comment 99 GNOME Infrastructure Team 2018-01-11 10:23:41 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-boxes/issues/60.