Code review comment for lp:~albertomilone/gnome-control-center/randr-virtual

Revision history for this message
Alberto Milone (albertomilone) wrote :

On Thursday 29 January 2009 18:16:09 you wrote:
> Alberto Milone wrote:
> > On Thursday 29 January 2009 17:48:37 Mark Shuttleworth wrote:
> >> Alberto Milone wrote:
> >>> Alberto Milone has proposed merging
> >>> lp:~albertomilone/gnome-control-center/randr-virtual into
> >>> lp:~ubuntu-core-dev/gnome-control-center/ubuntu.
> >>>
> >>> Requested reviews:
> >>> VCS imports (vcs-imports)
> >>>
> >>> With xserver 1.6, the ctrl-alt-backspace shortcut for killing X has
> >>> been defaulted to off.
> >>>
> >>> At UDS it was decided for Ubuntu to accept this upstream change, but to
> >>> provide GUI mechanisms for GNOME allowing the user to switch it on or
> >>> off.
> >>>
> >>> This branch implements this mechanism.
> >>
> >> I very much appreciate Alberto's work in implementing this, but would
> >> like to test the decision against our commitment to ease of use. I think
> >> enabling or disabling this feature in X is an expert option. I think we
> >> can choose to enable it, or disable it, and of course we can allow
> >> experts to change that through a config file or through a rebuild, but
> >> this capability *should not* be exposed to end-users as a GUI option. It
> >> will create more confusion than it will solve.
> >>
> >> Mark
> >
> > Dear Mark,
> >
> > I see your concern but the option which my patch adds is very simple, see
> > the following screenshot:
> > http://albertomilone.com/ubuntu/gnome/gnome3.png
> >
> > It says "Ctrl+Alt+Backspace restarts the xserver" and doesn't even
> > mention the word "DontZap".
>
> Alberto,
>
> While setting the option is simple:
> 1. It does make it too easy for novice users to inadvertently set up
> their machines to do something that is potentially undesirable.

This change requires root privileges (through PolicyKit) and won't have any
side effect unless Ctrl+Alt+Backspace is pressed.

> 2. Every option that we add to Ubuntu diminishes the value of every
> other option. I think this is an important point to consider. Is this
> option important enough to add to the cognitive effort that Ubuntu users
> must invest?
>
> Users have to stop and consider if they should use the option. What is
> it? Should I care? At that point they have already paid a price.
>

True. This will make users think when they open the screen resolution panel
but I think that the description is clear enough. A word like "restarts"
should be clear enough (even if you don't know what the server is) .

I didn't study Ergonomics though and I can be wrong ;)

> Therefore, we should be absolutely certain that the benefit to users is
> worth it. In this case, I wonder if the option really gives us an out
> with the vocal users who don't want to see the option changed, and there
> fore is meant to benefit us, not the users.
>
> Thoughts?

It seems clear to me that this change in X will cause a lack of consistency
(after a dist-upgrade) in the behaviour of the system. New users will probably
benefit from the change in X but some old users will not be pleased, others
may even think that something is wrong with their system (as we're not
speaking about advanced users).

I think of this patch as a compromise between leaving users confused after an
upgrade and keeping DontZap disabled by default in X (despite upstream's
decision). In any case this change is definitely something worth mentioning in
the release notes together with some easy steps to re-enable this
functionality.

Of course I'm fine with either solution (i.e. rejecting or accepting this
patch).

Alberto

« Back to merge proposal