awesomeWM / awesome

awesome window manager
https://awesomewm.org/
GNU General Public License v2.0
6.37k stars 597 forks source link

wibox.widget.slider "button::release" don't work with LMB #1241

Open r3lgar opened 7 years ago

r3lgar commented 7 years ago

Output of awesome --version:

awesome v3.5.2-2192-g333cd649-dirty (The Fox)
 • Compiled against Lua 5.1.5 (running with Lua 5.1)
 • D-Bus support: ✔
 • execinfo support: ✔
 • RandR 1.5 support: ✔
 • LGI version: 0.9.0

How to reproduce the issue:

test_slider = wibox.widget {
  bar_color = "#000000",
  handle_color = "#FFFFFF",
  handle_shape = shape.circle,
  value = 0,
  minimum = 0,
  maximum = 100,
  widget = wibox.widget.slider,
}

test_slider:connect_signal("button::release", function(w)
  naughty.notify({text = tostring(w.value)})
  return true
end)

Actual result:

Don't work with LMB (button_id == 1), only with others (2, 3, 4, 5).

Expected result:

Run callback function on LMB (button_id == 1) on "button::release" signal.

psychon commented 7 years ago

Sigh. Technical explanation of what happens: The slider widget internally connects to the button::press signal for button 1. It then starts a keygrabber to handle the "dragging". Due to this, the button release event is only given to the keygrabber and is not delivered to the widget. (The mouse pointer could e.g. be outside of the widget when the button is released.)

You could connect to the property::value-signal instead, but that will also fire when you do test_slider:set_value(0.1337)...

Why do you want to connect to this signal at all? Is the property::value-signal perhaps what you really want?

Elv13 commented 7 years ago
diff --git a/lib/wibox/widget/slider.lua b/lib/wibox/widget/slider.lua
index 14e724479..10d80b843 100644
--- a/lib/wibox/widget/slider.lua
+++ b/lib/wibox/widget/slider.lua
@@ -423,6 +423,9 @@ local function mouse_press(self, x, y, button_id, _, geo)

     capi.mousegrabber.run(function(mouse)
         if not mouse.buttons[1] then
+            local fw = geo.drawable:find_widgets(mouse.x, mouse.y)
+
+            self:emit_signal("button::release", mouse.x, mouse.y, mouse, {}, fw)
             return false
         end

It could be fixed by something like this (with the right arguments, this is both wrong and untested)

r3lgar commented 7 years ago

@psychon

I need to call external programm at end of action, not on each value changes. If I will flooding, it'll ban my widget.

Also in slider's documentation no word about property::value.

r3lgar commented 7 years ago

@Elv13

error: /usr/share/awesome/lib/wibox/widget/slider.lua:425: attempt to call method 'find_widgets' (a nil value)
2016-11-30 22:05:14 W: awesome: event_handle_mousegrabber:123: Stopping mousegrabber.
Elv13 commented 7 years ago

Also in slider's documentation no word about property::value.

The documentation doesn't document them because it assumes all properties have a signal going along with them. But yeah, someone with enough time should add them. It's already done in some other modules

**Signal:**

 * `property::...`

But this is a lot of boiler plate and out macro system (from CMake) isn't powerful enough to automate those (and ldoc doesn't support this, as this is totally Awesome specific)

@Elv13

I know, I said "something like this", I already knew it was broken. It's just an hint for someone who wish to write a proper patch. I think the problem here is geo.drawable:find_widgets(mouse.x, mouse.y), the extra drawable is the C one. But it still wont work right away.

r3lgar commented 7 years ago

I have not enough skills to fix that. Can it be done at all?

Elv13 commented 7 years ago

Can it be done at all?

Sure, by fixing the patch above. The slider is the only widget (so far) affected by this bug. Creating it was quite an hassel for us and it has to deal with 3 different coordinate space to work, but this isn't the issue here. Your issue is, as @psychon said, that the keygrabber is active when the event happen and it "eats it". So it has to be manaually sent from the keygrabber loop before it leaves. mouse::move/enter/leave ( also need to be emited to be compliant with the shared widget documentation.

For now, you can use mouse::leave. It will work as long as the cursor is over the widget when the press is released. This is a rather bad workaround as the mouse could be outside when it happens. But fixing the patch isn't hard, it only takes some time and we don't have any.

r3lgar commented 7 years ago

Should capi.mousegrabber get callback and run it (without Lua wrappers)? I think, problem is first in C code, then in Lua. Isn't it?

I don't understand C at the appropriate level (actually I do not understand at all), so I can't write the patch. Sigh.

Elv13 commented 7 years ago

No, there is no C involved. The patch above is mostly what you want. Just makes sure the coordinates system are compatible and your good to go.

psychon commented 7 years ago

@funeralismatic Does this help you?

diff --git a/lib/wibox/widget/slider.lua b/lib/wibox/widget/slider.lua
index 14e7244..ff8f6c6 100644
--- a/lib/wibox/widget/slider.lua
+++ b/lib/wibox/widget/slider.lua
@@ -423,6 +423,8 @@ local function mouse_press(self, x, y, button_id, _, geo)

     capi.mousegrabber.run(function(mouse)
         if not mouse.buttons[1] then
+            -- Not actually correct, but might be close enough
+            self:emit_signal("button::release", 42, 42, 1, {}, geo)
             return false
         end
r3lgar commented 7 years ago

@psychon

Does this help you?

Seems that works.

42

:laughing: