GNOME Bugzilla – Bug 669771
Present bootable medias on 'Source' page
Last modified: 2016-03-31 14:00:42 UTC
Currently this is limited to hardware media but once this is in place, adding support for user's ISO files (based on Tracker) shouldn't be very difficult.
Created attachment 207213 [details] [review] wizard: Remove redundant source page
Created attachment 207214 [details] [review] Introducing MediaManager class Separate out installer media management into a separate class.
Created attachment 207215 [details] [review] wizard: Separate fields for separate MenuBox instances
Created attachment 207216 [details] [review] wizard: Present bootable medias on 'Source' page Currently this is limited to hardware media but once this is in place, adding support for user's ISO files (based on Tracker) shouldn't be very difficult.
Created attachment 207217 [details] [review] wizard: Move to 'Preparation' once file/media is selected We were just going to URI page when user chose a file or media and had to click 'Continue'. Not only there was an unneeded click required, there is also no need to show URI to user.
Proof that it works: http://static.fi/~zeenix/tmp/boxes-hw-media-sources.webm :)
Review of attachment 207213 [details] [review]: ack.. it is meant to be a different page, it was there a the reminder as the current page URI is really not for files. It is based on the design for remote desktops. We don't have design for it, but we can try to make one. Something that would just show some data related to the selected file, a different icon, a different text (and no entry!)
Review of attachment 207214 [details] [review]: ack ::: src/media-manager.vala @@ +17,3 @@ + } + + public async InstallerMedia get_media_for_path (string path, Cancellable? cancellable) throws GLib.Error { that method name could be more explicit: make_installer_for_path () ?
Review of attachment 207215 [details] [review]: ::: src/wizard-source.vala @@ +36,3 @@ + private Boxes.MenuBox main_menubox; + private Boxes.MenuBox url_menubox; do we need to keep the url_menubox around? I would just declare it in the scope it is used, with "var url_menubox =".
Review of attachment 207216 [details] [review]: looks ok, just a few nitpicks ::: src/app.vala @@ +117,3 @@ connections = new HashTable<string, GVir.Connection> (str_hash, str_equal); + duration = settings.get_int ("animation-duration"); + setup_ui (); any reason why this needs to be moved down? @@ +175,3 @@ + if (source.name == "QEMU Session") + notify_property ("default-connection"); + any reason why this needs to be moved up? ::: src/vm-creator.vala @@ +12,3 @@ + + app.notify["default-connection"].connect (() => { + connection = app.default_connection; instead of using signal notifiers, and copying a reference, why not just use: private Connection connection { get { return app.default_connection; } } ::: src/wizard.vala @@ +233,3 @@ + install_media = this.wizard_source.install_media; + prep_progress.fraction = 1.0; + Idle.add (() => { is the idle needed? I remember we discussed some issues and some solution related to the usage of idle in the wizard, but I can't remember. Have you tried without the idle?
(In reply to comment #7) > Review of attachment 207213 [details] [review]: > > ack.. it is meant to be a different page, it was there a the reminder as the > current page URI is really not for files. It is based on the design for remote > desktops. We don't have design for it, but we can try to make one. Something > that would just show some data related to the selected file, a different icon, > a different text (and no entry!) File for remote desktop? You lost me there. How does that work?
Review of attachment 207217 [details] [review]: ok, this is related to the first patch, ack.
(In reply to comment #11) > (In reply to comment #7) > > Review of attachment 207213 [details] [review] [details]: > > > > ack.. it is meant to be a different page, it was there a the reminder as the > > current page URI is really not for files. It is based on the design for remote > > desktops. We don't have design for it, but we can try to make one. Something > > that would just show some data related to the selected file, a different icon, > > a different text (and no entry!) > > File for remote desktop? You lost me there. How does that work? the "URL page" is the page to enter URL for remote collection & desktops. It wasn't meant for local file location. I was expecting a sub-page similar to that "URL page" for local files, the way I described. But peraps this isn't needed at all in fact
iow, it's sort of a hack to open a dialog when clicking on "File" today. In my mind, it is supposed to slide into a different page to select a file. That "FILE" page design doesn't exist. so we came up with a just a open file dialog that really doesn't fit well.
(In reply to comment #10) > Review of attachment 207216 [details] [review]: > > looks ok, just a few nitpicks > > ::: src/app.vala > @@ +117,3 @@ > connections = new HashTable<string, GVir.Connection> (str_hash, > str_equal); > + duration = settings.get_int ("animation-duration"); > + setup_ui (); > > any reason why this needs to be moved down? So that connection hash table is there before any other modules initialized during setup_ui() chain can access it without crashing (VMCreator in this case). Perhaps I should just move the connections initialization to top/construct instead? > @@ +175,3 @@ > + if (source.name == "QEMU Session") > + notify_property ("default-connection"); > + > > any reason why this needs to be moved up? To ensure that default_connection is initialized before any item/machine is created for its domains (and hence trigering of event handlers) to avoid any unforseeable consequences/crashes (I think I was actually getting a crash but can't be sure). > ::: src/vm-creator.vala > @@ +12,3 @@ > + > + app.notify["default-connection"].connect (() => { > + connection = app.default_connection; > > instead of using signal notifiers, and copying a reference, why not just use: > > private Connection connection { get { return app.default_connection; } } I know it is not important right now but I just have this habbit of avoiding cyclic references. :) > ::: src/wizard.vala > @@ +233,3 @@ > + install_media = this.wizard_source.install_media; > + prep_progress.fraction = 1.0; > + Idle.add (() => { > > is the idle needed? I remember we discussed some issues and some solution > related to the usage of idle in the wizard, but I can't remember. Have you > tried without the idle? Indeed I have. We need the current page change to complete first before we can move to the next one or we could return a boolean from 'prepare' func to indicate that we want to already move to the next page instead.
(In reply to comment #14) > iow, it's sort of a hack to open a dialog when clicking on "File" today. In my > mind, it is supposed to slide into a different page to select a file. That > "FILE" page design doesn't exist. so we came up with a just a open file dialog > that really doesn't fit well. Ah! I already have a almost-baked solution to present ISOs from tracker[1] (with logos \o/). In the presence of that, I think the importance/usage of 'Files' option will greatly diminish so I won't worry much about that for now. [1] http://static.fi/~zeenix/tmp/boxes+os-logos.png
(In reply to comment #8) > Review of attachment 207214 [details] [review]: > > ack > > ::: src/media-manager.vala > @@ +17,3 @@ > + } > + > + public async InstallerMedia get_media_for_path (string path, Cancellable? > cancellable) throws GLib.Error { > > that method name could be more explicit: make_installer_for_path () ? I have been (trying to) consistently use either the term 'installer media' or just 'media' if that feels too long so how about 'create_installer_media_for_path' ?
(In reply to comment #15) > So that connection hash table is there before any other modules initialized > during setup_ui() chain can access it without crashing (VMCreator in this > case). Perhaps I should just move the connections initialization to > top/construct instead? that would make more sense. > > @@ +175,3 @@ > > + if (source.name == "QEMU Session") > > + notify_property ("default-connection"); > > + > > > > any reason why this needs to be moved up? > > To ensure that default_connection is initialized before any item/machine is > created for its domains (and hence trigering of event handlers) to avoid any > unforseeable consequences/crashes (I think I was actually getting a crash but > can't be sure). ok > > ::: src/vm-creator.vala > > @@ +12,3 @@ > > + > > + app.notify["default-connection"].connect (() => { > > + connection = app.default_connection; > > > > instead of using signal notifiers, and copying a reference, why not just use: > > > > private Connection connection { get { return app.default_connection; } } > > I know it is not important right now but I just have this habbit of avoiding > cyclic references. :) I would argue you are hiding it behing the app "notify" signal. Instead, you could make it explicit, without requiring signals. > Indeed I have. We need the current page change to complete first before we can > move to the next one or we could return a boolean from 'prepare' func to > indicate that we want to already move to the next page instead. right, some other day we need to make this better.
(In reply to comment #17) > I have been (trying to) consistently use either the term 'installer media' or > just 'media' if that feels too long so how about > 'create_installer_media_for_path' ? The name "media" is "an object" in my mind. An "installer" is a better name for an install helper, so ok for "create/make_installer_something()". "make" is typical name for factory pattern methods, "create" works too.
Created attachment 207396 [details] [review] Introducing MediaManager class Separate out installer media management into a separate class.
Created attachment 207397 [details] [review] wizard: Separate fields for separate MenuBox instances
Created attachment 207398 [details] [review] wizard: Present bootable medias on 'Source' page Currently this is limited to hardware media but once this is in place, adding support for user's ISO files (based on Tracker) shouldn't be very difficult.
Review of attachment 207396 [details] [review]: ::: src/media-manager.vala @@ +17,3 @@ + } + + public async InstallerMedia get_media_for_path (string path, Cancellable? cancellable) throws GLib.Error { the method name hasn't been updated
Review of attachment 207397 [details] [review]: ack
(In reply to comment #23) > Review of attachment 207396 [details] [review]: > > ::: src/media-manager.vala > @@ +17,3 @@ > + } > + > + public async InstallerMedia get_media_for_path (string path, Cancellable? > cancellable) throws GLib.Error { > > the method name hasn't been updated Oops I renamed the method in wrong class. :)
Review of attachment 207398 [details] [review]: ack ::: src/vm-creator.vala @@ +6,2 @@ private class Boxes.VMCreator { + private weak App app; none of the other "App app;" use a weak reference. I guess there are many places where it should be used... do we really care for app-life singletons? I would say no, but of course, feel free to fix all those too.
Created attachment 207400 [details] [review] Introducing MediaManager class Separate out installer media management into a separate class.
Created attachment 207401 [details] [review] wizard: Present bootable medias on 'Source' page Currently this is limited to hardware media but once this is in place, adding support for user's ISO files (based on Tracker) shouldn't be very difficult.
Review of attachment 207400 [details] [review]: ack
Review of attachment 207401 [details] [review]: ack
Attachment 207217 [details] pushed as 594c3d3 - wizard: Move to 'Preparation' once file/media is selected Attachment 207397 [details] pushed as 97da6f5 - wizard: Separate fields for separate MenuBox instances Attachment 207400 [details] pushed as 8e3ca32 - Introducing MediaManager class Attachment 207401 [details] pushed as 598f438 - wizard: Present bootable medias on 'Source' page