GNOME Bugzilla – Bug 750784
Refactor wizard pages into their own classes
Last modified: 2018-01-11 10:23:41 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.
Created attachment 305064 [details] [review] Add wizard-setup-page This adds the WizardSetupPage widget. It is part of the wizard refactoring process.
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.
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.
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.
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.
Review of attachment 305064 [details] [review]: Also in the description, instead of repeating the shortlog with different words, please describe what this class is.
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.
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?
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.
(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. =)
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.
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.
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.
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.
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.
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.
(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. :)
Review of attachment 305224 [details] [review]: Looks good but needs to be squashed with following patch.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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 :)
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.
Review of attachment 305366 [details] [review]: Sure, I'll do it.
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.
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.
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.
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.
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.
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.
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?
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 :)
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.
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
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.
Review of attachment 305395 [details] [review]: lgtm
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.
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.
Created attachment 305456 [details] [review] Rename WizardSource to WizardSourcePage This change is needed to keep the wizard's pages' name consistent.
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.
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.
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.
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; });
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.
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.
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.
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. :)
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.
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.
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.
Created attachment 305689 [details] [review] Rename WizardSource to WizardSourcePage This change is needed to keep the wizard's pages' name consistent.
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.
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.
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.
Review of attachment 305686 [details] [review]: ACK
Review of attachment 305687 [details] [review]: "wizard" -> "Wizard" "source page" -> WizardSource "into it" - "into WizardSource
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() ?
Review of attachment 305689 [details] [review]: ack
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.
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
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.
Review of attachment 305690 [details] [review]: OK, let's change this later when refactoring the pages themselves then.
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.
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.
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.
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.
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.
Created attachment 305823 [details] [review] Rename WizardSource to WizardSourcePage This change is needed to keep the wizard's pages' name consistent.
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.
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.
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.
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.
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.
(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.
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.
Review of attachment 305821 [details] [review]: ack
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 (
Review of attachment 305823 [details] [review]: ACK, assuming nothing changed since last review.
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. :)
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.
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.
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. :)
Review of attachment 305828 [details] [review]: Not needed. We should just have WizardSummary (maybe renamed to WizardReviewPage for consistency).
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. :)
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.
Alright, could you please get this to completion in next few months?
Sure.
-- 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.