On Fri, Jul 03, 2009 at 10:48:49PM +0800, Ryan Li wrote:
> On code level, the main thing I have done is to split the
> ScanAndConnect() function into Scan() and Connect(), and remove the
> `goto' statement, using a static member as replacement, and Connect() is
> triggered only when the value combo box is changed. Also changed the
> layout of the window, removed the Fixed widget, making the whole thing
> much easier to extend.
>
> There must be bugs and inconveniences, and I hope to hear your opinions.
I really like this patch, especially making it more modular, breaking
up the Scan and Connect functions. Thank you very much!
I do have a few complaints before applying:
1) The StatusBarHandler class is a great idea. It should be a
standalone class, either BackupWindow.h or its own header.
Also I notice that StatusBarHandler gets rid of the call
to m_pStatusBar->show_now()... any thoughts on that?
2) All Barry functionality was wrapped in the DeviceInterface
class. As such, m_pProbe should not be in BackupWindow,
and BackupWindow shouldn't have to do 'new Probe', nor
worry about its memory handling.
3) The drop down list should say "No device selected" or
"Please select device" if there are multiple devices
available. It shouldn't just be empty. Also, if there
is only one device available, it should connect to it
right away. If you want to make this configurable, that's
ok, (default to the old behaviour) but for those who use
the GUI a lot, that extra series of clicks just to select
their one and only device will get tiring.
4) I don't like the new packed layout. This is a show stopper for me.
Everything is packed up tight against the edge of the
window, and looks bad. I've attached a screenshot of what
the default looks like on my system.
On the "future ideas" front (which might be future patches if you like),
it might be a good idea for Scan() to clear any existing table entries, so
that it can be called repeatedly to refresh the device list. Perhaps on
a timer or a button.
Also, the better way of handling CheckWorkingDevice() is to enable and
disable buttons and menu items based on what is possible given the current
state. If no device is selected, the Backup button, for example, should
be greyed out. The check you have now is fine, this would just be nice
to have.
- Chris
------------------------------------------------------------------------------
_______________________________________________
Barry-devel mailing list
Barry-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/barry-devel
