Rename `awful.popup` `hide_on_right_click` property to `hide_on_click` #3665

Aire-One · 2022-07-26T23:01:56Z

I stepped on this lately, the implementation of the hide_fct doesn't match the name since there is no check on what button is actually pressed.

It sounds more reasonable to me to rename the hide_on_right_click property to hide_on_click, because I feel the current behavior is more natural.

It was either: break the current behavior, or change the property name. Note that the latter can also preserve the API backwards compatibility by keeping the old property with an additional deprecation message.

I have also write some tests to make sure everything is working the as expected now.

codecov · 2022-07-26T23:12:16Z

Codecov Report

Merging #3665 (8ad78c7) into master (b16f628) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3665      +/-   ##
==========================================
+ Coverage   90.91%   90.94%   +0.02%     
==========================================
  Files         896      897       +1     
  Lines       56633    56689      +56     
==========================================
+ Hits        51489    51553      +64     
+ Misses       5144     5136       -8

Flag	Coverage Δ
gcov	`90.94% <100.00%> (+0.02%)`	⬆️
luacov	`93.68% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/awful/popup.lua	`94.17% <100.00%> (+1.03%)`	⬆️
lib/gears/object.lua	`88.63% <100.00%> (+0.54%)`	⬆️
spec/gears/object_spec.lua	`92.17% <100.00%> (+0.74%)`	⬆️
tests/test-awful-popup-hide-on-click.lua	`100.00% <100.00%> (ø)`
lib/wibox/widget/imagebox.lua	`95.53% <0.00%> (-0.90%)`	⬇️
lib/awful/widget/layoutlist.lua	`89.88% <0.00%> (+0.59%)`	⬆️
spawn.c	`86.16% <0.00%> (+3.64%)`	⬆️

sclu1034 · 2022-07-27T08:33:13Z

spec/awful/popup_spec.lua

+}
+_G.awesome = gtable.join(_G.awesome, { connect_signal = function() end })
+_G.screen = {
+    connect_signal = function() end,
+    set_index_miss_handler = function() end,
+    set_newindex_miss_handler = function() end,
+}
+_G.drawin = setmetatable(
+    gtable.join(gobject { enable_properties = true, enable_auto_signals = true }, {
+        set_index_miss_handler = function() end,
+        set_newindex_miss_handler = function() end,
+    }),
+    {
+        __call = function()
+            return {
+                connect_signal = function() end,
+            }
+        end,
+    }
+)
+package.loaded["wibox.drawable"] = setmetatable({}, {
+    __call = function()
+        return {
+            _inform_visible = function() end,
+            connect_signal = function() end,
+            set_bg = function() end,
+            set_fg = function() end,
+            draw = function() end,
+            set_widget = function() end,
+            get_widget = function() end,
+        }
+    end,
+})
+package.loaded["gears.debug"] =
+    { deprecate = function() end, deprecate_class = function() end }
+
+local popup = require "awful.popup"
+
+describe("awful.popup", function()
+    describe("hide_on_click", function()
+        it(
+            "Constructor without the param should not connect the hide_fct",
+            function()
+                local p = popup { widget = utils.widget_stub() }
+                assert.is_nil(p._signals["button::press"])
+            end
+        )
+
+        it("Constructor parameter should connect the hide_fct", function()
+            local p =
+                popup { widget = utils.widget_stub(), hide_on_click = true }
+            assert.is_true(
+                p._signals["button::press"].strong[p._private.hide_fct]
+            )
+        end)
+
+        it("Setter set_hide_click should connect/disconnect the the hide_fct", function()
+            local p = popup { widget = utils.widget_stub() }
+            assert.is_nil(p._signals["button::press"])
+
+            p:set_hide_on_click(true)
+            assert.is_true(
+                p._signals["button::press"].strong[p._private.hide_fct]
+            )
+
+            p:set_hide_on_click(false)
+            assert.is_nil(
+                p._signals["button::press"].strong[p._private.hide_fct]
+            )
+        end)
+
+        it(
+            "Legacy constructor parameter hide_on_right_click should still work",
+            function()
+                local p = popup {
+                    widget = utils.widget_stub(),
+                    hide_on_right_click = true,
+                }
+                assert.is_true(
+                    p._signals["button::press"].strong[p._private.hide_fct]
+                )
+            end
+        )
+
+        it("Legacy setter set_hide_on_right_click should still work", function()
+            local p = popup { widget = utils.widget_stub() }
+            assert.is_nil(p._signals["button::press"])
+
+            p:set_hide_on_right_click(true)
+            assert.is_true(
+                p._signals["button::press"].strong[p._private.hide_fct]
+            )
+
+            p:set_hide_on_right_click(false)
+            assert.is_nil(
+                p._signals["button::press"].strong[p._private.hide_fct]
+            )
+        end)
+    end)
+end)


Unit tests shouldn't test private properties and implementation details. They shouldn't know or care about those things.
Another red flag is the fact that you had to noop-stub half the widget system to get this to work. That will never be an accurate test of what's going on in the non-stubbed library.

The integration test in tests is the correct way to test functionality like that.

Maybe we could make a "global" monkey-patch of gears.object and it's CAPI variant to check if a signal is connected. Testing private APIs is bad, but testing at a message passing level is a very good thing. we have plenty of broken signals right now and very little tooling to detect them.

Please don't.

If the thing to test for is "if I do A, is signal X emitted?", that's fine. We don't need any monkey-patching for that, just a spy.new() passed to connect_signal and an assert.spy().was_called.

But if the thing to test is "if I do A, does thing X happen?", then the internal signal is an implementation detail that shouldn't be tested. What should be tested is "did X happen?". And that test should be agnostic of whether a signal, function or any other system was called for that to happen, now or in any future revision of the implementation.

The public contract for hide_on_click should not include "there is a private function, but you can still access it, and that function is then assigned to another private table based on the specific signal we use internally". There shouldn't be a test breaking because someone decided to make the function actually local, renamed it, or because the "when button is clicked" logic changed.

Hm, yeah. I didn't feel so good about these unit tests. I'm happy to see some feedback on it. Thank you both of you @sclu1034 and @Elv13.

First, to describe my goal with these unit tests, I want to make sure that setting the parameter, or calling the setter, would actually trigger the appropriate signal connection. So that's a "if I do A, does thing X happen?" as described by @sclu1034. The idea was to make sure the parameter is actually taken into account. (it was mostly in the regard of the recent PR about the with_shell parameter blatantly ignored by the implementation.)

Another red flag is the fact that you had to noop-stub half the widget system to get this to work.

I have to disagree with this one. In a unit test suite, you only load the module that the test targets. In this case, awful.popup. However, for the module to work, we need to have some tools from Awesome that just can't be here, like the CAPI. It just doesn't exist when we require("awful.popup") with busted.

Maybe we could make a "global" monkey-patch of gears.object and it's CAPI variant to check if a signal is connected.

We don't need any monkey-patching for that, just a spy.new() passed to connect_signal and an assert.spy().was_called.

Yup, we could have definition with spies in the preload.lua busted helper for this kind of objects.

The public contract for hide_on_click should not include "there is a private function, but you can still access it, and that function is then assigned to another private table based on the specific signal we use internally". There shouldn't be a test breaking because someone decided to make the function actually local, renamed it, or because the "when button is clicked" logic changed.

This sounds right, but it doesn't give a solution for the "I want to make sure that setting the parameter, or calling the setter, would actually trigger the appropriate signal connection" problem. Hm... This is actually an obvious example of "testing implementation details".

Maybe we could also make hide_on_click either a boolean or a key/value table of button id as keys and boolean as values? However, that's not required to merge this PR. It's just "umm, that would be nice!".

Making it a key/value table of button id as keys and boolean as values is basically a simplification (would it even be easier?) of the buttons property. Sooo 🤷

Making hide_on_click a boolean property can actually be a good idea. We can make it behave as a property by only adding a getter that does the self._signals["button::press"].strong[self._private.hide_fct] ~=nil test. ~~So the unit test would have to check that after setting the hide_on_click value, the getter returns the appropriate boolean value.~~ (this is actually a bad idea)

I guess this way, we can reconcile both parties.

Hm, I guess we could also just test that setting hide_on_click on the constructor will actually call the "set_hide_on_click" method.

Elv13

Thanks for this. I think we will need a full backward compatibility property for this (@deprecatedproperty).

Maybe we could also make hide_on_click either a boolean or a key/value table of button id as keys and boolean as values? However, that's not required to merge this PR. It's just "umm, that would be nice!".

As for the tests, I agree with @sclu1034 the private API part for the signals. However I am fine with all the shims. It tests what it aims to test. I really don't have a strict line in the sand for testing. I usually do the doc example because it has a bigger impact compared to the time they take to write. But it's a bad fit for this fix. The sequence is currently ill-suited for widgets, even if it's technically possible to abuse it for them. All other template cannot reflect the "click" action.

Elv13 · 2022-07-28T04:16:12Z

spec/awful/popup_spec.lua

+}
+_G.awesome = gtable.join(_G.awesome, { connect_signal = function() end })
+_G.screen = {
+    connect_signal = function() end,
+    set_index_miss_handler = function() end,
+    set_newindex_miss_handler = function() end,
+}
+_G.drawin = setmetatable(
+    gtable.join(gobject { enable_properties = true, enable_auto_signals = true }, {
+        set_index_miss_handler = function() end,
+        set_newindex_miss_handler = function() end,
+    }),
+    {
+        __call = function()
+            return {
+                connect_signal = function() end,
+            }
+        end,
+    }
+)
+package.loaded["wibox.drawable"] = setmetatable({}, {
+    __call = function()
+        return {
+            _inform_visible = function() end,
+            connect_signal = function() end,
+            set_bg = function() end,
+            set_fg = function() end,
+            draw = function() end,
+            set_widget = function() end,
+            get_widget = function() end,
+        }
+    end,
+})
+package.loaded["gears.debug"] =
+    { deprecate = function() end, deprecate_class = function() end }
+
+local popup = require "awful.popup"
+
+describe("awful.popup", function()
+    describe("hide_on_click", function()
+        it(
+            "Constructor without the param should not connect the hide_fct",
+            function()
+                local p = popup { widget = utils.widget_stub() }
+                assert.is_nil(p._signals["button::press"])
+            end
+        )
+
+        it("Constructor parameter should connect the hide_fct", function()
+            local p =
+                popup { widget = utils.widget_stub(), hide_on_click = true }
+            assert.is_true(
+                p._signals["button::press"].strong[p._private.hide_fct]
+            )
+        end)
+
+        it("Setter set_hide_click should connect/disconnect the the hide_fct", function()
+            local p = popup { widget = utils.widget_stub() }
+            assert.is_nil(p._signals["button::press"])
+
+            p:set_hide_on_click(true)
+            assert.is_true(
+                p._signals["button::press"].strong[p._private.hide_fct]
+            )
+
+            p:set_hide_on_click(false)
+            assert.is_nil(
+                p._signals["button::press"].strong[p._private.hide_fct]
+            )
+        end)
+
+        it(
+            "Legacy constructor parameter hide_on_right_click should still work",
+            function()
+                local p = popup {
+                    widget = utils.widget_stub(),
+                    hide_on_right_click = true,
+                }
+                assert.is_true(
+                    p._signals["button::press"].strong[p._private.hide_fct]
+                )
+            end
+        )
+
+        it("Legacy setter set_hide_on_right_click should still work", function()
+            local p = popup { widget = utils.widget_stub() }
+            assert.is_nil(p._signals["button::press"])
+
+            p:set_hide_on_right_click(true)
+            assert.is_true(
+                p._signals["button::press"].strong[p._private.hide_fct]
+            )
+
+            p:set_hide_on_right_click(false)
+            assert.is_nil(
+                p._signals["button::press"].strong[p._private.hide_fct]
+            )
+        end)
+    end)
+end)


Maybe we could make a "global" monkey-patch of gears.object and it's CAPI variant to check if a signal is connected. Testing private APIs is bad, but testing at a message passing level is a very good thing. we have plenty of broken signals right now and very little tooling to detect them.

Aire-One · 2022-08-06T13:32:09Z

I took an hour or two this morning to work a little more on this unit testing thing...

I ended up with something like :

        it(
            "Call set_hide_on_click with `true` should connect the hide_fct",
            function()
                local p = popup { widget = utils.widget_stub() }
                local spy_connect = spy.on(p, "connect_signal")
                local spy_disconnect = spy.on(p, "disconnect_signal")

                p:set_hide_on_click(true)
                assert.spy(spy_connect).was_called()
                assert.spy(spy_disconnect).was_not_called()
            end
        )

It seems cleaner since we now spy on the connect/disconnect signals. But I couldn't find a way to make the spies to work with the constructor, so I think the test is incomplete.

I decided to drop the unit-test for now. Since I couldn't find something that make me happy with it. I'll probably put some more time on it latter~~~.

On another note, I have added commit 687e5f8. It implements the appropriate getter, so p.hide_on_click is now a valid property where the user can write and read for real.

sclu1034

The version with spy.on(p, "connect_signal") would still be testing an implementation detail. Not as bad as the previous one, as no private values are accessed, but still not something a unit test should do.

sclu1034 · 2022-08-08T09:16:34Z

lib/awful/popup.lua

+    local button_press_signals = self._signals["button::press"]
+    return button_press_signals and button_press_signals.strong[self._private.hide_fct] or false


This depends on the internal implementation of gears.object, which something like awful.popup should know nothing about.
The proper way to do this would be to provide something like is_connected(signal_name, fn) in gears.object.

lib/gears/object.lua

Elv13 · 2022-10-02T00:11:30Z

lib/gears/object.lua

+-- @tparam function func The callback to check if connected.
+-- @treturn boolean Whether the signal is connected.
+-- @method is_weak_signal_connected
+function object:is_weak_signal_connected(name, func)


could we merge both strong and weak variants into one? I don't really see an use case where it would make a difference.

The implementation of the `hide_fct`actually doesn't check for the button pressed. And it sounds more user-friendly that it works this way. So, it is reasonable to rename the constructor parameter to `hide_on_click`, and also apply this same naming update to the setter. Note that the changes here should preserve full backward compatibility with deprecation warning.

This adds integration test to validate the behavior of `hide_on_click`.

sclu1034 · 2022-10-08T11:45:31Z

docs/common/object.ldoc

+
+--- Check if the callback is connected to the signal.
+--
+-- This function check both kind of signal connection (strong and weak).


Suggested change

-- This function check both kind of signal connection (strong and weak).

-- This function checks both kinds of signal connection (strong and weak).

sclu1034 · 2022-10-08T11:48:59Z

lib/gears/object.lua

+    local sig = find_signal(self, name)
+    return not not (sig and (sig.strong[func] or sig.weak[func]))


find_signal creates the entry when it doesn't exist, so you don't need to check if sig exists.

actionless · 2022-12-05T01:43:07Z

lib/awful/popup.lua

@@ -453,8 +465,8 @@ end
 -- @tparam string|table args.preferred_positions
 -- @tparam string|table args.preferred_anchors
 -- @tparam table|number args.offset The X and Y offset compared to the parent object
-- @tparam boolean args.hide_on_right_click Whether or not to hide the popup on
--  right clicks.
+-- @tparam boolean args.hide_on_click Whether or not to hide the popup on


what about turning it from boolean into int, so user could choose the mouse button?

sclu1034 suggested changes Jul 27, 2022

View reviewed changes

Elv13 requested changes Jul 28, 2022

View reviewed changes

Aire-One force-pushed the fix/apopup-hide_on_click branch from 144331b to 687e5f8 Compare August 6, 2022 13:23

sclu1034 suggested changes Aug 8, 2022

View reviewed changes

Aire-One force-pushed the fix/apopup-hide_on_click branch from 687e5f8 to ef46d3f Compare August 9, 2022 12:12

sclu1034 previously approved these changes Aug 9, 2022

View reviewed changes

Aire-One commented Aug 26, 2022

View reviewed changes

lib/gears/object.lua Outdated Show resolved Hide resolved

Elv13 reviewed Oct 2, 2022

View reviewed changes

Aire-One dismissed sclu1034’s stale review via 0af67d6 October 4, 2022 17:41

Aire-One added 4 commits October 4, 2022 19:41

test(popup): hide_on_click property

d536a50

This adds integration test to validate the behavior of `hide_on_click`.

feat(object): implement is(_weak)_signal_connected

cd8df4f

feat(popup): implement hide_on_click getter

40ec479

Aire-One force-pushed the fix/apopup-hide_on_click branch from 0af67d6 to 111ce05 Compare October 4, 2022 17:42

Aire-One added 2 commits October 4, 2022 19:49

feat(object): merge both is_signal_connected

2a16946

doc(object): add is_signal_connected to common doc

8ad78c7

Aire-One force-pushed the fix/apopup-hide_on_click branch from 111ce05 to 8ad78c7 Compare October 4, 2022 17:49

sclu1034 suggested changes Oct 8, 2022

View reviewed changes

actionless reviewed Dec 5, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename `awful.popup` `hide_on_right_click` property to `hide_on_click` #3665

Rename `awful.popup` `hide_on_right_click` property to `hide_on_click` #3665

Aire-One commented Jul 26, 2022 •

edited

Loading

codecov bot commented Jul 26, 2022 •

edited

Loading

sclu1034 Jul 27, 2022

Elv13 Jul 28, 2022

sclu1034 Jul 28, 2022

Aire-One Jul 31, 2022 •

edited

Loading

Aire-One Jul 31, 2022 •

edited

Loading

Elv13 left a comment

Elv13 Jul 28, 2022

Aire-One commented Aug 6, 2022

sclu1034 left a comment

sclu1034 Aug 8, 2022

Elv13 Oct 2, 2022

Aire-One Oct 4, 2022

sclu1034 Oct 8, 2022

sclu1034 Oct 8, 2022

actionless Dec 5, 2022

		local button_press_signals = self._signals["button::press"]
		return button_press_signals and button_press_signals.strong[self._private.hide_fct] or false

	-- This function check both kind of signal connection (strong and weak).
	-- This function checks both kinds of signal connection (strong and weak).

		local sig = find_signal(self, name)
		return not not (sig and (sig.strong[func] or sig.weak[func]))

Rename awful.popup hide_on_right_click property to hide_on_click #3665

Are you sure you want to change the base?

Rename awful.popup hide_on_right_click property to hide_on_click #3665

Conversation

Aire-One commented Jul 26, 2022 • edited Loading

codecov bot commented Jul 26, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aire-One Jul 31, 2022 • edited Loading

Choose a reason for hiding this comment

Aire-One Jul 31, 2022 • edited Loading

Choose a reason for hiding this comment

Elv13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aire-One commented Aug 6, 2022

sclu1034 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rename `awful.popup` `hide_on_right_click` property to `hide_on_click` #3665

Rename `awful.popup` `hide_on_right_click` property to `hide_on_click` #3665

Aire-One commented Jul 26, 2022 •

edited

Loading

codecov bot commented Jul 26, 2022 •

edited

Loading

Aire-One Jul 31, 2022 •

edited

Loading

Aire-One Jul 31, 2022 •

edited

Loading