gnome-display-properties should revert change automatically if not acknowledged

Agreed this is fairly important.

I worked a bit on the dialog Friday, but unfortunately I found that the way the gui and daemon work, the settings are applied and *then* tested, rather than vice versa as the old Screen Resolution tool did, so it's not 100% obvious how to implement a revert capability. I've asked upstream for direction, and am awaiting feedback.

This is a serious problem:

The old applet reverted any change after a given time, thus recovering from wrong resolutions and false frequencies.

Currently, it's easy to completely break the only screen available by setting an unsupported resolution/frequency or, even worse, by turning it off!

Unassigning myself; I've received no feedback from upstream on this issue and do not have plans myself at this time to work on this feature.

Bryce, I disagree on shipping this tool in hardy if there is no way to revert broken change, it's just too easy to break the display using the tool right now and there is no way to revert without using the command line. I'm reassigning the bug to you since I don't think anybody else is looking at this tool, maybe something to discuss durong the next platform team meeting if you think it should be assigned to somebody else

AFAIS, the original gnome-control-center-2.22.0/capplets/display/main.c has a revert included.

So is this an Ubuntu-specific change?

the changes comes from redhat and should go upstream next cycle, it's distro specific right now though

So this bug is 'Confirmed', right?

The previous code (in main.c) worked by applying the changes via xrandr, then prompting the user about if they're okay, and then writing to gconf. So if there was a catastrophic failure preventing the prompt, no config would be written.

In redhat's new code, it writes the changes to monitors.xml first, then the changes get applied by gnome-settings-daemon. So it is not as simple as copying the dialog from the old tool in there somewhere.

Isn't it possible to backup monitors.xml then to restore it after a few seconds ? If the user confirm, the backup could be removed.

Unfortunately, many cases where xrandr fails, it locks the system up, so we'd have no way to ensure that the restoration logic would always get called reliably.

But something similar could be done by e.g. moving the previous configuration to monitors.xml.old, and then have gnome-settings-daemon notice it and, if that file is older than monitors.xml by some amount (1 minute?), to move it back and restore the system. gnome-control-center could then be modified to display a dialog which would when Accept was pressed, would delete the monitors.xml.old. But this feels awfully hacky and flimsy to me; I'd worry it would cause more problems than it solved.

Bryce,

even if the restoration logic isn't 100% reliable, the important bit of the confirmation dialog question is that only at that point the new configuration should be written. If switching resolution breaks, and restoring it doesn't work either, then the user just needs a reboot. With immediately writing the new configuration before switching configuration, most users will need to resort to reinstalling their box.

In the desktop team meeting we reaffirmed that this is a release-critical regression which we need to fix for Hardy (even if it's hackish).

One hackish coming right up...

untested; for review only

Hi bryce,

gd_randr12_revert_support.patch :
doesn't apply cleanly (as 104_gd_randr12_revert_support.patch)

cc_randr12_revert_dialog.patch :
fail to build. You probably should add at least (most obvious):
#define REVERT_COUNT 40
Also gettext is quite broken.

Another implementation idea would be to make apply_stored_configuration
 (in g-s-d) configuration file independant, like :
apply_configuration (RWScreen *screen, gchar *file)

so apply would be something like that :
configurations_write(app->all_configurations, get_test_filename(), NULL)
apply_configuration (app->screen, get_test_filename());
run_revert_dialog ()
configurations_write(app->all_configurations, get_filename(), NULL)
apply_configuration (app->screen, get_filename());

In side not, i saw that upstream is implementing clone support right now.
Do you think that this may land in ubuntu, but i understand that this is not the priority ;)

Ok, forget my previous implementation idea, i was confused by the big mess
of the upstream tree. There is of course no apply_configuration in g-s-d, it's a
running daemon (damn i'm stupid). So xrandr_capplet talk to it with Xevents.

So i have an other implementation idea. Why not create an xevent for testing
purpose.We would call it _GNOME_RANDR_ATOM_TEST for exemple. It would call
a configuration_apply_stored_test (or configuration_apply_stored(screen, testfile)
as you want) that would apply a test file.

so apply() in xrandr_capplet would work like that :
configuration_save(,,testfile)
XsendEvent <- _GNOME_RANDR_ATOM_TEST message
run_revert_dialog
configuration_save(,,file)

Might be a bit confusion. I may hack a bit on it tomorrow night to see what i can do with it.

I've reviewed the changes up to Mar 19th, and I see the Mar 28 changes for clone mode, but haven't reviewed those yet. In any case I do plan on updating our code to newer upstream stuff once this revert dialog is done.

I look forward to seeing your xevent test code. Sounds like it still requires creating a test config file, but maybe it'd be a cleaner code path.

Fwiw, I spent the day yesterday testing the dialog (and xrandr in general) on 10 different hardware configurations (3 intel/dell systems, 3 ATI's, 4 nvidia's) using the (default) open source drivers. I haven't written up the testing results yet, but I found a wide range of bugs, mostly non-catastrophic. By and large, resolution changing works, although there are some low-to-medium severity issues on a few cards. Rotation is the thing that causes severe issues. On Intel it works without issue. On ATI it causes catastrophic failure. On nVidia, it isn't even offered as an option. I also tested monitor hotplugging support and found its support varies pretty widely.

Anyway, it looks like it's pretty much the rotation case on ATI that is going to be exercising the revert dialog. ;-) (Maybe it would be good to not allow rotation on ATI at all to begin with?)

> (Maybe it would be good to not allow rotation on ATI at all to begin with?)

Probably, yes. Rotation is not a feature that important, and it's better to disable it instead of letting users crash their machine when they try this setting (even if a reboot give a working machine now).

Hi all,

Here we go.
First the gnome-desktop patch that implement :
configuration_save_test
configuration_apply_test

Then the gnome-setting-daemon patch that make it react
to _GNOME_RANDR_TEST_ATOM xevent
by calling configuration_apply_test.

Finaly the gnome-control-center patch
that make use of the above.

Humm, why did it set mime-type to text/html ???

Anyway, this is a first shoot so some tests are required, but overall this seams to works ok.
No gettext for now but that might be a futur patch. Not the most urgent.

Now i'll take some rest ;)

Cheers

Florent, cool, I'll take a look at your patch tomorrow.

Meanwhile, I've continued work on my end, and have a new update available for testing:

http://people.ubuntu.com/~bryce/Uploads/gnome-desktop_2.22.0-0ubuntu4.dsc
http://people.ubuntu.com/~bryce/Uploads/gnome-control-center_2.22.0-0ubuntu4.dsc
http://people.ubuntu.com/~bryce/Uploads/libgnome-desktop-2_2.22.0-0ubuntu4_i386.deb
http://people.ubuntu.com/~bryce/Uploads/libgnome-window-settings1_2.22.0-0ubuntu4_i386.deb
http://people.ubuntu.com/~bryce/Uploads/gnome-desktop-data_2.22.0-0ubuntu4_all.deb
http://people.ubuntu.com/~bryce/Uploads/gnome-settings-daemon_2.22.0-0ubuntu1_i386.deb
http://people.ubuntu.com/~bryce/Uploads/gnome-control-center_2.22.0-0ubuntu4_i386.deb

I don't think I have all the dependencies specified correctly, but just install all of the above and reboot. In theory the revert dialog should work properly with this. I've only tested on my dev box, and it mostly worked - I suspect the failures are due to peculiarities of my development environment, but don't know for sure (notably, as long as I *increased* the resolution with each change, it was fine, but if I decreased, it blew up). I would greatly appreciate it if people could test these on as many different platforms as possible and let me know the results.

To test after installing and rebooting, try running gnome-display-properties and then changing to different resolutions, rotations, and refresh rates. On ATI, I already know rotation fails horribly, and with -nv it seems not supported at all; but it ought to work correctly on -intel.

looking at the gnome-desktop changes, they seem to leak file in the case where false is returned in configuration_apply_stored()

Heh, I should have read this bug before spamming the ubuntu-x mailing list.

Long story short: as acknowledged here, I managed to screw my only monitor (my laptop screen) via gnome-display-properties. It was even more serious, since I've just discovered that the offending file is .gnome2/montiors.xml (I somehow expected this to be in .gconf/)

By the way, a reinstall might *not* solve the problem. If you have a separate /home partition, and the user you create during the install is the same (same name, uid and gid) you had before (very likely in a home computer), you will still have the borked monitors.xml when you boot after install.

> Florent, cool, I'll take a look at your patch tomorrow.

Do as you want. With the actual code i don't see better way to do it.
For now you can't pass objects to g-s-d so you'll always need to store new config in a test file.
BTW, i wonder why they call configuration_apply_stored() via g-s-d and don't call it directly,
that would have been easier to deal with.

> looking at the gnome-desktop changes, they seem to leak file in the case where false is returned in configuration_apply_stored()

Bryce, you might want to use get_filename() in configuration_apply_stored() too.
This avoid hard coding 2 times the name of the conf file, and you get the benefice of idle_free.

Attached is an updated patch for gnome-control-center
It re enable translation.

The *.pot file is correctly updated during build but not the *.po.
This need some `make update-po` magic, and i don't now
how to do that cleanly.

Florent, yeah I think you're right, it looks cleaner that way and gets rid of the need for freeing things.

http://people.ubuntu.com/~bryce/Uploads/gnome-desktop_2.22.0-0ubuntu5.dsc

seb128, if it looks good to you now, go ahead and upload it.

This bug was fixed in the package gnome-control-center - 1:2.22.0-0ubuntu4

---------------
gnome-control-center (1:2.22.0-0ubuntu4) hardy; urgency=low

  [Bryce Harrington]
  * debian/patches/110_cc-randr12-revert-dialog.patch:
    - Adds a revert dialog to confirm changes before applying them
      permanently (LP: #197673)
  * debian/control:
    - Update Build-Depends for gnome-desktop

  [Gabor Szeder]
  * debian/patches/111_cc-randr12-dont-disable-active-monitor.patch
    - Don't allow switching off the only active monitor (LP: #208535)

 -- Bryce Harrington <email address hidden> Fri, 04 Apr 2008 16:24:34 -0700

I've been testing this dialog on my own system and it works for the most part, but there's a few situations where it may fail, such as if the user did not have an existing ~/.gnome2/monitors.xml. I would appreciate it if others could test this out with care and report any corner cases that aren't working properly. Please feel free to reopen this bug with these issues as they're encountered (or file new ones if you prefer a clean bug.)

Meanwhile, I'll work on getting our code re-synced with upstream; I see they have a number of fixes that will probably more properly resolve some of the g-s-d and other crashes we've seen.

If I change from 1152 to 1048 and then revert I am left with a gap about an inch wide down the right hand side of the screen.
Underneath that "gap" I have an image of the old screen.
I notice that when I go from 1152 to 1048 the normal way (ie keep settings), the same thing happens but then after about 4-5 seconds the screen adjusts itself to fill up the space.
It is as if it isn't doing a refresh after the revert action.

This is old equipment struggling a bit on the Compiz "normal" settings. AMD Duron 750, ATI Radeon 9200 (128 Mb), 512 Mb RAM.

Hey all,

I just upgraded to Intrepid and played around with the resolution settings. Just to see how it works. I took a resolution way too high for my monitor (1400x1050 on a Dell 17 inch LCD that could handle maximally 1280x960) and ended up with a black screen, that just did not want to revert back. I had to start a x-term session to be able to recover my display. I think the above mentioned test period is quite critical to implement too in Intrepid...
To test whether it was just a matter for an upgraded system I tested this too on a virtual live-cd session. Also there it did automatically revert back after a small period of time, although there I could not choose for any resolutions that could crash the system.
Another related thing: should the failsafe session of gnome not automatically fall back to a safe screen resolution (read as low as possible), even that session wanted to start at a resolution of 1400x1050?

Cheers

Unfortunately, in Ubuntu 8.10 this is still WAAAAY broken. (Ref: bug 316961)

Doing the same thing - trying out resolutions on a ViewSonic A75F monitor, not only did I totally hoze my session - I hozed it in classic style.

It took me several hours of hacking around with a recovery terminal before I - by the Grace of God Almighty - finally got my session un-hozed.

The only way I could see a non-technical user doing this would be to do a *TOTAL* nuke and re-install. IMHO, a rather drastic workaround, 'eh? {:-p

I am adding the requested behavior changes to this comment - some are relatively easy (changes to the default Gnome setup), others are more complex (the actual revert logic).

- - - - - - BEGIN INSERTED TEXT - - - - - -
Requested changes:

1. Implement the display time-out feature as noted above.

2. Add an item to the recovery menu that allows the user to reset the display parameters to a sane value, (say 800x600, 60hz).

3. (Optional) Add, as one of the boot choices, an option that boots normally except it changes the screen resolution to something sane.

Note that simply passing " -- bestResolution" does not work well, since the user gets the un-desired resolution first.
- - - - - - END INSERTED TEXT - - - - - -

What say ye?

Jim

Changing from FIXED to INCOMPLETE (= not-fixed, re-opened?)
Added comment and reference to duplicate bug 316961 entered by me on 1/13/09

Jim

Sebastien,

I have decided to toss this into your in-basket for disposition.
(I'm not sure if I'm supposed to even DO this - apologies in advance if this is a huge breach of protocol.)

1. This REALLY needs to be fixed.

2. I have no idea who the "responsible developer" would be at this point in time.

Thanks!

Jim

don't assign bugs to people that's not you who decides whoever is assigned to a task, closing the bug again since there is already one open about the issue in intrepid and jaunty

A thousand humble apologies.

My original bug was marked as a duplicate of THIS bug - and THIS is not a duplicate of anything else - so I felt it was reasonable to believe that this bug was the "root" bug that all other duplicates should point toward.

Especially since there was a post-fix comment complaining that the issue was still un-resolved. So, I re-opened the bug.

If that is NOT the case, and there IS a later entry open bug against that issue, shouldn't this bug point to it? Had I known that I would have left this bug alone.

Thanks for the help!

Jim