Created attachment 228519 [details] [review]
Add a SelectArea() DBus method

This will be useful for e.g. selecting an area for a screenshot.

Review of attachment 228519 [details] [review]:

::: js/ui/selectArea.js
@@ +43,3 @@
+
+        this._rubberband = new Clutter.Canvas();
+        this._rubberband.connect('draw', Lang.bind(this, this._onRubberbandDraw));

Hm. Have you considered using a simple Clutter.Rectangle with a border and background color which you manipulate the x/y/width/height of, rather than doing cairo drawing?

@@ +129,3 @@
+        global.unset_cursor();
+
+        this.emit('destroy', geometry);

I don't really like the signal name. I'd prefer something like 'finished'.

Created attachment 230978 [details] [review]
Add a SelectArea() DBus method

--

- use ClutterRectangle
- rename destroy to finished
- more cleanups

Review of attachment 230978 [details] [review]:

::: js/ui/selectArea.js
@@ +111,3 @@
+
+    _onButtonRelease: function(actor, event) {
+        this._destroy(this._getGeometry());

I'd like to see us fade out the area like we do with rubber bands in e.g. Nautilus.

Created attachment 230987 [details] [review]
Add a SelectArea() DBus method

--

Good idea - how about this?

Review of attachment 230987 [details] [review]:

Hm, I'm not sure if the fade-out is something that we should add on escape, and I'm also not sure if it will appear in the screenshot.

Created attachment 230997 [details] [review]
Add a SelectArea() DBus method

--

Don't fade out when cancelling with Escape.

Review of attachment 230997 [details] [review]:

::: js/ui/selectArea.js
@@ +125,3 @@
+        }
+
+        if (fade)

Tweener will do this for you automatically if you mark time as 0:

   { opacity: 0,
     time: fade ? 0.2 : 0,
     ... });

Created attachment 230999 [details] [review]
Add a SelectArea() DBus method

--

Good point; updated patch.

Review of attachment 230999 [details] [review]:

Sure.

Attachment 230999 [details] pushed as dd19459 - Add a SelectArea() DBus method