GNOME Bugzilla – Bug 727675
HiDPI Window scaling factor: add a confirmation dialog or a warning?
Last modified: 2014-12-30 11:25:50 UTC
Hi, Trying out GNOME 3.12 on Fedora 20, I stupidly changed the HiDPI window scaling factor in GNOME Tweak Tool. A very bad idea on a 1366x768 screen... After this operation, I could barely see the top of the window and I never managed (even with Super+Mouse to move the window) to revert the change. Hopefully, restarting the computer with an external monitor (1920x1080), the secondary monitor was not cloned and I could change back the scaling factor. I know a simple end user should not use GNOME tweak tool, but a nice enhancement would be to add a confirmation dialog (like when you change resolution), for instance: Do you want to keep this display settings ? Settings changes will revert in 10 seconds. Hope that helps, Rémi.
*** Bug 727714 has been marked as a duplicate of this bug. ***
Created attachment 287105 [details] [review] Add a confimation dialog Add a confirmation message that will revert changes after 20 seconds if the user does not make a choice (keep or revert).
*** Bug 728192 has been marked as a duplicate of this bug. ***
Review of attachment 287105 [details] [review]: ::: gtweak/tweaks/tweak_group_windows.py @@ +61,2 @@ self._xsettings = XSettingsOverrides() + self._xsetting_original = self._xsettings.get_window_scaling_factor() Awkward name. Perhaps _original_factor ? @@ +82,3 @@ + + def _close(self): + if self.countdown > 0: if self.source > 0: remove() self.source = 0 let's not risk missing removing the source because of a (potential) bug in the countdown bookkeeping. @@ +87,2 @@ def _on_value_changed(self, adj): + if adj.get_value() != self._xsetting_original: Please write this as if adj.get_value() == original: return to trim on the indentation @@ +89,3 @@ + self._xsettings.set_window_scaling_factor(adj.get_value()) + + self.countdown = 20 Style nit: all the variables you add on self here should be prefixed with an underscore since they're "private". @@ +99,3 @@ + text=first_message, + buttons=(_("Revert Settings"), Gtk.ResponseType.NO, + _("Keep Changes"), Gtk.ResponseType.YES)) I get a runtime warning saying that we should use the "add_buttons" method for adding buttons. Please change @@ +110,3 @@ + self._close() + + elif response == Gtk.ResponseType.NO: elif: Try cancelling the dialog by pressing the Escape key on your keyboard... @@ +113,3 @@ + self._xsettings.set_window_scaling_factor(self._xsetting_original) + adj.set_value(self._xsetting_original) + self._close() call self._close() in a single place out of the if/else clause
Created attachment 287250 [details] [review] Revised patch
(In reply to comment #4) Hi Rui, thanks for the review, I've implemented your comments except for this one: > if self.source > 0: > remove() > self.source = 0 > > let's not risk missing removing the source because of a (potential) bug in the > countdown bookkeeping. I still think that the correct way of doing it is checking for the countdown and not the source, I've also checked how it works in the display setting and it's the same.
Review of attachment 287250 [details] [review]: ::: gtweak/tweaks/tweak_group_windows.py @@ +82,3 @@ + + def _close(self): + if self._countdown > 0: (In reply to comment #6) > I still think that the correct way of doing it is checking for the countdown > and not the source, Sorry but no. This is cleanup for the GSource which should be unrelated to any other logic, i.e. the GSource either is there and we need to remove it or it isn't. The idiom used in most GNOME code is to save the GSource id and set it to 0 when the GSource is removed which means that if the id is >0 the GSource hasn't been removed yet and thus needs cleaning up. > I've also checked how it works in the display setting and > it's the same. Which "display setting"? If you mean the code in gnome-shell, it does check this._timeoutId which is the GSource id. @@ +94,3 @@ + + first_message = _("Do you want to keep these HiDPI settings?") + self.second_message = _("Settings will be reverted in %d seconds") self._second_message @@ +96,3 @@ + self.second_message = _("Settings will be reverted in %d seconds") + + self.dialog = Gtk.MessageDialog( self._dialog @@ +104,3 @@ + self.dialog.format_secondary_text(self.second_message % self._countdown) + + self.source = GLib.timeout_add_seconds(interval=1, function=self._timeout_func) self._source
Created attachment 287479 [details] [review] Proposed patch Revised patch including Rui's comments
Review of attachment 287479 [details] [review]: Ok, this is ready to go in. But we're in string and UI freeze on the 3.14 branch so you'll need to ask for freeze breaks[1] before pushing. [1] https://wiki.gnome.org/ReleasePlanning/RequestingFreezeBreaks ::: gtweak/tweaks/tweak_group_windows.py @@ +76,3 @@ + + if self._countdown == 0: + self._source = 0 You could just call this._close() here, but not a big deal
*** Bug 740107 has been marked as a duplicate of this bug. ***
should we land this in 3.15 now ?
(In reply to comment #11) > should we land this in 3.15 now ? Matthias, there is no branch for 3.14 so I was waiting to push it to master.
4db51ee..cdac8bf master -> master This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
Thanks for your work !