Merge lp:~djaler1/screenshot-tool/fix-1670010 into lp:~elementary-apps/screenshot-tool/trunk

[Kirill Romanov]

Fix bug #1670010

Fix bug #1670010

[Sergey Kislyakov (defman21)]

Works fine for me.

review: Approve

[Jeremy Wootten (jeremywootten)]

I could not reproduce this bug in the current trunk.

@delay should not be allowed to be zero (or less) anyway. The SpinButton has a minimum value of 1, but the settings allow the user to change the delay to zero or less which, for negative delays, causes this branch to hang. It also hangs if you have a delay of zero with redaction.

As a quick fix you could check the value of delay on construction in order that it is consistent with the SpinButton range.

delay = int.max (1, settings.get_int ("delay"));

Ideally the settings schema should be also changed to not allow values less than 1.

review: Needs Fixing (code, function)

[Kirill Romanov (djaler1)]

I can set zero delay from command line, for example. And I need this functionality, I implement it. Cause 1 second delay is too long for me, when I use keybindings

[Kirill Romanov (djaler1)]

And negative values of delay is not problem of current branch, cause this branch is only for bug fix.

[Sergey Kislyakov (defman21)]

I don't see any reasons to make the delay always be >= 1. I don't want to wait for a second to make a screenshot. There shouldn't be a way to set it < 0 (it's not related to the fix though and it's a different bug), but there's nothing wrong with 0 delay.

[Jeremy Wootten (jeremywootten)]

If a 0 second delay is to be accepted then the SpinButton should also allow it and, more importantly, it should also work with redact == true. The value of timout must not be negative so I believe fixing is still required.

[Kirill Romanov (djaler1)]

Jeremy, all what you say is not about current bug fix.

[Kirill Romanov (djaler1)]

In my branch I just add 100ms if zero delay is set. And the correction of the old is a separate matter

[Sergey Kislyakov (defman21)]

I think it's better to send another merge request to address the issue you're talking about, Jeremy. I've filed a bug related to 0 delay which could happen even without the Kirill's patch.

[lp:~djaler1/screenshot-tool/fix-1670010]

320. By Kirill Romanov on 2017-04-09

Fix #1681254

[Kirill Romanov (djaler1)]

Jeremy, I added some fixes about negative timeout. Hope you'' like it

[Sergey Kislyakov (defman21)]

Looks fine for me.

review: Approve

[Jeremy Wootten (jeremywootten)]

@Sergey: I agree it is in general better for merge to address a single issue. I would make an exception where the same, or closely linked, lines of code are affected and the issues are linked (in this case to do with delay).

[Jeremy Wootten (jeremywootten)]

@Kirill: I like the separate get_timeout function, however I find that with a delay setting of zero the screenshot is sometimes unsuccessful or corrupted, especially with redaction . I am not sure of the reason but you could try setting a non-zero minimum value returned from get_timeout () (although it would be better to understand the real reason). I think it would be more convenient for timeout to be consistently in milliseconds.

Alternatively, you could revert the minimum (reliable) delay to 1 second and address the problems with zero delay in another branch.

review: Needs Fixing (function)

[lp:~djaler1/screenshot-tool/fix-1670010]

321. By Kirill Romanov on 2017-04-10

get_timeout() now in millisecond, and return minimum 100ms

[Kirill Romanov (djaler1)]

@Jeremy: What about now?

[Jeremy Wootten (jeremywootten)]

Yes, that works fine now :-) Approving.

review: Approve (code, function)

=== modified file 'schemas/net.launchpad.screenshot.gschema.xml' (properties changed: -x to +x)
--- schemas/net.launchpad.screenshot.gschema.xml	2016-03-24 04:14:18 +0000
+++ schemas/net.launchpad.screenshot.gschema.xml	2017-04-10 18:08:41 +0000
@@ -1,5 +1,5 @@
 <schemalist>
-	<schema id="net.launchpad.screenshot" path="/net/launchpad/screenshot/" gettext-domain="screenshot">
+    <schema id="net.launchpad.screenshot" path="/net/launchpad/screenshot/" gettext-domain="screenshot">
         <key name="format" type="s">
             <default>"png"</default>
         </key>
@@ -13,10 +13,11 @@
             <default>false</default>
         </key>
         <key name="delay" type="i">
+            <range min="0" max="15"/>
             <default>1</default>
         </key>
         <key name="folder-dir" type="s">
             <default>""</default>
         </key>
-	</schema>
+    </schema>
 </schemalist>

=== modified file 'src/ScreenshotWindow.vala'
--- src/ScreenshotWindow.vala	2017-03-21 17:13:44 +0000
+++ src/ScreenshotWindow.vala	2017-04-10 18:08:41 +0000
@@ -100,7 +100,7 @@
             var delay_label = new Gtk.Label (_("Delay in seconds:"));
             delay_label.halign = Gtk.Align.END;
 
-            var delay_spin = new Gtk.SpinButton.with_range (1, 15, 1);
+            var delay_spin = new Gtk.SpinButton.with_range (0, 15, 1);
             settings.bind ("delay", delay_spin, "value", GLib.SettingsBindFlags.DEFAULT);
 
             var grid = new Gtk.Grid ();
@@ -172,7 +172,7 @@
 
         public ScreenshotWindow.from_cmd (int? action, int? delay, bool? grab_pointer, bool? redact, bool? clipboard) {
             if (delay != null) {
-                this.delay = delay;
+                this.delay = int.max (0, delay);
             }
 
             if (grab_pointer != null) {
@@ -405,10 +405,24 @@
             }
         }
 
+        private int get_timeout(int delay, bool redact) {
+            int timeout = delay * 1000;
+
+            if (redact) {
+                timeout -= 1000;
+            }
+
+            if (timeout < 100) {
+                timeout = 100;
+            }
+
+            return timeout;
+        }
+
         private void capture_screen () {
             this.hide ();
 
-            Timeout.add_seconds (delay - (redact ? 1 : 0), () => {
+            Timeout.add (get_timeout (delay, redact), () => {
                 if (from_command == false) {
                     this.present ();
                 }
@@ -424,7 +438,7 @@
             screen = Gdk.Screen.get_default ();
 
             this.hide ();
-            Timeout.add_seconds (delay - (redact ? 1 : 0), () => {
+            Timeout.add (get_timeout (delay, redact), () => {
                 list = screen.get_window_stack ();
                 foreach (Gdk.Window item in list) {
                     if (screen.get_active_window () == item) {
@@ -479,11 +493,11 @@
             var win = selection_area.get_window ();
 
             selection_area.captured.connect (() => {
-            	if (delay == 0) {
-            		selection_area.set_opacity (0);
-            	}
+                if (delay == 0) {
+                    selection_area.set_opacity (0);
+                }
                 selection_area.close ();
-                Timeout.add_seconds (delay - (redact ? 1 : 0), () => {
+                Timeout.add (get_timeout (delay, redact), () => {
                     if (from_command == false) {
                         this.present ();
                     }