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

Bug #197673 reported by Emmanuel Pacaud
36
This bug affects 1 person
Affects Status Importance Assigned to Milestone
gnome-control-center (Ubuntu)
Fix Released
High
Ubuntu Desktop Bugs

Bug Description

Binary package hint: gnome-control-center

Since xrandr is not rock stable yet, it would be nice if the gnome-display-properties dialog would ask for change acknowledgment after the new settings are applied, and revert automatically to the previous ones after a delay (like windows does, for examples). At least it would help when the x server has not crashed completely... :)

ii gnome-control- 1:2.21.92-0ubu utilities to configure the GNOME desktop

Changed in gnome-control-center:
assignee: nobody → bryceharrington
importance: Undecided → Wishlist
Revision history for this message
Bryce Harrington (bryce) wrote :

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.

Changed in gnome-control-center:
status: New → In Progress
Revision history for this message
Rotbart van Dainig (rotbart-van-dainig) wrote :

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!

Revision history for this message
Bryce Harrington (bryce) wrote :

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.

Changed in gnome-control-center:
assignee: bryceharrington → nobody
Revision history for this message
Sebastien Bacher (seb128) wrote :

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

Changed in gnome-control-center:
assignee: nobody → bryceharrington
importance: Wishlist → High
milestone: none → ubuntu-8.04
status: In Progress → New
Revision history for this message
Rotbart van Dainig (rotbart-van-dainig) wrote :

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

So is this an Ubuntu-specific change?

Revision history for this message
Sebastien Bacher (seb128) wrote :

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

Revision history for this message
Rotbart van Dainig (rotbart-van-dainig) wrote :

So this bug is 'Confirmed', right?

Changed in gnome-control-center:
status: New → Confirmed
Revision history for this message
Bryce Harrington (bryce) wrote :

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.

Revision history for this message
Lionel Dricot (ploum-deactivatedaccount) wrote :

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.

Revision history for this message
Bryce Harrington (bryce) wrote :

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.

Revision history for this message
Martin Pitt (pitti) wrote :

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).

Revision history for this message
Bryce Harrington (bryce) wrote :

One hackish coming right up...

Revision history for this message
Bryce Harrington (bryce) wrote :

untested; for review only

Changed in gnome-control-center:
status: Confirmed → In Progress
Revision history for this message
Florent Mertens (givre) wrote :

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 ;)

Revision history for this message
Florent Mertens (givre) wrote :

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.

Revision history for this message
Bryce Harrington (bryce) wrote :

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?)

Revision history for this message
Emmanuel Pacaud (emmanuel-pacaud) wrote :

> (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).

Revision history for this message
Florent Mertens (givre) wrote :

Hi all,

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

Revision history for this message
Florent Mertens (givre) wrote :

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

Revision history for this message
Florent Mertens (givre) wrote :

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

Revision history for this message
Florent Mertens (givre) wrote :

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

Revision history for this message
Bryce Harrington (bryce) wrote :

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.

Revision history for this message
Sebastien Bacher (seb128) wrote :

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

Revision history for this message
Miguel Martinez (el-quark) wrote :

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.

Revision history for this message
Florent Mertens (givre) wrote :

> 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.

Revision history for this message
Florent Mertens (givre) wrote :

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.

Revision history for this message
Bryce Harrington (bryce) wrote :

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.

Revision history for this message
Launchpad Janitor (janitor) wrote :

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

Changed in gnome-control-center:
status: In Progress → Fix Released
Revision history for this message
Bryce Harrington (bryce) wrote :

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.

Revision history for this message
Bevin_Watson (bevin-watson) wrote :

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.

Revision history for this message
Bart de Koning (bratdaking) wrote :

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

Bryce Harrington (bryce)
Changed in gnome-control-center:
assignee: bryceharrington → nobody
milestone: ubuntu-8.04 → none
Revision history for this message
Jim (JR) Harris (jimrh) wrote :

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

Revision history for this message
Jim (JR) Harris (jimrh) wrote :

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

Changed in gnome-control-center:
status: Fix Released → Incomplete
Revision history for this message
Jim (JR) Harris (jimrh) wrote :

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

Changed in gnome-control-center:
assignee: nobody → seb128
Revision history for this message
Sebastien Bacher (seb128) wrote :

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

Changed in gnome-control-center:
assignee: seb128 → desktop-bugs
status: Incomplete → Fix Released
Revision history for this message
Jim (JR) Harris (jimrh) wrote :

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

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Bug attachments

Remote bug watches

Bug watches keep track of this bug in other bug trackers.