Re: [spice-gtk PATCH 1/3 v2] spicy-connect: Connect dialog changed to window

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hey,

On Tue, Jun 09, 2015 at 04:59:32PM +0200, Lukas Venhoda wrote:
> Connect dialog in spicy had no transinient parent.
> It didn't make much sense for it to be a dialog.
> 
> Changed the connect dialog to a window.
> Moved the dialog code to its own module.

I'd make the description a bit more complex, eg "Since the connect dialog in
spicy does not have a transient parent, it does not make much sense for
it to be a dialog. This commit changes the dialog to be a GtkWindow, and
moves the dialog code to its own file."

> 
> Changed response ID from ambiguous -1 and 0 to GTK_RESPONSE_ACCEPT
> and GTK_RESPONSE_REJECT.
> 
> Improved UX:
>    - ESC to close, ENTER to connect.
>    - Address and port are now required to connect.
>    - Focusing in entries will now unselect recent connection.
>       - If you rewrite something in entries, you can now reselect
>         connection in the recent chooser.

Having each of these changes in a separate commit would make the overall
series easier to review as the dialog -> window change is probably
fairly mechanical, the move to a new file should change very little
code, and then each of these improvements would be a very small commit,
which would be much easier to review for potential bug additions.
Commits 2 and 3 would belong to the commit moving the code to a new
file, as they logically belong together.

With everything in a single commit, the reviewer has to figure out to
what of these logical change each hunk belongs to, which is much harder
and time consuming :(

Christophe

Attachment: pgpbo2iHNhduj.pgp
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]